Skip to content

Load system cert pool for image push to accommodate direct cloud storage access#3067

Merged
openshift-merge-robot merged 1 commit intoopenshift:layeringfrom
jkyros:layering-fix-certificates
Apr 7, 2022
Merged

Load system cert pool for image push to accommodate direct cloud storage access#3067
openshift-merge-robot merged 1 commit intoopenshift:layeringfrom
jkyros:layering-fix-certificates

Conversation

@jkyros
Copy link
Copy Markdown
Member

@jkyros jkyros commented Apr 6, 2022

Something changed in openshift last week to where after about 20 minutes or so of the cluster being up, requests to/for images start getting forwarded directly to the cloud storage endpoint (signed by aws/google/whoever) intead of being proxied by the cluster (endpoint signed by our cluster signer).

This should have been fine, and probably was fine everywhere else, except for our hacky scratch image push, where we weren't loading the SystemCertPool, which meant we couldn't verify those cloud provider certificates.

We only loaded one of our cluster signer certificates because we didn't know we would ever be talking to the clouds directly (and, well, we weren't until other non-MCO changes last week).

This loads the SystemCertPool so we can verify those cloud certificates instead of failing with x509 errors.

There is an issue where after some period of time,(usually around 20
minutes of the cluster being up)  the requests to images start getting
forwarded directly to the cloud storage buckets wherethey reside.

Those endpoints present certificates that are signed by
the cloud provider (e.g. amazon,google,microsoft,etc) and not by our
cluster certificates. We did not previously load the system cert pool,
so those requests would fail.

This makes sure we also load our system cert pool so we can verify those
certificates and don't fail with x509 errors when we attempt to
communicate with the registry.
@mkenigs
Copy link
Copy Markdown
Contributor

mkenigs commented Apr 6, 2022

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2022
@cgwalters
Copy link
Copy Markdown
Member

Excellent commit message!

@yuqi-zhang
Copy link
Copy Markdown
Contributor

Just to make sure, this only affects master branch? (Since whatever changed presumably only in 4.11)

@yuqi-zhang
Copy link
Copy Markdown
Contributor

Ah oops this is against layering
/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkyros, mkenigs, yuqi-zhang

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2022
@jkyros
Copy link
Copy Markdown
Member Author

jkyros commented Apr 6, 2022

/retest-required

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

1 similar comment
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@mkenigs
Copy link
Copy Markdown
Contributor

mkenigs commented Apr 6, 2022

/retest-required

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

6 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 7, 2022

@jkyros: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-op-single-node 7d8d6b8 link false /test e2e-gcp-op-single-node
ci/prow/e2e-gcp-single-node 7d8d6b8 link false /test e2e-gcp-single-node
ci/prow/e2e-aws-upgrade 7d8d6b8 link false /test e2e-aws-upgrade

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

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

@openshift-merge-robot openshift-merge-robot merged commit 9602871 into openshift:layering Apr 7, 2022
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants