Expose RHSM configuration as BP customization (COMPOSER-2308)

[thozza]

RHSM in the image definitions is the last difference between RHEL RHUI images on el9 / el10, which can't be expressed as a BP customization. To enable the unification of RHUI and non-RHUI images and simplification of the image definitions, expose the RHSM configuration as a BP customization.

I considered naming the high-level customization Subscription, however the name is already used by a customization exposed only by the Cloud API in osbuild-composer. So using it for the BP customization would make it clash with the existing one and look different for on-prem vs. service scenario.

Notable changes

• I introduced an internal implementation of the RHSM configuration and replaced the osbuild stage options with it in the distro and pipeline implementations.
• Simplified the OSCustomizations by having only a single instance of the RHSM configuration, which should be applied to the image, instead of having two and deciding which one to use based on the fact if the image should be subscribed or not. The decision logic is now moved to the function which produces OSCustomization instance from ImageConfig, ImageOptions and BP customizations.

Open questions

• I'm not sure if we want to simply override the default RHSM configuration from the image definition if the RHSM configuration is provided also as a BP customization, or merge them with the BP customization having precedence. Overriding would mean that values set in the default image configuration, but not set in the BP would disappear from the produced manifest. However, overriding would make the implementation much simpler, because the merging requires quite a lot of additional code, which I don't like.

[achilleas-k]

• I introduced an internal implementation of the RHSM configuration and replaced the osbuild stage options with it in the distro and pipeline implementations.
• Simplified the OSCustomizations by having only a single instance of the RHSM configuration, which should be applied to the image, instead of having two and deciding which one to use based on the fact if the image should be subscribed or not. The decision logic is now moved to the function which produces OSCustomization instance from ImageConfig, ImageOptions and BP customizations.

❤️ ❤️

Open questions

• I'm not sure if we want to simply override the default RHSM configuration from the image definition if the RHSM configuration is provided also as a BP customization, or merge them with the BP customization having precedence. Overriding would mean that values set in the default image configuration, but not set in the BP would disappear from the produced manifest. However, overriding would make the implementation much simpler, because the merging requires quite a lot of additional code, which I don't like.

Yeah, I'm not sure either. There's a lot of code in there to accommodate this and I'm wondering if it's worth it. I think this blueprint customization is mostly for us (I don't expect users to be fiddling with RHSM configs) and the brew build configs will be pretty stable, not changing much if at all. Given all that, it's probably not worth having a bunch of code to support conveniently changing a small subset of the configuration. If this was a more user-facing customization, I'd argue for making it easier to tweak individual options without completely changing the config, but if I'm right that it's not, maybe simplifying the code (and the behaviour) is better in the long run.

[achilleas-k]

Requesting changes for the package move (from pkg/subscription to pkg/customizations/subscription).
Otherwise LGTM (pending discussion about merging vs overriding the full config).

[kingsleyzissou]

Open questions

• I'm not sure if we want to simply override the default RHSM configuration from the image definition if the RHSM configuration is provided also as a BP customization, or merge them with the BP customization having precedence. Overriding would mean that values set in the default image configuration, but not set in the BP would disappear from the produced manifest. However, overriding would make the implementation much simpler, because the merging requires quite a lot of additional code, which I don't like.

Yeah, I'm not sure either. There's a lot of code in there to accommodate this and I'm wondering if it's worth it. I think this blueprint customization is mostly for us (I don't expect users to be fiddling with RHSM configs) and the brew build configs will be pretty stable, not changing much if at all. Given all that, it's probably not worth having a bunch of code to support conveniently changing a small subset of the configuration. If this was a more user-facing customization, I'd argue for making it easier to tweak individual options without completely changing the config, but if I'm right that it's not, maybe simplifying the code (and the behaviour) is better in the long run.

Just to add to the open question with additional questions. If we go the route of overriding the configs this means that the YumPlugins config will always get removed. Would we want to at least check for that and merge it, since that would be pretty straight forward? Or if it's mostly a customization used by us, would we want to expose that and make sure the customizations are okay?

[thozza]

• I'm not sure if we want to simply override the default RHSM configuration from the image definition if the RHSM configuration is provided also as a BP customization, or merge them with the BP customization having precedence. Overriding would mean that values set in the default image configuration, but not set in the BP would disappear from the produced manifest. However, overriding would make the implementation much simpler, because the merging requires quite a lot of additional code, which I don't like.

Yeah, I'm not sure either. There's a lot of code in there to accommodate this and I'm wondering if it's worth it. I think this blueprint customization is mostly for us (I don't expect users to be fiddling with RHSM configs) and the brew build configs will be pretty stable, not changing much if at all. Given all that, it's probably not worth having a bunch of code to support conveniently changing a small subset of the configuration. If this was a more user-facing customization, I'd argue for making it easier to tweak individual options without completely changing the config, but if I'm right that it's not, maybe simplifying the code (and the behaviour) is better in the long run.

I literally felt bad while writing all that code. 😅 I also don't like the end result and agree that it is too much code for a little benefit.

While I agree that the customization is mostly for us, it will get exposed to end users because Cloud API uses the blueprint implementation from osbuild-composer. And it is also used by Weldr API, thus exposed to users. I could add the customization to ImageOptions, so that it could be exposed only via Cloud API, but it feels wrong.

I can think of only one relatively good reason to keep the merging behavior. The default image RHSM config may contain configuration for subscription-manager itself and for its DNF plugins. Imagine that one provides only subscription-manager config in the BP, which would then wipe out DNF plugins config if these were part of the default configuration. This felt weird. But we can clearly document it.

There's maybe a middle-ground to do the merging only one level higher. This means that if ones provides DNF plugins config, we override only that part and don't touch the sub-man part. If one provides sub-man config, we override only that part and don't touch DNF plugins config. But this may still mean quite a lot of code to handle the logic...

IOW, I'm happy to simplify this in any way, just want to find out what majority of people think is the best option. 😇

[thozza]

Just to add to the open question with additional questions. If we go the route of overriding the configs this means that the YumPlugins config will always get removed. Would we want to at least check for that and merge it, since that would be pretty straight forward? Or if it's mostly a customization used by us, would we want to expose that and make sure the customizations are okay?

Fair question.

So, the reason why YumPlugins are kind of neglected by the implementation is that they are used only for RHEL-7 image definitions. And we do not expose or support building RHEL-7 anywhere externally, only internally in Brew. My plan is to unify the RHUI / non-RHUI image definitions only for RHEL-9+ once this lands, so I kind of ignored YUM 😇

So in this regard, a complete override or the middle-ground that I outlined in my previous comment would probably be cleaner.

[mvo5]

Thank you! Inline are two quick drive-by ideas

[achilleas-k]

While I agree that the customization is mostly for us, it will get exposed to end users because Cloud API uses the blueprint implementation from osbuild-composer. And it is also used by Weldr API, thus exposed to users.

Right, of course. I just meant that users probably wont be using it much, but I could be wrong. I don't imagine many users would want to change the configuration for subman. Or would they?

[thozza]

While I agree that the customization is mostly for us, it will get exposed to end users because Cloud API uses the blueprint implementation from osbuild-composer. And it is also used by Weldr API, thus exposed to users.

Right, of course. I just meant that users probably wont be using it much, but I could be wrong. I don't imagine many users would want to change the configuration for subman. Or would they?

Right. Hopefully none of the end users will use it 🤞 They shouldn't need to.

[schuellerf]

ultimately this will imply an additional section in https://github.com/osbuild/osbuild.github.io/blob/main/docs/user-guide/01-blueprint-reference.md
did I get this right?
probably stating that it's for on-prem only (until it's handed up to the API?)

[achilleas-k]

ultimately this will imply an additional section in https://github.com/osbuild/osbuild.github.io/blob/main/docs/user-guide/01-blueprint-reference.md did I get this right? probably stating that it's for on-prem only (until it's handed up to the API?)

Needs to land in osbuild-composer first but yes.

[thozza]

I implemented all of the suggestions, except for the override behavior. The reason is that removal of pointers from the internal representation of RHSMConfig simplified the code enough that it may be acceptable. Nevertheless, I'm happy to change it.

To summarize the options for BP RHSM customization behavior when interacting with the image default configuration:

1. Merge the image default config with BP customization with the BP customization having a precedence. Merging is done on the lower level of struct properties. (the current implementation)
2. Override the image default config with BP customization. Potentially throwing away configuration values specified in the image definition, even if not set in the BP customization.
3. Do merging as in the point (1,), but on a higher level configuration blocks (e.g. "DNF Plugins" and "SubMan configuration").

Summary:

• Moved subscription package to pkg/customizations.
• Simplified the internal representation of RHSMConfig to use pointers only for primitive types, which simplified the implementation quite a lot.
• Implemented suggestions from @mvo5 to simplify a few conditions and reduce nesting.

[edit] I force pushed once more, because I additionally simplified RHSMConfigFromBP() and handling of BP customization in osCustomizations().

[achilleas-k]

Great stuff.
LGTM

[kingsleyzissou]

LGTM

[achilleas-k]

@mvo5 Can you take a look at your comments and resolve them (if you're happy :) )?

[mvo5]

Thank you!