config/v1/types_cluster_version: Update force and verified godocs #885
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
wking
force-pushed
the
clarify-force-and-verified-docs
branch
from
8598057 to
c49454a
Compare
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
Contributor
|
/approve @wking can you fix the verification error? |
bparees
removed
the
lifecycle/stale
openshift-ci
bot
added
the
approved
wking
force-pushed
the
clarify-force-and-verified-docs
branch
from
c49454a to
b03d3f2
Compare
openshift-ci
bot
added
the
needs-rebase
wking
force-pushed
the
clarify-force-and-verified-docs
branch
from
b03d3f2 to
4a38236
Compare
openshift-ci
bot
removed
the
needs-rebase
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
force-pushed
the
clarify-force-and-verified-docs
branch
from
4a38236 to
4d4f989
Compare
Member
Author
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, wking 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 |
bparees
reviewed
Sep 11, 2021
| properties: | ||
| force: | ||
| description: "force allows an administrator to update to an image that has failed verification, does not appear in the availableUpdates list, or otherwise would be blocked by normal protections on update. This option should only be used when the authenticity of the provided image has been verified out of band because the provided image will run with full administrative access to the cluster. Do not use this flag with images that comes from unknown or potentially malicious sources. \n This flag does not override other forms of consistency checking that are required before a new update is deployed." | ||
| description: force allows an administrator to update to an image that has failed verification or upgradeable checks. This option should only be used when the authenticity of the provided image has been verified out of band because the provided image will run with full administrative access to the cluster. Do not use this flag with images that comes from unknown or potentially malicious sources. |
Contributor
There was a problem hiding this comment.
@wking "upgradeable checks" is still somewhat unclear to someone not steeped in our internal meanings.
"there is no edge from your cluster version to this target version" is an "upgradeable check"(at least to a naive user) but not one that force has any influence on, right?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Addressing some of the ambiguities raised in rhbz#1825396.
These docs have not been adjusted since the properties landed in ab4ff93 (#293). The initial cluster-version operator implementation only used them for signature checks, openshift/cluster-version-operator@f8eff25191 (openshift/cluster-version-operator#170). At that time,
RetrievePayloadwould 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 setverified: true. If that verification failed, it would look atforce, and ifforcewas true, it would setverified: falseand proceed with the update, while ifforcewas false, it would reject the update.Despite the API docs,
availableUpdatesnever came into this in the CVO code. The onlyavailableUpdatesguards are client-side inoc, and the API docs are discussion the ClusterVersion API, notoc adm upgrade's--forceAPI. In openshift/oc@0501d04ff1 (openshift/oc#109),oc's client-side guards split--allow-explicit-upgrade(for updating to something not inavailableUpdates) and--allow-upgrade-with-warnings(for updating despite client-side guards againstProgressingandDegradedClusterVersion 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 (openshift/cluster-version-operator#243).
syncOnceran all the precondition checks, which at that point was just theUpgradeableClusterOperator and ClusterVersion condition check. If that check failed, butforcewas true, it would proceed with the update, while ifforcewas false, it would reject the update. In neither case wouldverifiedbe 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 byforce) and not the "other forms of consistency checking" phrasing (whatever those were supposed to be).CC @smarterclayton