Skip to content

[BZ1810461] ShiftStack - Add procedure to allow Image Registry Operator to trust Swift#39641

Merged
maxwelldb merged 1 commit intoopenshift:mainfrom
maxwelldb:ocponosp-swift-self-cert-bz1810461
Mar 4, 2022
Merged

[BZ1810461] ShiftStack - Add procedure to allow Image Registry Operator to trust Swift#39641
maxwelldb merged 1 commit intoopenshift:mainfrom
maxwelldb:ocponosp-swift-self-cert-bz1810461

Conversation

@maxwelldb
Copy link
Contributor

@maxwelldb maxwelldb commented Dec 8, 2021

@maxwelldb maxwelldb added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2021
@maxwelldb maxwelldb self-assigned this Dec 8, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2021
@netlify
Copy link

netlify bot commented Dec 8, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: fbc46fe

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/622250d04799be00087ca33b

😎 Browse the preview: https://deploy-preview-39641--osdocs.netlify.app

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 8, 2021
@maxwelldb maxwelldb added this to the Future Release milestone Dec 8, 2021
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a necessary step?

Copy link
Contributor

Choose a reason for hiding this comment

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

disableRedirect is in config.imageregistry, not image.config.

Copy link
Member

Choose a reason for hiding this comment

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

According to https://bugzilla.redhat.com/show_bug.cgi?id=1810461#c13, the following should be all that is needed:

$ oc patch configs.imageregistry.operator.openshift.io cluster --type merge --patch '{"spec":{"disableRedirect":"true"}}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mandre As in that command should be almost the entirety of this PR? Easy enough, if so.

Any thoughts on the second part of this comment? https://bugzilla.redhat.com/show_bug.cgi?id=1810461#c14

Copy link
Member

Choose a reason for hiding this comment

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

@mandre As in that command should be almost the entirety of this PR? Easy enough, if so.

Any thoughts on the second part of this comment? https://bugzilla.redhat.com/show_bug.cgi?id=1810461#c14

If I understand comment 14 correctly what we're doing here (disable redirect) is what the customer has confirmed fixes the issue. I do not think we want to document the other suggested solution as it's a lot more complex and forces the nodes to trust the CA ultimately (while we trust it only when initiating connections to OpenStack otherwise).

Copy link
Contributor Author

@maxwelldb maxwelldb left a comment

Choose a reason for hiding this comment

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

Made a few suggestions for syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mandre Could you confirm this command given this comment? https://github.com/openshift/openshift-docs/pull/39641/files#r765059344

Copy link
Member

Choose a reason for hiding this comment

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

This is redundant with what you're doing on line 23.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TY!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mandre So, just this? 3c855ce

Copy link
Member

Choose a reason for hiding this comment

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

Yup, perfect 👍

@maxwelldb maxwelldb requested a review from mandre February 21, 2022 20:06
@maxwelldb maxwelldb force-pushed the ocponosp-swift-self-cert-bz1810461 branch 2 times, most recently from 577a4df to 450b525 Compare February 22, 2022 20:10
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

LGTM

@maxwelldb maxwelldb requested a review from xiuwang February 25, 2022 13:23
@maxwelldb maxwelldb added the peer-review-needed Signifies that the peer review team needs to review this PR label Feb 28, 2022
@xiuwang
Copy link

xiuwang commented Mar 1, 2022

LGTM

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

This is looking good!

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this procedure have prereqs? It looks like there's nothing to change in the command itself, so I suspect there might be. You might want to add them at some point.

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 4, 2022
@maxwelldb maxwelldb force-pushed the ocponosp-swift-self-cert-bz1810461 branch from 7a887e3 to fbc46fe Compare March 4, 2022 17:47
@maxwelldb maxwelldb merged commit 8d76074 into openshift:main Mar 4, 2022
@maxwelldb
Copy link
Contributor Author

/cherry-pick enterprise-4.10

@openshift-cherrypick-robot

@maxwelldb: new pull request created: #42840

Details

In response to this:

/cherry-pick enterprise-4.10

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.

@maxwelldb
Copy link
Contributor Author

@xiuwang Can you confirm that this procedure is valid for 4.6-4.10? This PR has a 4.10 label on it, though I'm not sure if I made a mistake at some point.

@maxwelldb
Copy link
Contributor Author

/cherry-pick enterprise-4.10

@openshift-cherrypick-robot

@maxwelldb: new pull request could not be created: failed to create pull request against openshift/openshift-docs#enterprise-4.10 from head openshift-cherrypick-robot:cherry-pick-39641-to-enterprise-4.10: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for openshift-cherrypick-robot:cherry-pick-39641-to-enterprise-4.10."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

Details

In response to this:

/cherry-pick enterprise-4.10

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.

@bobfuru bobfuru modified the milestones: Future Release, OCP 4.10 GA Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.10 peer-review-done Signifies that the peer review team has reviewed this PR 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