Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36310 - Migrate off ContentViews generated for repository export #11144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,16 @@ def enabled_repos

#api :POST, "/environments/:environment_id/consumers", N_("Register a consumer in environment")
def consumer_create
host = Katello::RegistrationManager.process_registration(rhsm_params, find_content_view_environments)
content_view_environments = find_content_view_environments
if content_view_environments.any? { |cve| cve.content_view.generated_for_repository_export? }
render json: {
displayMessage: _("Cannot register with a content view generated for repository export"),
errors: [_("Cannot register with a content view generated for repository export")]
}, status: :bad_request
return
end

host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be find_content_view_environments (see L#321 below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it back. I don't see this path being called from subman but may be some one who is curl posting to this URL directly will be calling this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, still the same issue:

[ActiveRecord::RecordInvalid] exception expected, not
Class: <ActiveModel::UnknownAttributeError>
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this is what it was originally before my PR:

    def consumer_create
      host = Katello::RegistrationManager.process_registration(rhsm_params, find_content_view_environments) # note second arg here, calls find_content_view_environments method which is defined later in the same file

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

My PR had:

    def consumer_create
      content_view_environments = find_content_view_environments # content_view_environments is defined here
      if content_view_environments.any? { |cve| cve.content_view.generated_for_repository_export? }
        render json: {
          displayMessage: _("Cannot register with a content view generated for repository export"),
          errors: [_("Cannot register with a content view generated for repository export")]
        }, status: :bad_request
        return
      end

      host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments) # second arg here now uses above definition

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

And yours had:

    def consumer_create
      host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments) # but content_view_environments wasn't defined as in mine

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

So I wasn't suggesting that you need to reinstate my version of the change, just that you never FULLY reverted it to the original

That said, it seems the current issue in the tests is occurring regardless. I'm not sure what is the cause

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ActiveRecord::RecordInvalid] exception expected, not
Class: ActiveModel::UnknownAttributeError
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">

can you share the stack trace for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you share the stack trace for this.

I kicked off a re-run in CI of the failed tests to be sure:

 Error: Failure: test_0011_should not allow generated content view to be associated with activation key

[ActiveRecord::RecordInvalid] exception expected, not
Class: <ActiveModel::UnknownAttributeError>
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">
---Backtrace---
katello/test/models/activation_key_test.rb:114:in `block (2 levels) in <class:ActivationKeyTest>'
katello/test/models/activation_key_test.rb:113:in `block in <class:ActivationKeyTest>'
---------------

It looks like the test needs to try updating convent_view_environments instead


host.reload

Expand Down Expand Up @@ -413,7 +422,8 @@ def find_activation_keys
def update_environments_from_facts(param_environments)
return if param_environments.blank?
new_envs = param_environments.map do |env|
get_content_view_environment("cp_id", env['id'])
cve = get_content_view_environment("cp_id", env['id'])
cve unless cve&.content_view&.generated_for_repository_export?
end
new_envs.compact!
Rails.logger.debug "Setting new content view environments for host #{@host.to_label}: #{new_envs.map(&:label)}"
Expand Down
47 changes: 47 additions & 0 deletions db/migrate/20240815080259_migrate_off_generated_content_views.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
class MigrateOffGeneratedContentViews < ActiveRecord::Migration[6.1]
class FakeCVECF < ApplicationRecord
self.table_name = 'katello_content_view_environment_content_facets'
end

def up
say_with_time "Migrating hosts off generated content views" do
migrate_hosts
end
end

def down
say "This migration cannot be reversed", true
end

private

def migrate_hosts
say_with_time "Migrating hosts..." do
generated_content_views = Katello::ContentView.generated_for_repository

facets = Katello::Host::ContentFacet.joins(:content_view_environments).
where(content_view_environments: { content_view: generated_content_views })
facets.all.each do |content_facet|
valid_cves = content_facet.content_view_environments.where.not(content_view: generated_content_views)
if valid_cves.empty?
organization = content_facet.host.organization
default_cve = organization.content_view_environments.find_by(lifecycle_environment: organization.library,
content_view: organization.default_content_view)
if default_cve
FakeCVECF.where(content_facet_id: content_facet).delete_all
FakeCVECF.create!(content_facet_id: content_facet.id,
content_view_environment_id: default_cve.id)
say "Replaced all content views with Default Organization View for host #{content_facet.host.name}", true
else
say "No Default Organization View found for host #{content_facet.host.name}. Skipping.", true
end
else
FakeCVECF.where(content_facet_id: content_facet).
where.not(content_view_environment_id: valid_cves).
delete_all
say "Removed offending content views for host #{content_facet.host.name}", true
end
end
end
end
end
29 changes: 27 additions & 2 deletions test/controllers/api/rhsm/candlepin_proxies_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ module Katello
end
end

describe "register with a lifecycle environment" do
describe "register with a single lifecycle environment" do
before do
@facts = { 'network.hostname' => 'somehostname'}
@content_view_environment = ContentViewEnvironment.find(katello_content_view_environments(:library_default_view_environment).id)
Expand Down Expand Up @@ -122,7 +122,7 @@ module Katello
assert_response 400
end

it "should not register" do
it "should not register with dead services" do
::Katello::RegistrationManager.expects(:check_registration_services).returns(false)
::Katello::RegistrationManager.expects(:process_registration).never

Expand All @@ -131,6 +131,31 @@ module Katello

assert_response 500
end

it "should not register with a content view generated for repository export" do
organization = Organization.find(taxonomies(:empty_organization).id)
library = organization.library

generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => organization)
generated_cve = FactoryBot.create(:katello_content_view_environment,
:content_view => generated_cv,
:environment => library)

Resources::Candlepin::Consumer.stubs(:get)
::Katello::RegistrationManager.expects(:check_registration_services).returns(true)
::Katello::RegistrationManager.expects(:process_registration).never

post(
:consumer_create,
:params => {
:organization_id => organization.label,
:environment_id => generated_cve.cp_id,
:facts => @facts
}
)

assert_response 400
end
end

describe "update enabled_repos" do
Expand Down
10 changes: 10 additions & 0 deletions test/models/activation_key_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,16 @@ def setup
end
end

test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)

exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view: generated_cv)
end

assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end
Comment on lines +110 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)
exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view: generated_cv)
end
assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end
test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)
generated_cve = FactoryBot.create(:katello_content_view_environment, :content_view => generated_cv, :environment => katello_environments(:library))
exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view_environments: [generated_cve])
end
assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end

Something along these lines looks necessary due to the changes in 746bda7#diff-d7dd2b8980210ef5f81a8ac4d2bfd2384210cf326a1b165a5b6869b0c661a241


def test_search_name
activation_keys = ActivationKey.search_for("name = \"#{@dev_staging_view_key.name}\"")
assert_includes activation_keys, @dev_staging_view_key
Expand Down
11 changes: 11 additions & 0 deletions test/models/host/content_facet_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,17 @@ def test_audit_for_content_facet
content_facet_rec = host1.associated_audits.where(auditable_id: content_facet1.id)
assert content_facet_rec, "No associated audit record for content_facet"
end

def test_content_view_environments_not_generated_for_repository_export
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => view.organization)
generated_cve = FactoryBot.create(:katello_content_view_environment, :content_view => generated_cv, :environment => library)

exception = assert_raises(ActiveRecord::RecordInvalid) do
content_facet.content_view_environments = [generated_cve]
end

assert_includes exception.message, "Content view '#{generated_cv.name}' is a generated content view, which cannot be assigned to hosts or activation keys."
end
end

class ContentFacetErrataTest < ContentFacetBase
Expand Down
Loading