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 #32835 - Add RHSM and content URL as info providers #9413

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jun 18, 2021

This introduces two Smart Proxy extension methods and exposes them as info providers. This allows clients to use the ENC API to (re)configure subscription-manager.

First of all, this implements logic to determine the RHSM URL. This is constructed by looking at Pulp. While this is really the wrong way, there is currently no alternative. (I started https://github.com/ekohl/smart_proxy_rhsm a while back and that could solve it - another is to bolt it onto the Pulp module).

Ideally this would be exposed as a setting as well. There is a plan to align everything on port 443. If the Smart Proxy exposes a setting, Katello doesn't need to guess and a smooth migration can be realized.

This all becomes more important when the host subscription RPM is phased out.

Then it also introduces pulp_content_url which is already a setting that's exposed via the Smart Proxy Pulp feature. This is also used in the various places where it's needed.

It does highlight a missing setting for pulp_ansible content. Again, Katello should not have to guess URLs but discover it from the infrastructure.

Another potential use case that this allows is using REX to move a host from one content source to another.

Currently a draft since I have not verified this at all. However, I did want to link it and share it. What's also probably missing is provisioning macros.

@theforeman-bot
Copy link

Issues: #32835

Copy link
Member Author

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm not sure if I can really finish this, but this is IMHO important for our enterprise readiness meetings that we've been having. There's also some features that are starting to depend on this information being present (and correct).

  • Host registration
  • Removal of the registration RPM
  • Aligning RHSM on port 443 for all services

app/lib/katello/resources/registry.rb Outdated Show resolved Hide resolved
app/models/katello/concerns/smart_proxy_extensions.rb Outdated Show resolved Hide resolved
elsif ansible_collection?
"#{scheme}://#{pulp_uri.host.downcase}/pulp_ansible/galaxy/#{relative_path}/api"
pulp_uri.path = "/pulp_ansible/galaxy/#{relative_path}/api"
Copy link
Member Author

Choose a reason for hiding this comment

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

This also needs to be exposed as a setting by smart_proxy_pulp

@ekohl
Copy link
Member Author

ekohl commented Jun 21, 2021

Related PR: #9417

@ekohl
Copy link
Member Author

ekohl commented Jul 9, 2021

That has been merged. I'll rebase this on #9450 once that's been merged.

@jturel
Copy link
Member

jturel commented Jul 26, 2021

9450 is merged! please drop this out of Draft state when it's ready for review :)

@jturel
Copy link
Member

jturel commented Aug 10, 2021

@ekohl this is on our board as Ready for Review but I'm gonna move it to our backlog until it's out of draft state. Do you intend to have this ready for 4.2?

@ekohl
Copy link
Member Author

ekohl commented Sep 7, 2021

I'd love to, but I can't promise anything. Recently theforeman/smart_proxy_pulp@b6582ef was introduced in smart_proxy_pulp so at least the prerequisites are merged. However, my current priority is dealing with the Puppet extraction. I'd appreciate it if someone else could take over.

cc @wbclark

@jturel
Copy link
Member

jturel commented Jan 5, 2022

Should this be closed / backlogged until someone can address it or it's explicitly required?

@ekohl ekohl force-pushed the 32835-rhsm-pulp-info-providers branch from 6fa02c5 to a058777 Compare January 6, 2022 13:51
@ekohl
Copy link
Member Author

ekohl commented Jan 6, 2022

Given a lot of the related work has been merged already, this is now actually quite trivial. I must admit I haven't tested it since I don't have a development environment, but it should be quite easy. It's exposed in /api/hosts/:id/enc. If a content source is assigned to it, the URLs should be present.

The other (unrelated work) will be submitted as a separate PR.

@ekohl ekohl mentioned this pull request Jan 6, 2022
@ekohl
Copy link
Member Author

ekohl commented Jan 6, 2022

The other (unrelated work) will be submitted as a separate PR.

#9878

@jturel
Copy link
Member

jturel commented Jan 6, 2022

Found this while testing:

16:15:42 rails.1   | 2022-01-06T16:15:42 [I|app|b8c7adba] Started GET "/api/v2/hosts/4/enc" for 127.0.0.1 at 2022-01-06 16:15:42 +0000
16:15:42 rails.1   | 2022-01-06T16:15:42 [I|app|b8c7adba] Processing by Api::V2::HostsController#enc as JSON
16:15:42 rails.1   | 2022-01-06T16:15:42 [I|app|b8c7adba]   Parameters: {"apiv"=>"v2", "id"=>"4"}
16:15:42 rails.1   | 2022-01-06T16:15:42 [I|app|b8c7adba] Authorized user admin(Admin User)
16:15:42 rails.1   | 2022-01-06T16:15:42 [W|app|b8c7adba] Action failed
16:15:42 rails.1   | 2022-01-06T16:15:42 [I|app|b8c7adba] Backtrace for 'Action failed' error (ActiveModel::MissingAttributeError): can't write unknown attribute `rhsm_url`
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activemodel-6.0.3.7/lib/active_model/attribute.rb:206:in `with_value_from_database'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activemodel-6.0.3.7/lib/active_model/attribute_set.rb:49:in `write_from_user'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.0.3.7/lib/active_record/attribute_methods/write.rb:43:in `_write_attribute'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.0.3.7/lib/active_record/attribute_methods/write.rb:36:in `write_attribute'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.0.3.7/lib/active_record/attribute_methods.rb:337:in `[]='
16:15:42 rails.1   |  b8c7adba | /home/vagrant/katello/app/models/katello/host/info_provider.rb:21:in `host_info'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/app/models/concerns/host_info_extensions.rb:27:in `block in info'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/app/models/concerns/host_info_extensions.rb:25:in `each'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/app/models/concerns/host_info_extensions.rb:25:in `info'
16:15:42 rails.1   |  b8c7adba | /home/vagrant/foreman/app/controllers/api/v2/hosts_controller.rb:182:in `enc'

@ekohl ekohl force-pushed the 32835-rhsm-pulp-info-providers branch from a058777 to 2894bd9 Compare January 6, 2022 16:21
@ekohl
Copy link
Member Author

ekohl commented Jan 6, 2022

Oh that's my bad. They should be parameters rather than top level. Updated now.

This exposes the RHSM and content URL as info providers. This allows
clients to use the ENC API to (re)configure subscription-manager. This
all becomes more important when the host subscription RPM is phased out.
@ekohl ekohl force-pushed the 32835-rhsm-pulp-info-providers branch from 2894bd9 to 6b31eee Compare January 6, 2022 16:41
@@ -16,6 +16,15 @@ def host_info
if host.content_facet.present?
info['parameters']['kickstart_repository'] = host.content_facet.kickstart_repository.try(:label)
end

if (rhsm_url = host.content_source&.rhsm_url)
Copy link
Member

Choose a reason for hiding this comment

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

I learned that simply registering a content host directly to the server does not establish a content source and so these won't be set. Is that compatible with how you see this being used, or should they get the values from the default smart proxy at that point? Or is it a bug that content_source is nil in that scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would consider that a bug but I'm not sure how others see it. IMHO every host that has content should have a content_source assigned. Otherwise you always need to remember to do host.content_source || pulp3_primary.

It also means that #9780 may be based on an incorrect assumption and currently can't be used to reconfigure if the host is assigned to the primary.

Copy link
Member

Choose a reason for hiding this comment

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

#9780 works OK. I can create an issue to track that unless you want to dive into it on this PR

@jturel
Copy link
Member

jturel commented Jan 24, 2022

@ekohl I don't think this needs to be a draft any longer!

@ekohl ekohl marked this pull request as ready for review January 24, 2022 17:16
@ekohl
Copy link
Member Author

ekohl commented Jan 24, 2022

Then I'd like it if this was merged.

@jturel
Copy link
Member

jturel commented Jan 24, 2022

Just realized the katello tests failed but can't see the build any longer. running again.

[test katello]

@jturel jturel self-assigned this Jan 24, 2022
@jturel jturel merged commit f3daacd into Katello:master Jan 24, 2022
@ekohl ekohl deleted the 32835-rhsm-pulp-info-providers branch January 24, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants