AAP: import/export (HMS-8836) (HMS-8837)

[ksiekl]

AAP was in image_request, but since it is not an extra registration and more a firstboot, I moved it to customizations. Also edited tests for import and export.

Just one question - do I understand correctly that when exporting a bp, we want to redact certificates (I also removed config key), but when importing, we expect the user to have the certificates in the request? In my head it made sense, so hopefully that's ok :)

[kingsleyzissou]

From a changes point of view it looks good, I don't have a huge amount of domain knowledge here though, so I'll wait for @lzap to review also

[lzap]

Oh no this again. I am not the best one to ask if something belongs to blueprint (aka customizations) or not, I feel I was going in circles when we were discussing some similar fields. So I am going to drag @achilleas-k into the conversation. Let me CC @lucasgarfield and make a quick summary first:

• We added "firstboot" script in a way that it is completely constructed via custom files in the UI. This was our (team envision) first bigger contribution, now we know that was not the best decision.
• Then we added Satellite registration following the same design pattern - the firstboot for satellite is also constructed in the UI.
• We have realized that firstboot and satellite should have been a proper customization in blueprint processed by images. This was not yet fixed and I do not believe there is a Jira for this.
• For AAP registration, we have decided to process it in the CRC backend, again, it creates file customizations like the UI.
• One customization was properly added as a tho, it was the "cacert" which turned out to be useful to stand on its legs from the day one - I added this and it is being used in both registrations.

So we ended up with a mixture of three approaches for the three little features for CRC:

• Firstboot and Satellite handled in the UI.
• AAP handled in the CRC.
• CA cert as the only "sub-customization" that is properly handled by images library.

Now we have this secondary problem and the reason of this PR is where the registration fields do belong - is that a customization? Or is that a image request thing? I think we need this to be settled for once with a plan going forward. I am not happy about the situation I initiated and then left building UBP. Tho I have to say the clunky state of the current BP might play a role in how it ended now :-)

Since I am finishing off with the initial work of UBP and work will likely shift more onto other folks for the integration, I would like to step up and take this effort under my umbrella and lead it towards the ideal state which would be proper fisrtboot and satellite/aap registration customizations in the BP/UBP.

Shall we meet, discuss and plan this? Cheers.

[lzap]

@ksiekl you brought very good points, but I would like to stop for a moment, discuss how to approach this and plan accordingly. The AAP work is part of a bigger technical gap that I think we should clear first. But to answer your concerns:

Registration data is part of image request for a reason, we thought customization is not a good place for registration-related things despite there already is a RHSM registration. I am not completely on board with this honestly.

Redacting generated firstboot files is a very good point, yes. Tho I will say if we refactor this into a proper customization handled by images library, there will be no need of this as the files will be created during manifest generation.

[kingsleyzissou]

We have realized that firstboot and satellite should have been a proper customization in blueprint processed by images. This was not yet fixed and I do not believe there is a Jira for this.

Yeah this keeps coming back to bite us. Let me create a jira for it!

Redacting generated firstboot files is a very good point, yes. Tho I will say if we refactor this into a proper customization handled by images library, there will be no need of this as the files will be created during manifest generation.

Also might be an opportunity for @ksiekl to look at other parts of the stack

Edit: A satellite jira already exists (HMS-6309), I cloned it and created HMS-8851

[lzap]

Also might be an opportunity for @ksiekl to look at other parts of the stack

Absolutely, apologies for being a bit protective from composer/images blueprints work. By all means, it does not bite or anything! :-D

Edit: Also, keeping the code in CRC for a bit might not be a bad idea. We might even consider these to features to be just CRC only and never put them down the stack. Firstboot probably is a good candidate for full support anyways.

[ksiekl]

From what you're saying @lzap , it seems like there really are bad inconsistencies in the customizations. I wasn't here for the first two, and jumped into AAP when it was already planned out, so I expected this to be what we want - to handle AAP in crc, and after a talk with @lucasgarfield we agreed AAP is more of a customization. I didn't know the problem ran this deep, but I am of course open for discussion about that with everyone, and yes - let's plan this more!

[lzap]

I just noticed this is a PR with two commits. If you need the import/export patch in, I think this discussion can be separate, in that case just move import/export commit into a separate PR and rename this PR to "AAP: move aap registration from image_request to customization" instead so we can continue the discussion around the refactoring if you want.

[achilleas-k]

My thoughts on the customization topic.

I don't know how urgent this is. I realise doing a customization "properly" takes time. If there is a due date approaching for this feature, it's okay to make it a customization here in a bit of a hacky way and then fill in the gaps later. But even in that case, let's plan the backend feature first and think about what the customization will look like in the on-prem blueprint (both the current one and the new), so we avoid any inconsistencies.

[ksiekl]

@lzap since import and export depends directly on the structure of the bp request, I would leave these commits together if it's ok with you. Creating a separate PR with i/e with aap in image_request now would be useless extra work if we decide to keep the aap in customizations, and I would have to rewrite it again to the same state it is in now.

I'd like to wait for the conversation about the customizations vs. image_request to be resolved and make changes accordingly.

[lzap]

My thoughts on the customization topic.

I was hoping you would comment on the registration in customization vs image request. :-D

I am going to work on firstboot customization full support because that is bothering me the most. I think it is okay to leave AAP as is perhaps put Satellite on par with this solution so at least we have a parity. Once firstboot is done, I think it allows for a nice workaround for AAP/Satellite users who would need to do on-prem registrations - just add few lines here and there and you are good to go.

[lzap]

To summary:

This PR moves AAP registration info from image request to customization. I think this is only valid if we want to continue moving AAP (and Sat) registration to the images blueprint (or UBP) therefore on-prem. Because if we don't, the moment CRC API is upgraded to UBP-compatible V2 API this field will need to go back to somewhere else, likely image request part of the API call.

You need to ask PO if this is what we want to do, I will say that I already started native firstboot support in BL/UBP work and once landed, it will be pretty easy for on-prem users to simply copy-paste their own curl command if they want to.

@croissanne @kingsleyzissou @lucasgarfield

[lzap]

We discussed this after the meeting. Summary:

• Make AAP/Sat a proper images customization.
• Move AAP CRC API registration from image request to customization.
• Discussion about what belongs into customization should be part of the UBP effort.

[lzap]

All good, just do not drop the TLS cert during secret redaction.

[lzap]

This is an API breaking change, will UI be ready when we merge this? It will go straight into stage.

[ksiekl]

UI is done, but will need a review.
It contains playwright test that was waiting for these changes to include import and export. I can tag people for review today, but it will need to be merged after this gets merged.

[achilleas-k]

Is it? It's just additive, isn't it?

[lzap]

It is a refactoring, see the hunk above.

Well, if there are no upgrade concerns, I am fine.

[ksiekl]

Sorry, needed to fix the test after tls certificate change

[lzap]

Leaving the merge on @kingsleyzissou thanks great work.