-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use recert for proxy seed reconfiguration #410
Conversation
Skipping CI for Draft Pull Request. |
d51fb0c
to
9f3b439
Compare
if !ok { | ||
return fmt.Errorf( | ||
"seed image %s is missing the %s label, please build a new image using the latest version of the lca-cli", | ||
seedImageRef, common.SeedFormatOCILabel) | ||
"seed image is missing the %s label, please build a new image using the latest version of the lca-cli", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily "latest version", but seed was generated from "incompatible version of LCA" maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but unrelated to PR. Created #423
} | ||
|
||
clusterInfoJSON := string(clusterInfoJSONSBytes) | ||
if err := s.createAndPushSeedImage(clusterInfoJSON); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want all this in a label? Why not just validate the data from the manifest.json after pulling the image? I get that it would save the pull, but is there some other benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I do like the idea of this being an easy way to access the manifest.json data without pulling the image, for expanded image validation. Are there any cons to it? Like would we end up with restrictions on what could get added to the manifest.json? I think no, but I'm not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly, it's nice to have. Couldn't think of any cons. It's information that's already in the seed, just now duplicated to different location as well (previously just a file inside a layer, now also in the manifest), so I don't see any additional compatibility restrictions this would add.
Like would we end up with restrictions on what could get added to the manifest.json?
Perhaps in terms of size. I guess at that point in the future where we need something huge in there, it would be an obvious problem and we could redact that huge thing we put in manifest.json
before we serialize it to put inside the label
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this looks great to me, thanks for adding it
7baeb49
to
fc48508
Compare
/lgtm |
/test ibu-e2e-flow |
/test ibu-e2e-flow |
1 similar comment
/test ibu-e2e-flow |
/test ibu-e2e-flow |
/test ibu-e2e-flow |
/remove-label cluster-config-api-changed |
# Overview LCA will now use recert's new proxy feature. This will allow us to support environments where the seed proxy is different than the upgraded cluster's proxy. It will also allow us to perform a rollout-free seed proxy reconfiguration. Clusters with a proxy can only be upgraded using seeds that have a proxy. Clusters without a proxy can only be upgraded using seeds that don't have a proxy. # Notes Two new fields have been added to `SeedReconfiguration`, `Proxy` and `StatusProxy` - see commit diff for an explanation of why we need both and how they're determined in every scenario. Added a new `HasProxy` field to the seed cluster info struct. Seed images will now contain a `com.openshift.lifecycle-agent.seed_cluster_info` OCI label which will contain a JSON serialized seed cluster info struct. This label will allow LCA (and maybe IBIO in the future?) to make informed decisions about the compatibility of a particular seed image with the cluster being upgraded. For example, LCA will refuse to upgrade a cluster that has a proxy with a seed that doesn't. This label helps LCA knows whether the seed has a proxy or not (through the new `HasProxy` field), and block the upgrade appropriately before the image even gets pulled. # install-config On top of proxy, we're also going to be using recert's new install-config option. See comment about the new InstallConfig field in the seed reconfiguration struct for more information about why.
/remove-label cluster-config-api-changed |
/test ibu-e2e-flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Overview
LCA will now use recert's new proxy feature. This will allow us to
support environments where the seed proxy is different than the upgraded
cluster's proxy. It will also allow us to perform a rollout-free seed
proxy reconfiguration.
Clusters with a proxy can only be upgraded using seeds that have a
proxy. Clusters without a proxy can only be upgraded using seeds that
don't have a proxy.
Notes
Two new fields have been added to
SeedReconfiguration
,Proxy
andStatusProxy
- see commit diff for an explanation of why we need bothand how they're determined in every scenario.
Added a new
HasProxy
field to the seed cluster info struct.Seed images will now contain a
com.openshift.lifecycle-agent.seed_cluster_info
OCI label which will contain a JSON serialized seed cluster info struct.
This label will allow LCA (and maybe IBIO in the future?) to make
informed decisions about the compatibility of a particular seed image
with the cluster being upgraded. For example, LCA will refuse to upgrade
a cluster that has a proxy with a seed that doesn't. This label helps
LCA knows whether the seed has a proxy or not (through the new
HasProxy
field), and block the upgrade appropriately before the imageeven gets pulled.