-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a patch to make ECR credentials work #4084
Add a patch to make ECR credentials work #4084
Conversation
/retest |
cc @symfrog @Justin2997 either of you interested in testing this out? |
@jonjohnsonjr That seems good to me. I don't have the infra to test it anymore. Maybe @lavoiedn can test it. |
Funny story that... Our cluster got destroyed and we're still bringing it back 😂 Not able to test it at the moment, will update if I get a chance to fix our setup! |
d82c485
to
e1bb6f3
Compare
@tzununbekov can you try to test this |
I've installed
while k8s job on the same cluster and image succeeded:
Is that a test case or do I miss something? |
UPD: looks like I should run |
Update from us, we got our cluster back and we're just about to need to test this. @januaryh you'll need this fix when you try to fetch the image for the rabbitmq adapter/controller. CC: @alexgervais |
@tzununbekov is it a fortio issue? knative/test-infra#968 |
This seems to be working again |
e1bb6f3
to
6a2d54f
Compare
I'm still getting |
I just tried this as well, and still getting
Did I miss any steps? Here is one of the log lines from the controller after I deployed:
cc: @mattmoor |
@tzununbekov @ryanbrainard thanks both for trying this -- can you see if this brings up anything relevant?
E.g. I hit this but that's expected because I'm running on GKE:
|
6a2d54f
to
74a0738
Compare
@jonjohnsonjr Seeing this:
|
@jonjohnsonjr Good news, I was able to get this to work! It turned out to be a kiam issue that was blocking this from working last time I tried it. I had to give ECR access via IAM to the @tzununbekov perhaps you have a similar issue? I was able to debug this by looking at the controller's events ( |
726a3bc
to
88e5782
Compare
@jonjohnsonjr I just re-tested this again after your rebase on a brand new fresh cluster (I was mucking around a lot on the other one, so wanted to be 100% sure). Still works 👍 |
/assign @mattmoor |
88e5782
to
cda3772
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonjohnsonjr, mattmoor 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 |
The patch from knative#4084 has been addressed. Addresses knative#4549
Fixes #1996
This patches in kubernetes/kubernetes#75585 and kubernetes/kubernetes#75587 in order to use the updated ECR credential provider. We'll be able to drop these patches once we've bumped our k8s dependencies to 1.15, but that's pretty far off.
Proposed Changes
Release Note