Update ClusterVersion to have a 'force' update flag and track verified #293
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
do-not-merge/hold
smarterclayton
force-pushed
the
verified
branch
from
9b1c367 to
40a81a8
Compare
Administrators will need a knob to override upgrades where the payload is not signed correctly, and future code will guard against more scenarios. Add a verified tag to history to convey whether we checked the update. This will be set on upgrades if the payload was verified before update.
smarterclayton
force-pushed
the
verified
branch
from
40a81a8 to
ab4ff93
Compare
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
smarterclayton
removed
the
do-not-merge/hold
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 16, 2020
The outgoing text is unchanged since the property landed ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). Those docs fit on the 'oc' client side until openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), but never applied to the cluster-version operator (CVO) side or the ClusterVersion type. The CVO uses Force to bypass verification failures [1] (invalid pullspec [2], lack of trusted signature [3], etc.) or preconditions [4] (ClusterVersion had Upgradeable=False for a requested minor bump [5]). [1]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/updatepayload.go#L91-L102 [2]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L138 [3]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L182 [4]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/sync_worker.go#L527-L529 [5]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/payload/precondition/clusterversion/upgradeable.go#L74-L79
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 16, 2020
The outgoing text is unchanged since the property landed ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). Those docs fit on the 'oc' client side until openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), but never applied to the cluster-version operator (CVO) side or the ClusterVersion type. The CVO uses Force to bypass verification failures [1] (invalid pullspec [2], lack of trusted signature [3], etc.) or preconditions [4] (ClusterVersion had Upgradeable=False for a requested minor bump [5]). availableUpdates is orthogonal. I don't know what "other forms of consistency checking" was about, so I've dropped that too. If folks want to call out explicit categories that cannot be overridden with Force, we should do that with less ambiguous wording. [1]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/updatepayload.go#L91-L102 [2]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L138 [3]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L182 [4]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/sync_worker.go#L527-L529 [5]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/payload/precondition/clusterversion/upgradeable.go#L74-L79
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 16, 2020
The outgoing text is unchanged since the property landed ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). Those docs fit on the 'oc' client side until openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), but never applied to the cluster-version operator (CVO) side or the ClusterVersion type. The CVO uses Force to bypass verification failures [1] (invalid pullspec [2], lack of trusted signature [3], etc.) or preconditions [4] (ClusterVersion had Upgradeable=False for a requested minor bump [5]). availableUpdates is orthogonal. I don't know what "other forms of consistency checking" was about, so I've dropped that too. If folks want to call out explicit categories that cannot be overridden with Force, we should do that with less ambiguous wording. Autogenerated bumps via: $ hack/update-swagger-docs.sh $ make update-codegen-crds [1]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/updatepayload.go#L91-L102 [2]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L138 [3]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/verify/verify.go#L182 [4]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/cvo/sync_worker.go#L527-L529 [5]: https://github.com/openshift/cluster-version-operator/blob/28e4400eeb9ded7e09ff684e75780599cb25ec2c/pkg/payload/precondition/clusterversion/upgradeable.go#L74-L79
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Apr 7, 2021
These docs have not been adjusted since the properties landed in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (sync: Report whether updates are verified and allow admin override, 2019-04-20, openshift/cluster-version-operator#170). At that time, RetrievePayload would always attempt to verify the proposed release image, including both signature validation and "does this look like a real release image?" checks. If that verification succeeded, it would set 'verified: true'. If that verification failed, it would look at 'force', and if 'force' was true, it would set 'verified: false' and proceed with the update, while if 'force' was false, it would reject the update. Despite the API docs, availableUpdates never came into this in the CVO code. The only availableUpdates guards are client-side in 'oc', and the API docs are discussion the ClusterVersion API, not 'oc adm upgrade's --force API. In openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), oc's client-side guards split --allow-explicit-upgrade (for updating to something not in availableUpdates) and --allow-upgrade-with-warnings (for updating despite client-side guards against Progressing and Degraded ClusterVersion conditions) away from --force, but again, never part of the ClusterVersion force property's CVO-side handling. Since then, the CVO grew precondition handlers in openshift/cluster-version-operator@aceb5bc66f (pkg: add precondition handlers and perform these checks before accepting Update, 2019-08-26, openshift/cluster-version-operator#243). syncOnce ran all the precondition checks, which at that point was just the Upgradeable ClusterOperator and ClusterVersion condition check. If that check failed, but 'force' was true, it would proceed with the update, while if 'force' was false, it would reject the update. In neither case would 'verified' be altered, it continued to only track the signature and "does this look like a real release image?" checks. The idea at the time was that these would fall under the "normal protections" phrasing (overriden by force) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Apr 7, 2021
These docs have not been adjusted since the properties landed in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (sync: Report whether updates are verified and allow admin override, 2019-04-20, openshift/cluster-version-operator#170). At that time, RetrievePayload would always attempt to verify the proposed release image, including both signature validation and "does this look like a real release image?" checks. If that verification succeeded, it would set 'verified: true'. If that verification failed, it would look at 'force', and if 'force' was true, it would set 'verified: false' and proceed with the update, while if 'force' was false, it would reject the update. Despite the API docs, availableUpdates never came into this in the CVO code. The only availableUpdates guards are client-side in 'oc', and the API docs are discussion the ClusterVersion API, not 'oc adm upgrade's --force API. In openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), oc's client-side guards split --allow-explicit-upgrade (for updating to something not in availableUpdates) and --allow-upgrade-with-warnings (for updating despite client-side guards against Progressing and Degraded ClusterVersion conditions) away from --force, but again, never part of the ClusterVersion force property's CVO-side handling. Since then, the CVO grew precondition handlers in openshift/cluster-version-operator@aceb5bc66f (pkg: add precondition handlers and perform these checks before accepting Update, 2019-08-26, openshift/cluster-version-operator#243). syncOnce ran all the precondition checks, which at that point was just the Upgradeable ClusterOperator and ClusterVersion condition check. If that check failed, but 'force' was true, it would proceed with the update, while if 'force' was false, it would reject the update. In neither case would 'verified' be altered; it continued to only track the signature and "does this look like a real release image?" checks. The idea at the time was that these would fall under the "normal protections" phrasing (overriden by force) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 14, 2021
These docs have not been adjusted since the properties landed in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (sync: Report whether updates are verified and allow admin override, 2019-04-20, openshift/cluster-version-operator#170). At that time, RetrievePayload would always attempt to verify the proposed release image, including both signature validation and "does this look like a real release image?" checks. If that verification succeeded, it would set 'verified: true'. If that verification failed, it would look at 'force', and if 'force' was true, it would set 'verified: false' and proceed with the update, while if 'force' was false, it would reject the update. Despite the API docs, availableUpdates never came into this in the CVO code. The only availableUpdates guards are client-side in 'oc', and the API docs are discussion the ClusterVersion API, not 'oc adm upgrade's --force API. In openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), oc's client-side guards split --allow-explicit-upgrade (for updating to something not in availableUpdates) and --allow-upgrade-with-warnings (for updating despite client-side guards against Progressing and Degraded ClusterVersion conditions) away from --force, but again, never part of the ClusterVersion force property's CVO-side handling. Since then, the CVO grew precondition handlers in openshift/cluster-version-operator@aceb5bc66f (pkg: add precondition handlers and perform these checks before accepting Update, 2019-08-26, openshift/cluster-version-operator#243). syncOnce ran all the precondition checks, which at that point was just the Upgradeable ClusterOperator and ClusterVersion condition check. If that check failed, but 'force' was true, it would proceed with the update, while if 'force' was false, it would reject the update. In neither case would 'verified' be altered; it continued to only track the signature and "does this look like a real release image?" checks. The idea at the time was that these would fall under the "normal protections" phrasing (overriden by force) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 14, 2021
These docs have not been adjusted since the properties landed in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (sync: Report whether updates are verified and allow admin override, 2019-04-20, openshift/cluster-version-operator#170). At that time, RetrievePayload would always attempt to verify the proposed release image, including both signature validation and "does this look like a real release image?" checks. If that verification succeeded, it would set 'verified: true'. If that verification failed, it would look at 'force', and if 'force' was true, it would set 'verified: false' and proceed with the update, while if 'force' was false, it would reject the update. Despite the API docs, availableUpdates never came into this in the CVO code. The only availableUpdates guards are client-side in 'oc', and the API docs are discussion the ClusterVersion API, not 'oc adm upgrade's --force API. In openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), oc's client-side guards split --allow-explicit-upgrade (for updating to something not in availableUpdates) and --allow-upgrade-with-warnings (for updating despite client-side guards against Progressing and Degraded ClusterVersion conditions) away from --force, but again, never part of the ClusterVersion force property's CVO-side handling. Since then, the CVO grew precondition handlers in openshift/cluster-version-operator@aceb5bc66f (pkg: add precondition handlers and perform these checks before accepting Update, 2019-08-26, openshift/cluster-version-operator#243). syncOnce ran all the precondition checks, which at that point was just the Upgradeable ClusterOperator and ClusterVersion condition check. If that check failed, but 'force' was true, it would proceed with the update, while if 'force' was false, it would reject the update. In neither case would 'verified' be altered; it continued to only track the signature and "does this look like a real release image?" checks. The idea at the time was that these would fall under the "normal protections" phrasing (overriden by force) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 14, 2021
These docs have not been adjusted since the properties landed in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (sync: Report whether updates are verified and allow admin override, 2019-04-20, openshift/cluster-version-operator#170). At that time, RetrievePayload would always attempt to verify the proposed release image, including both signature validation and "does this look like a real release image?" checks. If that verification succeeded, it would set 'verified: true'. If that verification failed, it would look at 'force', and if 'force' was true, it would set 'verified: false' and proceed with the update, while if 'force' was false, it would reject the update. Despite the API docs, availableUpdates never came into this in the CVO code. The only availableUpdates guards are client-side in 'oc', and the API docs are discussion the ClusterVersion API, not 'oc adm upgrade's --force API. In openshift/oc@0501d04ff1 (upgrade: Separate flags for safety instead of abusing force, 2019-09-27, openshift/oc#109), oc's client-side guards split --allow-explicit-upgrade (for updating to something not in availableUpdates) and --allow-upgrade-with-warnings (for updating despite client-side guards against Progressing and Degraded ClusterVersion conditions) away from --force, but again, never part of the ClusterVersion force property's CVO-side handling. Since then, the CVO grew precondition handlers in openshift/cluster-version-operator@aceb5bc66f (pkg: add precondition handlers and perform these checks before accepting Update, 2019-08-26, openshift/cluster-version-operator#243). syncOnce ran all the precondition checks, which at that point was just the Upgradeable ClusterOperator and ClusterVersion condition check. If that check failed, but 'force' was true, it would proceed with the update, while if 'force' was false, it would reject the update. In neither case would 'verified' be altered; it continued to only track the signature and "does this look like a real release image?" checks. The idea at the time was that these would fall under the "normal protections" phrasing (overriden by force) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Aug 17, 2021
This type used to be used for both the admin-provided spec.desiredUpdate and the operator-provided status.desired. That changed in 575f8d2 (config/v1: New Release type for ClusterVersionStatus, 2019-01-19, openshift#521, 4.6), when status.desired moved to a new Release type, so now Update is only used for spec.desiredUpdate. The properties have been '+optional' since they landed in 898d7e3 (api: Move ClusterVersion/ClusterOperator into config.openshift.io, 2018-11-09, openshift#127) and ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). I'm adding 'omitempty', because if the admin doesn't have a particular version or image in mind, returning explicit empty strings is distracting noise. With this commit, it will be easier to focus on the version or image property that did get set, and you need at least one of them to be set to be a usable update request. The force property is a bit more wiggly, since there may be some benefit to explicitly pointing out that the admin is not forcing the update. But forcing is supposed to be an exceptional-situation safety valve, so I'm adding omitempty there too, because the benefit of de-emphasizing the property's presence (and reducing the chance that an admin says "hey, let's see what 'force: true' does" without reading associated docs) outweighs the benefit of explicitly pointing out 'force: false' cases.
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Nov 3, 2025
We've been dragging this around since growing the 'force' property in ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Administrators will need a knob to override upgrades where the payload
is not signed correctly, and future code will guard against more scenarios.
Add a verified tag to history to convey whether we checked the update.
This will be set on upgrades if the payload was verified before update.
/hold
Waiting on openshift/cluster-version-operator#170. Requried for GA