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 #37744: Line up Foreman and Katello registration data #10282

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Areyus
Copy link

@Areyus Areyus commented Aug 20, 2024

Without the interface being created within Foreman, global registration with a REX interface selected failes on Katello instaces due to the interface not existing within Foreman.
Operatingsystem ID is now also included in register_katello_host to ensure data parity between both workflows.

Without the interface being created within Foreman, global registration
with a REX interface selected failes on Katello instaces due to the
interface not existing within Foreman.
@Areyus
Copy link
Author

Areyus commented Aug 20, 2024

Test failures look unrelated as far as I can tell

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.

I think the biggest reason there was some difference is that it uses subscription-manager to register the host. That already sends facts like the OS and interfaces.

Did you notice any problems or was it only to have the 2 blocks match?

<%= " --data 'host[lifecycle_environment_id]=#{@lifecycle_environment_id}' \\\n" if @lifecycle_environment_id.present? -%>
<%= " --data host[interfaces_attributes][0][identifier]=#{shell_escape(@remote_execution_interface)} \\\n" if @remote_execution_interface.present? -%>
Copy link
Member

Choose a reason for hiding this comment

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

Why escape this instead of quoting?

Suggested change
<%= " --data host[interfaces_attributes][0][identifier]=#{shell_escape(@remote_execution_interface)} \\\n" if @remote_execution_interface.present? -%>
<%= " --data 'host[interfaces_attributes][0][identifier]=#{@remote_execution_interface}' \\\n" if @remote_execution_interface.present? -%>

I suspect you copied it from 2 lines below, but now I wonder why that was the way it was.

Copy link
Author

Choose a reason for hiding this comment

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

I simply copied what register_host() did. I also asked myself why it was coded this way but simply assumed the original author probably has probably thought this through enough and I wanted to avoid breaking any weird escaping edge-cases by accident.

Copy link
Author

Choose a reason for hiding this comment

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

Looking at the PR that intoduced this line and the linked issue, this seems to indeed stem from some edge-cases with escaping certain characters, specifically single quotes:
https://projects.theforeman.org/issues/33033
#8687

I have no idea if single quotes are a valid part of an interface name

Copy link
Author

Choose a reason for hiding this comment

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

@ekohl do you have any more thoughts on this? With the reasoning from the PR that originally introduced this syntax, I would opt towards leaving shell_escape in just to be save.

@Areyus
Copy link
Author

Areyus commented Aug 20, 2024

I think the biggest reason there was some difference is that it uses subscription-manager to register the host. That already sends facts like the OS and interfaces.

Did you notice any problems or was it only to have the 2 blocks match?

Should have probably described it here in greater depth, too, but as described in the linked issue global registration does not work on Katello instances if you use the "Remote Execution Interface" parameter without sending at least the REX interface name. Now that I read your comment and thought about it in this context, this might be caused by us disabling inteface updates from facts?
I did not experience any issues with the missing OS information.

@ekohl
Copy link
Member

ekohl commented Aug 20, 2024

Now that I read your comment and thought about it in this context, this might be caused by us disabling inteface updates from facts?

That sounds very plausible.

I did not experience any issues with the missing OS information.

We also have a setting to disable OS updating from facts. You probably have that turned on, right? I suspect that if that's turned off, you'd have the same problem.

@Areyus
Copy link
Author

Areyus commented Aug 20, 2024

I did not experience any issues with the missing OS information.

We also have a setting to disable OS updating from facts. You probably have that turned on, right? I suspect that if that's turned off, you'd have the same problem.

Indeed, we have turned that setting on for our instances. I did a bit of testing in our test instance and I could not get it to break, but what I saw looked more like "Ignore facts for operating system" setting to be broken rather than registration properly working without OS info. Despite not having an OS specified in the registration or on the hostgroup, the created host ended having the correct OS set in Foreman.

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.

2 participants