OCPBUGS-25662: Add Image Credential Provider flags for Kubelet on AWS #4103
Conversation
openshift-ci-robot
added
jira/severity-important
|
@JoelSpeed: This pull request references Jira Issue OCPBUGS-25662, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test unit |
|
@JoelSpeed we had a chat about this in the QE sync meeting, and it looks like the CI failures here are not flakes. Do you have a theory on why this is breaking installation on many providers? |
|
Will need to bring up a cluster and have a look at what's going on to understand why this is breaking, will see if I can jump on that |
JoelSpeed
force-pushed
the
aws-ecr-credential-provider
branch
2 times, most recently
from
b7bf22a
to
d25ba25
Compare
| credentialProviderConfigFlag := "--image-credential-provider-config=/etc/kubernetes/credential-providers/" | ||
| switch cfg.Infra.Status.PlatformStatus.Type { | ||
| case configv1.AWSPlatformType: | ||
| return fmt.Sprintf("%s %s%s", credentialProviderBinDirFlag, credentialProviderConfigFlag, "ecr-credential-provider.yaml") |
There was a problem hiding this comment.
question (non-blocking): Would it make more sense to make ecr-credential-provider.yaml part of credentialProviderConfigFlag variable? In other words:
credentialProviderConfigFlag := "--image-credential-provider-config=/etc/kubernetes/credential-providers/ecr-credential-provider.yaml"Then the return statement would simplify to:
case configv1.AWSPlatformType:
return fmt.Sprintf("%s %s", credentialProviderBinDirFlag, credentialProviderConfigFlag)There was a problem hiding this comment.
In the future, this switch will support more platforms.We will want those variables separate so they can be combined in different ways to get the correct flags for the different platforms, I'm expecting the config file name itself to be different per platform
There was a problem hiding this comment.
Got it, thanks for the clarification!
|
Overall, this change looks good; modulo the CI failures. I just have a single non-blocking question, but feel free to ignore it if there are more pressing matters. |
|
/test e2e-aws-ovn |
JoelSpeed
force-pushed
the
aws-ecr-credential-provider
branch
from
d25ba25
to
2da4c76
Compare
|
/restest-required |
|
/jira refresh |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
jira/valid-bug
|
@JoelSpeed: This pull request references Jira Issue OCPBUGS-25662, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yliu127. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
/retest-required |
|
Looks like the OS image still doesn't have the binary in place yet |
|
The package has made it into RHCOS 416.92.202401170945-0, just need to wait on a 4.16 CI build to bump to that version and be accepted. |
|
Just checked on the CI builds again, still not updated to a newer RHCOS build yet, I believe there is an issue that ART are working on |
|
/retest-required |
|
e2e-gcp-op-single-node is a known failure due to https://issues.redhat.com/browse/OCPBUGS-26489 when time comes to merge ping me or MCO team to override that test if it's still broken. |
|
/hold cancel |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@cheesesashimi: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
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. |
|
/override ci/prow/e2e-gcp-op-single-node |
|
@cheesesashimi: Overrode contexts on behalf of cheesesashimi: ci/prow/e2e-gcp-op-single-node In response to this:
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. |
|
@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/e2e-gcp-op-single-node In response to this:
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. |
|
@JoelSpeed: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/retest-required |
openshift-merge-bot
bot
merged commit 6745e60
into
openshift:master
|
@JoelSpeed: Jira Issue OCPBUGS-25662: Some pull requests linked via external trackers have merged:
The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-25662 has not been moved to the MODIFIED state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build openshift-proxy-pull-test-container-v4.16.0-202401191549.p0.g6745e60.assembly.stream for distgit openshift-proxy-pull-test. |
|
/cherry-pick release-4.15 |
|
@JoelSpeed: new pull request created: #4134 In response to this:
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. |
|
Please revert, this breaks OKD, which doesn't ship this RPM |
- What I did
Adds configuration for the ECR credential provider to kubelet on AWS.
Unit tests show new flags being set correctly.
Depends on the ecr-credential-provider RPM shipping in RHCOS, this is currently a WIP.
/hold
- How to verify it
Kubelet should be able to pull an image from a private ECR registry.
- Description for the changelog