Skip to content

Conversation

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Sep 27, 2019

The --force flag is dangerous and potentially allows untrusted content to be upgraded to accidentally. Instead, introduce two new flags --allow-explicit-upgrade (for upgrading to something not in availableVersions) and --allow-upgrade-with-warnings (for upgrading when another upgrade is in progress or the cluster is reporting an error) and remove those checks from --force.

While this is an API change, it is necessary to ensure that users do not accidentally get access to untrusted content when performing upgrades across major versions in advance of graph updates, or when they are upgrading in disconnected environments.

/assign @jwforres
@eparis as discussed today

This should be a candidate for backport to 4.2 and 4.1. While it changes flag behavior, it will only break things in a "safe" way. This addresses a significant security challenge as we have introduced new meanings for --force.

# install 4.1.15

$ oc adm upgrade --to-image quay.io/openshift-release-dev/ocp-release:4.1.16
error: The requested upgrade image is not one of the available updates, you must pass --allow-explicit-upgrade to continue
$ oc adm upgrade --to-image quay.io/openshift-release-dev/ocp-release:4.1.16 --allow-explicit-upgrade

... // wait

$ oc adm upgrade --to-image quay.io/openshift-release-dev/ocp-release:4.1.7
error: The requested upgrade image is not one of the available updates, you must pass --allow-explicit-upgrade to continue
$ oc adm upgrade --to-image quay.io/openshift-release-dev/ocp-release:4.1.7 --allow-explicit-upgrade
error: Already upgrading, pass --allow-upgrade-with-warnings to override.

  Reason:
  Message: Working towards 4.1.16: 13% complete

$ oc adm upgrade --to-image quay.io/openshift-release-dev/ocp-release:4.1.7 --allow-explicit-upgrade --allow-upgrade-with-warnings
Updating to release image quay.io/openshift-release-dev/ocp-release:4.1.7

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 27, 2019
@smarterclayton
Copy link
Contributor Author

@abhinavdahiya you and I have discussed this, as part of ensuring 4.1 to 4.2 upgrades go smoothly we want to allow people in 4.1 to upgrade to 4.2 before we publish the edge without using --force to bypass content checks.

@smarterclayton smarterclayton changed the title upgrade: Separate flags for safety instead of abusing force Bug 1756453: Separate flags for safety instead of abusing force Sep 27, 2019
@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 27, 2019
@openshift-ci-robot
Copy link

@smarterclayton: This pull request references Bugzilla bug 1756453, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1756453: Separate flags for safety instead of abusing force

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/test-infra repository.

@smarterclayton smarterclayton changed the title Bug 1756453: Separate flags for safety instead of abusing force Bug 1756453: Separate upgrade flags for safety instead of abusing force Sep 27, 2019
flags.BoolVar(&o.Clear, "clear", o.Clear, "If an upgrade has been requested but not yet downloaded, cancel the update. This has no effect once the update has started.")
flags.BoolVar(&o.Force, "force", o.Force, "Upgrade even if an upgrade is in process or other error is blocking update.")
flags.BoolVar(&o.AllowExplicitUpgrade, "allow-explicit-upgrade", o.AllowExplicitUpgrade, "Upgrade even if the upgrade target is not listed in the available versions list.")
flags.BoolVar(&o.AllowUnsafeUpgrade, "allow-unsafe-upgrade", o.AllowUnsafeUpgrade, "Upgrade even if an upgrade is in process or the cluster is error is blocking update.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed changing this one to allow-upgrade-when-unhealthy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying --allow-upgrade-with-warnings?

@smarterclayton smarterclayton force-pushed the upgrade branch 2 times, most recently from fe67036 to 90be3d0 Compare September 27, 2019 19:10
@abhinavdahiya
Copy link
Contributor

I like this.

LGTM

@openshift-ci-robot
Copy link

@smarterclayton: This pull request references Bugzilla bug 1756453, which is valid.

Details

In response to this:

Bug 1756453: Separate upgrade flags for safety instead of abusing force

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/test-infra repository.

Copy link
Contributor

@crawford crawford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit related to copy. LGTM

flags.BoolVar(&o.Clear, "clear", o.Clear, "If an upgrade has been requested but not yet downloaded, cancel the update. This has no effect once the update has started.")
flags.BoolVar(&o.Force, "force", o.Force, "Upgrade even if an upgrade is in process or other error is blocking update.")
flags.BoolVar(&o.AllowExplicitUpgrade, "allow-explicit-upgrade", o.AllowExplicitUpgrade, "Upgrade even if the upgrade target is not listed in the available versions list.")
flags.BoolVar(&o.AllowUpgradeWithWarnings, "allow-upgrade-with-warnings", o.AllowUpgradeWithWarnings, "Upgrade even if an upgrade is in process or the cluster is error is blocking update.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other potential colors for the shed:

--allow-unsound-upgrade
--allow-impatient-upgrade

@abhinavdahiya
Copy link
Contributor

There also this bug https://bugzilla.redhat.com/show_bug.cgi?id=1713263 that states that --to-latest and --force don't work together correctly? Is that expected @smarterclayton ?

@smarterclayton
Copy link
Contributor Author

That is not expected, are you going to track that separately or want me to throw it in here? I don't have an issue doing that.

@abhinavdahiya
Copy link
Contributor

That is not expected, are you going to track that separately or want me to throw it in here? I don't have an issue doing that.

just wanted to get the expectation esp if it covers the new flags too..., we are already tracking this and we will work to fix it separately.

The --force flag is dangerous and potentially allows untrusted
content to be upgraded to accidentally. Instead, introduce two
new flags `--allow-explicit-upgrade` (for upgrading to something not
in availableVersions) and `--allow-upgrade-with-warnings` (for upgrading
when another upgrade is in progress or the cluster is reporting
an error) and remove those checks from `--force`.

While this is an API change, it is necessary to ensure that users
do not accidentally get access to untrusted content when
performing upgrades across major versions in advance of graph
updates, or when they are upgrading in disconnected environments.
@smarterclayton
Copy link
Contributor Author

/retest

@jwforres
Copy link
Member

jwforres commented Oct 4, 2019

/lgtm
/test e2e-cmd

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwforres, smarterclayton

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Oct 4, 2019 via email

@smarterclayton
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 5bde324 into openshift:master Oct 6, 2019
@openshift-ci-robot
Copy link

@smarterclayton: All pull requests linked via external trackers have merged. Bugzilla bug 1756453 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1756453: Separate upgrade flags for safety instead of abusing force

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/test-infra repository.

wking added a commit to wking/oc that referenced this pull request Apr 21, 2020
The warning is from cd30f2f (Add `oc adm upgrade` to display
available updates or trigger an update, 2018-12-04), but the "does not
check for upgrade compatibility..." portion is stale since 0501d04
(upgrade: Separate flags for safety instead of abusing force,
2019-09-27, openshift#109) added --allow-explicit-upgrade.
wking added a commit to wking/oc that referenced this pull request Apr 21, 2020
The warning is from cd30f2f (Add `oc adm upgrade` to display
available updates or trigger an update, 2018-12-04), but the "does not
check for upgrade compatibility..." portion is stale since 0501d04
(upgrade: Separate flags for safety instead of abusing force,
2019-09-27, openshift#109) added --allow-explicit-upgrade.

Also fixes an unrelated "next available" -> "latest available" for
--to-latest, fixing a typo from the history-breaking 3abf60a (setup
oc repo, 2019-06-17).
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/oc that referenced this pull request Feb 21, 2022
We've had the warning at least since this repo was created in
3abf60a (setup oc repo, 2019-06-17).  But since we grew
--allow-explicit-upgrade in 0501d04 (upgrade: Separate flags for
safety instead of abusing force, 2019-09-27, openshift#109), --to-image has
been just as restricted to recommended updates as --to.  Removing the
warning here avoids uneccessary alarm.  We can save the warning for
--allow-explicit-upgrade, whose help text I'm consolidating in this
commit.  The new structure is:

* --to and --to-image together, since in the absecne of
  --allow-explicit-upgrade those are equally safe.

* The paragraph about unsupported updates or broken CVO <-> upstream
  update service communication.  Before this paragraph had focused on
  --to-image, but I've adjusted it to focus on
  --allow-explicit-upgrade.

* The --allow-upgrade-with-warnings paragraph.  I've moved the
  sentence about retargeting updates over to this paragraph, since
  --allow-upgrade-with-warnings is the relevant flag for that issue.

* The --force paragraph, because that's about a cluster-side check
  that occurs after the oc-side --allow-* checks, so having it last in
  the help text gives us the same order in help as we get for check
  application.
wking added a commit to wking/oc that referenced this pull request Feb 21, 2022
We've had the warning at least since this repo was created in
3abf60a (setup oc repo, 2019-06-17).  But since we grew
--allow-explicit-upgrade in 0501d04 (upgrade: Separate flags for
safety instead of abusing force, 2019-09-27, openshift#109), --to-image has
been just as restricted to recommended updates as --to.  Removing the
warning here avoids unnecessary alarm.  We can save the warning for
--allow-explicit-upgrade, whose help text I'm consolidating in this
commit.  The new structure is:

* --to and --to-image together, since in the absence of
  --allow-explicit-upgrade those are equally safe.

* The paragraph about unsupported updates or broken CVO <-> upstream
  update service communication.  Before this paragraph had focused on
  --to-image, but I've adjusted it to focus on
  --allow-explicit-upgrade.

* The --allow-upgrade-with-warnings paragraph.  I've moved the
  sentence about retargeting updates over to this paragraph, since
  --allow-upgrade-with-warnings is the relevant flag for that issue.

* The --force paragraph, because that's about a cluster-side check
  that occurs after the oc-side --allow-* checks, so having it last in
  the help text gives us the same order in help as we get for check
  application.
wking added a commit to wking/oc that referenced this pull request Mar 14, 2022
We've had the warning at least since this repo was created in
3abf60a (setup oc repo, 2019-06-17).  But since we grew
--allow-explicit-upgrade in 0501d04 (upgrade: Separate flags for
safety instead of abusing force, 2019-09-27, openshift#109), --to-image has
been just as restricted to recommended updates as --to.  Removing the
warning here avoids unnecessary alarm.  We can save the warning for
--allow-explicit-upgrade, whose help text I'm consolidating in this
commit.  The new structure is:

* --to and --to-image together, since in the absence of
  --allow-explicit-upgrade those are equally safe.

* The paragraph about unsupported updates or broken CVO <-> upstream
  update service communication.  Before this paragraph had focused on
  --to-image, but I've adjusted it to focus on
  --allow-explicit-upgrade.

* The --allow-upgrade-with-warnings paragraph.  I've moved the
  sentence about retargeting updates over to this paragraph, since
  --allow-upgrade-with-warnings is the relevant flag for that issue.

* The --force paragraph, because that's about a cluster-side check
  that occurs after the oc-side --allow-* checks, so having it last in
  the help text gives us the same order in help as we get for check
  application.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants