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 #35626 - Registration & proxy's registration_url #9464

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Oct 13, 2022

Use registration_url from Registration SP module
for registration command.

RFC: https://community.theforeman.org/t/rfc-host-registration-and-load-balancers/30462
Katello: https://projects.theforeman.org/issues/35627
Smart Proxy: https://projects.theforeman.org/issues/35639
Installer: TODO

@theforeman-bot
Copy link
Member

Issues: #35626

Copy link
Member

@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.

My biggest concern here is that the template URL is HTTP and not HTTPS. Normally registration uses HTTPS, right? Is that intended?

@stejskalleos
Copy link
Contributor Author

My biggest concern here is that the template URL is HTTP and not HTTPS. Normally registration uses HTTPS, right? Is that intended?

I wouldn't say it's intended, but if users use template_url for haproxy (as described in this comment) it can be https URL.

@ekohl
Copy link
Member

ekohl commented Oct 13, 2022

It indeed can be HTTPS, but in that case (AFAIK) you can't really kickstart since we don't set up certificates. At least back in the day Anaconda couldn't even retrieve its kickstart over HTTPS. Perhaps that's better now, but that's why it HTTP and not HTTPS. That's why I'd still suggest to introduce a new setting on the registration module. That way users don't have to choose between the two.

@ehelms
Copy link
Member

ehelms commented Oct 13, 2022

That's why I'd still suggest to introduce a new setting on the registration module. That way users don't have to choose between the two.

Expanding on this, the suggestion is to add a new setting into registration module (https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/registration.yml.example) that is named registration_url and be set directly at installation time and is reported back to Foreman and keeps it separate from the templates URL?

Does the registration workflow hit directly the templates URL?

@stejskalleos
Copy link
Contributor Author

Does the registration workflow hit directly the templates URL?

Yes, it's used in the end of registration when we are calling back to Foreman to update build status, in host_init_config_default template, nothing else.

Having registration_url for LB case sounds like a good idea, implementation shouldn't be big problem and it would solve the http problem with templates_url, so 👍 from me.

@ekohl
Copy link
Member

ekohl commented Oct 14, 2022

Expanding on this, the suggestion is to add a new setting into registration module (https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/registration.yml.example) that is named registration_url and be set directly at installation time and is reported back to Foreman and keeps it separate from the templates URL?

Exactly.

@stejskalleos stejskalleos changed the title Fixes #35626 - Use proxy template URL in registration Fixes #35626 - Use proxy registration_url in registration Oct 19, 2022
@stejskalleos stejskalleos changed the title Fixes #35626 - Use proxy registration_url in registration Fixes #35626 - Registration & proxy's registration_url Oct 19, 2022
Copy link
Member

@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.

ACK on the Ruby side, but I haven't done anything with the JS side for a while so I'd appreciate if someone else could take a look.

Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @stejskalleos 👍🏼

@Ron-Lavi
Copy link
Member

Should we try merging Katello/katello#10315, theforeman/smart-proxy#850 all at the same time? any other concerns before merging?

@stejskalleos
Copy link
Contributor Author

stejskalleos commented Oct 31, 2022

Should we try merging Katello/katello#10315, theforeman/smart-proxy#850 all at the same time? any other concerns before merging?

Katello can wait few days, but smart-proxy should go together with this PR

@ekohl ekohl merged commit 0c769b9 into theforeman:develop Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants