-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Oauth2 support without Python #626
Conversation
Okay, this looks a bit better, this could use some tests similar to https://github.com/bazel-contrib/rules_oci/blob/2.x/e2e/assertion/oci_pull_auth_tests.bats We will also need better error messages. |
While trying to oci.pull an azure container registry, I still get this error:
The PR #600 had actually fixed that issue for me though I didn't like the way it was done either |
we should somehow incorporate that change here. identity tokens are not something we support at the moment, so we might as well it right at once and improve on it. |
If I |
I think I fixed everything requested except for tests. Current tests do not pass for me so it is hard to develop new, any suggestions?
And this is the test log that I get: |
I ran the bazel with the above three changes ERROR:
For my case, it doesn't seem to reach line 287, because I did not get the fail(IDENTITY_TOKEN_WARNING) without the bazelrc changes |
I am not sure if the container registry that you are using is compatible with this fix or what is wrong in your case. Firstly, I will post my reproduction if it can help on any way:
First I get 403, my azure subscription was actually disabled, then I enabled it and logged in successfully, then I can download the image. In the error stack trace you can also see that it enters the new code. You can see details of my setup with further commands. For you, new code is not used at all as you mentioned. The condition For me, it goes to line 234, i.e. calls
and then calls
and based on I am curious why it works differently for you, you might want to trace how credentials are obtained for you and compare with what I have written here. |
So, I think the problem is in parsing the ~/.docker/config.json:
Line 240 looks for what follows ":" as password but as per this config it is ["identity_token"] . Since here auth is weirdly ending with ":" |
My best bet is that you are using linux, I managed to reproduce the issue on ubuntu. So the fix in the current PR works for Mac OS, but not for linux. |
I integrated fix from @ddeville to this PR, I believe we now have a complete support of azure across operating systems. And for oauth2 hopefully in all cases. |
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.
This looks fantastic now. Only we knew how to add some tests for this.
I am going to hold this for a little bit so we can figure out some testing infra for this. i'll circle back to this next week. |
Hmm, there are some failures. Can you handle those? |
@thesayyn it was a simple check missing, now it's all good |
Great work! Looking forward to having it in the next releases. |
Thank you. @thesayyn will this also get pulled to 2.x? |
Co-authored-by: xbog300 <[email protected]>
I'm still seeing issues with azure actually. It might work if the login is |
Due to inactivity, I picked up this PR: #581
I implememented the mentioned workarounds, i.e. in case of windows, I try to run script using powershell, otherwise, I fallback to curl and then wget, both wrapped in a bash script.
I tested these with an azure repo which requires oauth2. I also tested it on windows. I found that the config.json is not found due to "stat" binary not existing on windows, so I fixed this too.