Operator upgrades and approvedimage pcr storage

[SpaceFace02]

This PR adds operator upgrades and removing image-pcr cm in favour of storing pcr events and values in approvedimagestatus.

1. the operator's reconcile loop will attempt to converge all dependant images on an upgrade, either manually applied or via OLM.
2. Various helpers have been added to make the code more readable and concise.
3. It also includes integration tests for the same, testing a real version upgrade, from a old tag on quay, to your locally built artifacts and binaries.
   5.Reference value configmap is removed as we have moved to trustee version 0.20.0. These reference values are managed by trustee and are ephemeral for the duration of the deployment.
4. It also includes some Quality of Life improvements for attestation keys to make them a bit less prone to race cond
   itions.
5. AttestationKeys finalizers return immediately if trustee is also being deleted (via TEC deletion) and upgrades from older versions fail unless labels are attached to the secrets.
6. It also adds a few tests for testing combination pcrs, and whether events are stored and hex hashes can be reconst
   ructed.
7. rRemoval of the image-pcr configmap as discussed, and instead proceeds storing pcr values and events in Ap
   provedImage status.

[sourcery-ai]

Sorry @SpaceFace02, your pull request is larger than the review limit of 150,000 diff characters

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SpaceFace02

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SpaceFace02]

Please let me know if I need to split this PR somehow, I couldn't figure out how to split it further.

[yairpod]

This PR is big and from the commit messages looks to be doing several distinct things.

How is the image-pcr configmap removal related to the operator upgrade? looks to me like it can be an independent PR.

The commit named "feat: operator upgrades, changes to trustee rvmap and rv config, attestationkey improvements" looks like it includes many different change that might not be at all related? the commit message lists the changes:

1. adds operator upgrades - the core of this PR
2. It also includes integration tests for the same - vary good, might be nice to be in a separate commit in the same PR, but not necessary.
3. Various helpers have been added to make the code more readable - are these helpers related to the operator upgrades ? otherwise they should probably be in a different PR.
4. "Reference value configmap is removed as we have moved to trustee version 0.20.0." - this sounds like a separate PR, even if the Upgrade is dependent upon it.
5. "Quality of Life improvements" to reduce race conditions - also sounds like a separate PR.
6. "AttestationKeys finalizers return immediately if trustee is also being deleted (via TEC deletion).
   And upgrades from older versions fail unless labels are attached to the secrets." - are these the "Quality of Life improvements" from the last line? or separate fixes?

[SpaceFace02]

How is the image-pcr configmap removal related to the operator upgrade? looks to me like it can be an independent PR.

They are not directly related, but since I am upgrading from 0.2.2 (with image-pcrs) to the current version (without image-pcrs), it does tie in together with upgrades. I have tried to split it in 2 commits instead of 2 PRs.

Various helpers have been added to make the code more readable

yes, all helpers are related to operator upgrades.

"Reference value configmap is removed as we have moved to trustee version 0.20.0." - this sounds like a separate PR, even if the Upgrade is dependent upon it.
"Quality of Life improvements" to reduce race conditions - also sounds like a separate PR.

Kind of hard to split it in multiple PRs because all these changes are so coupled, although can do if needed

"AttestationKeys finalizers return immediately if trustee is also being deleted (via TEC deletion).
And upgrades from older versions fail unless labels are attached to the secrets." - are these the "Quality of Life improvements" from the last line? or separate fixes?

This is the Quality of Life improvement for attestation key, which ties in with upgrades.

TLDR: It does make sense to split this PR considering even Sourcery's limit has been reached and to make it easier for review.

[SpaceFace02]

I plan to split it like this:

1. Operator upgrades PR (includes attestation key improvements), including integration tests (maybe seperate commit)
2. RV map changes.
3. Storing pcr values and events in ApprovedImage status

[yairpod]

I plan to split it like this:

1. Operator upgrades PR (includes attestation key improvements), including integration tests (maybe seperate commit)

What are the "attestation key improvements"? do they make sense without the context of the upgrades? sounds like they belong in their own PR that the Upgrades PR will be dependent upon

2. RV map changes.

3. Storing pcr values and events in ApprovedImage status

are the pcr values and events in ApprovedImage related to each other of do they have individual value and viability?

[SpaceFace02]

What are the "attestation key improvements"? do they make sense without the context of the upgrades? sounds like they belong in their own PR that the Upgrades PR will be dependent upon

They are tied in to upgrades, because in version 0.2.2, we did not have a label on secrets (refer this, and while upgrading we need to make sure the label is present. Otherwise this change is not needed, if we are upgrading from version 0.2.3 onwards.

are the pcr values and events in ApprovedImage related to each other of do they have individual value and viability?

Yes, this PR removed image-pcrs configmap in favour of storing pcr values and events in ApprovedImage status itself, to prevent the race condtion of multiple jobs updating the image-pcrs cm. Not sure I understand the second part of your question

[yairpod]

What are the "attestation key improvements"? do they make sense without the context of the upgrades? sounds like they belong in their own PR that the Upgrades PR will be dependent upon

They are tied in to upgrades, because in version 0.2.2, we did not have a label on secrets (refer this, and while upgrading we need to make sure the label is present. Otherwise this change is not needed, if we are upgrading from version 0.2.3 onwards.

are the pcr values and events in ApprovedImage related to each other of do they have individual value and viability?

Yes, this PR removed image-pcrs configmap in favour of storing pcr values and events in ApprovedImage status itself, to prevent the race condtion of multiple jobs updating the image-pcrs cm. Not sure I understand the second part of your question

Then it sounds like he 3 PRs you suggested are the right breakdown, if the Upgrades PR is still too big we could split it and have the label be in a seperate PR, but I hope a separate commit will be enough.

[SpaceFace02]

Then it sounds like he 3 PRs you suggested are the right breakdown, if the Upgrades PR is still too big we could split it and have the label be in a seperate PR, but I hope a separate commit will be enough.

Sounds good,

I had a general question to everyone: Do we need each commit to pass the GHA pipeline? Because then the effort would increase to make sure each commit passes all checks, instead of just the PR passing all checks.

edit: nvm, looks like the pipeline runs on only the latest commit in the PR

[Jakob-Naucke]

I had a general question to everyone: Do we need each commit to pass the GHA pipeline? Because then the effort would increase to make sure each commit passes all checks, instead of just the PR passing all checks.

this isn't enforced and I don't think GHA really supports it, but it is preferred so that bisects work

[openshift-ci]

@SpaceFace02: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/azure-integration-test	faaffd2	link	false	/test azure-integration-test

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.