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 #32678 - katello_ca_consumer in registration template #8563

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Jun 1, 2021

Move rhsm_reconfigure script [0] from katello_consumer.rpm to
global_registration template so the rpm is not needed anymore

Migrated script is without support of RHEL5 and older
subscription-manager versions (0.96 and bellow)

[0] rhsm_reconfigure_script

@theforeman-bot
Copy link
Member

Issues: #32678

@ehelms
Copy link
Member

ehelms commented Jun 4, 2021

@jlsherrill @jturel @evgeni This touches on areas you all have history and familiarity with, and I am raising questions about how we can tweak this to have a clean implementation moving forward and leave the baggage back in https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/lib/puppet/provider/rhsm_reconfigure_script/rhsm_reconfigure_script.rb#L32. I would appreciate if y'all could each review thoroughly, making sure to look back at the current script to ensure we do not have any regressions but that we also do not carry forward any cruft if it can be avoided.

@evgeni
Copy link
Member

evgeni commented Jun 14, 2021

One thing that I realized while looking at this, but that is not explicitly related.

There are many scripts out there, which check "is this machine subbed to katello/satellite" and if we change anything in the local representation (on the client machine), those might fail. I know we want to get rid of the RPM, and that's fine, but we need to very loudly communicate this and also make sure all other "markers" (like the /etc/rhsm/ca/katello-server-ca.pem path) should remain the same.

Example: https://github.com/Katello/katello-client-bootstrap/blob/e6ce46b62fbb3414a2065facf5cb420801ac3fe5/bootstrap.py#L355-L360

@ehelms
Copy link
Member

ehelms commented Jun 14, 2021

There are many scripts out there, which check "is this machine subbed to katello/satellite" and if we change anything in the local representation (on the client machine), those might fail. I know we want to get rid of the RPM, and that's fine, but we need to very loudly communicate this and also make sure all other "markers" (like the /etc/rhsm/ca/katello-server-ca.pem path) should remain the same.

Example: https://github.com/Katello/katello-client-bootstrap/blob/e6ce46b62fbb3414a2065facf5cb420801ac3fe5/bootstrap.py#L355-L360

These paths or at least names, will change in the future. I think then we should aim for some more reliable, and "API"-like way of indicating that a system is registered to a Foreman. Either laying down a file somewhere on the machine, or working to get a config value in sub-man/rhc that indicates this going forward. I'll happily go write up an issue somewhere if we can agree to what we think is the best approach to that. I think today if you were using pure Global Registration without Katello or Puppet, you would have no indicator that the system was tied to a Foreman instance right? So would a file laid down somewhere that is more generic make sense for broader use cases?

@evgeni
Copy link
Member

evgeni commented Jun 14, 2021

These paths or at least names, will change in the future.

I think we should then try to teach at least bootstrap about the new paths and/or the API-like thing you mention, as even if it's deprecated, I see users using it in the future, and it should properly detect the system being subscribed.

And yes, today, without Kat/Pup, you have no way two know who manages the system.

@ekohl
Copy link
Member

ekohl commented Jun 14, 2021

This is going a bit off topic, but I'm not sure where else to discuss it.

In Puppet speak, I think we should come up with some fact that returns the host. From a clients perspective, you know the content and RHSM URLs for sure. Is that sufficient or do you want to know more?

From a configuration management perspective, what I'd really like is for the ENC to provide this information. Then you can use Puppet/REX/Ansible to correct the registration. This should allow users to move hosts between content proxies.

If you provide it in both the ENC and as facts, you can also detect out-of-sync configs and correct them.

Back to this PR: if you have that info in the ENC, you already have the helpers to provide it as a macro here.


subscription-manager register <%= '--force' if @force %> \
--org='<%= @organization.label %>' \
--activationkey='<%= activation_keys %>' || <%= @ignore_subman_errors ? 'true' : 'cleanup_and_exit 1' %>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I should know this, does this mean that I can only register with an activation key? What happens if I want to register with username and password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, registration with activation key is the only workflow supported right now. If users wants to use login & password they have to edit the template and add parameters manually

Copy link
Member

Choose a reason for hiding this comment

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

Is username and password support on the Roadmap?

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, bootstrap doesn't support username/pass either today.

Copy link
Member

Choose a reason for hiding this comment

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

Bootstrap does so many things and that is one it does not do? TIL. Let me try to just write out the scenario that I am envisioning and ask a question at the end for discussion of how we want things to work for users.

The move to have GR directly re-configure subscription-manager allows us to drop the bootstrap RPM and centralize on the GR API as the way to register. Today, users can and are used to installing the bootstrap RPM and then calling subscription-manager to register a host with username and password or activation key. This current design enforces using an activation key.

Is that a design choice we want to make? And if yes, how would it affect users who are used to username and password?

Copy link
Member

Choose a reason for hiding this comment

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

I think that in our template we shouldn't really care about username/password. AKs do have benefits and I can imagine that at some point we actually generally a new key on the fly and automatically expire it after it's been used when this workflow is used. Like a one-time-activation-key. Not something to implement here, but having a registration workflow inside Foreman that supports it does make that flow possible.

Copy link
Member

Choose a reason for hiding this comment

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

I know this is "registration", but throwing the idea out there anyway. If a user does not supply an activation key we could opt to configure subscription-manager but not register with subscription-manager and output a message saying this must be done manually by calling subscription-manager. That would allow for this existing workflow to work.

I am OK if we drop username/password support. I would want us to be clear about that and call it out as a deprecated or removed (depending on timeline) feature for users in our release notes.

I'd like to also get @jlsherrill and @jturel to think on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am too kinda hesitant to completely remove this functionality. Its generally the scenario that developers use, and i wouldnt' be surprised if users did too. Its even mentioned in the docs: https://docs.theforeman.org/nightly/Content_Management_Guide/index-katello.html

I like the workflow you mention, if an activation key isn't present, print out the sub-man command without the activation key and let the user run it, alternatively, it could run the command and let them input their user/pass directly?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should print a command to run: the registration should either work (and be complete) or fail for a technical reason. It shouldn't start with a TODO. I'm also hesitant to make the script interactive.

Given bootstrap doesn't support it either, are we really dropping anything? It feels more like a possible future RFE.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the above command has feature-parity with bootstrap, everything else is RFE and/or "not the main workflow we support, you can script it yourself"

@ares
Copy link
Member

ares commented Jun 17, 2021

Back to this PR: if you have that info in the ENC, you already have the helpers to provide it as a macro here.

@ekohl while I like the idea of adding an info provider that would define the host rhsm source, this would only be useful for existing hosts. We're talking about registration template here, the host at that point does not exist. We'd only have this available in host-init-config template (the second step) but that's after we configured the rhsm already. It would be great for ongoing configuration of a registered machine.

@ekohl
Copy link
Member

ekohl commented Jun 17, 2021

@ekohl while I like the idea of adding an info provider that would define the host rhsm source, this would only be useful for existing hosts. We're talking about registration template here, the host at that point does not exist. We'd only have this available in host-init-config template (the second step) but that's after we configured the rhsm already. It would be great for ongoing configuration of a registered machine.

Correct, but my point is that I want some function that returns the correct value and use that function wherever you need the URL. That could be an ENC or a template macro. That gives us a single place of truth.

Hardcoding it in the template is IMHO always wrong. Perhaps should be provided via the context with a TODO to get it directly from Katello. That way we're at least prepared for the future and template authors know not to rely on hardcoded paths.

@stejskalleos
Copy link
Contributor Author

Rebased & updated,
fixed issue with copying katello-ca cert & added URL helper for rhsm_base_url, see Katello/katello#9417

@ehelms
Copy link
Member

ehelms commented Jul 21, 2021

I opened a PR to add some bats tests for global registration: theforeman/forklift#1391
This is not a blocker to this PR but rather inspired by the PR that we should add tests across the workflows to ensure as we transition our expectations continue to work.

@stejskalleos
Copy link
Contributor Author

Rebased & Updated

  • Now creating backup of rhsm.conf
  • added method for choosing package manager
  • created redmine task for cleanup & rollback methods

@ehelms
Copy link
Member

ehelms commented Jul 28, 2021

[test upgrade]

ehelms
ehelms previously approved these changes Jul 28, 2021
Copy link
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

I would like to get theforeman/forklift#1391 resolved and available so that we have assurances that changes like this in the future don't break our expected workflows.

@ares
Copy link
Member

ares commented Aug 12, 2021

will require a rebase now

@stejskalleos
Copy link
Contributor Author

Rebased & updated (e2918f9)

@ehelms
Copy link
Member

ehelms commented Aug 31, 2021

[test katello]

@stejskalleos
Copy link
Contributor Author

Rebased & updated: fixed following errors when registering CentOS 8:

cp: cannot stat '/etc/rhsm/rhsm.conf': No such file or directory
cp: cannot create regular file '/etc/rhsm/ca/katello-server-ca.pem': No such file or directory
chmod: cannot access '/etc/rhsm/ca/katello-server-ca.pem': No such file or directory

@stejskalleos
Copy link
Contributor Author

Rebased with changes from #8789

Move `rhsm_reconfigure` script from `katello_consumer.rpm` to
`global_registration` template so the `rpm` is not needed anymore

Migrated script is without support of RHEL5 and older
`subscription-manager` versions (0.96 and bellow)
@stejskalleos
Copy link
Contributor Author

@evgeni @ekohl @jlsherrill @ehelms

PR updated, rebased & all green.
Can we merge? It's been here for a while & I have some other PR waiting for this one.

Comment on lines +47 to +49
else
PKG_MANAGER='yum'
fi
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to support Fedora <21

@evgeni
Copy link
Member

evgeni commented Oct 5, 2021

I think all my points were addressed, so 👍

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Given two acks and the fact I've tested the functionality I think this can finally get it! @ehelms bats tests were also already merged, so I'm merging this.

Thanks @stejskalleos for a nice improvement, we can now officially deprecate the katello_ca_consumer rpm. Thanks @ehelms @ekohl and @evgeni for reviewing.

@ares ares merged commit 4062e17 into theforeman:develop Oct 8, 2021
@ezr-ondrej
Copy link
Member

Is this worth for adding a Headline feature?

@ekohl
Copy link
Member

ekohl commented Oct 13, 2021

For Foreman itself I wonder if it's useful since it's heavily reliant on Katello. Katello should certainly mention this in their release notes.

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.

8 participants