Skip to content

[image-builder-bob] Prioritize using additional auth #10235

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

Closed
wants to merge 1 commit into from

Conversation

princerachit
Copy link
Contributor

@princerachit princerachit commented May 25, 2022

Description

This implements the PR #10094 in a different way. Instead of changing the names of env vars, override existing vars with additional auth. I am not sure how this will impact gitpod installations. AFAIK additional auth is only used in the context of user supplied auth, so using it instead of existing configured auth should make sense.

Prefer merging #9337 first as that will give us more control in testing and debugging issues related to this PR or any changes related to image-builder in general without impacting production env.

Related Issue(s)

The initial revert was done due to an incident see: INC-150
#10089

How to test

Refer to original PR #10094

Release Notes

Fix conflicting auth selection for image-builder-bob

Documentation

@princerachit princerachit requested a review from a team May 25, 2022 06:26
@github-actions github-actions bot added team: workspace Issue belongs to the Workspace team and removed size/M labels May 25, 2022
@princerachit
Copy link
Contributor Author

/hold

Prefer merging #9337 first

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

Hi @princerachit If I remember correctly, base and target are both our registry, they are just used for different purposes, and base does not refer to FROM in the dockerfile

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

The FROM defined by the user in the Dockerfile do not go through the bob_proxy, because they do not start with localhost

@princerachit
Copy link
Contributor Author

Hi @princerachit If I remember correctly, base and target are both our registry, they are just used for different purposes, and base does not refer to FROM in the dockerfile

Let me correct the naming

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

it not about naming...
image

as you can see, base and target both out registry...

If you want to download a private registry image, this is not the place to change

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

I think this one is real source image auth...

err := configFile.LoadFromReader(bytes.NewReader([]byte(fmt.Sprintf(`{"auths": %v }`, authLayer))))
if err != nil {

@princerachit princerachit force-pushed the prs/use-dedicated-auth branch from fc2f633 to 7d75592 Compare May 25, 2022 06:48
@princerachit princerachit marked this pull request as draft May 25, 2022 06:48
@princerachit princerachit changed the title Revert "Revert "[image-builder-bob] Use separate auth for target and … [image-builder-bob] Prioritize using additional auth May 25, 2022
@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

In fact, we're using lcoalhost:8080 as the push registry, so anything we do on bob_proxy is unlikely to affect the pull image authentication

@princerachit
Copy link
Contributor Author

princerachit commented May 25, 2022

I think this one is real source image auth...

err := configFile.LoadFromReader(bytes.NewReader([]byte(fmt.Sprintf(`{"auths": %v }`, authLayer))))
if err != nil {

👍🏾
I think that part of the code is dead atm. I have pushed a simpler change. Will test it out now.

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

I mean it is useless to make any changes on the bob_proxy...

err := configFile.LoadFromReader(bytes.NewReader([]byte(fmt.Sprintf(`{"auths": %v }`, authLayer))))
if err != nil {

If you want to support pulling images from the user's private registry, then you have to make changes in this location

@iQQBot iQQBot self-assigned this May 25, 2022
@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

Another thing is that I was under the impression that we don't have a place to pass user auth, so if users want to use a private registry, how do they configure it?

@princerachit
Copy link
Contributor Author

This is the PR which configures using a private registry to pull image. The caveat is, you can not use it in dockerfile.

#8550

If you look at the PR the additional auth supplied should be used to pull the image. But in a special case when such config already exist in the pull secret then this causes a conflict of credential selection.

I think the changes I just pushed cannot really address it and would rather cause other issues when trying to push to container registry. We probably need to separate out the variables to distinguish user provided auth for image pulling vs auth for container registry used by gitpod.

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

OK, now I get it

@princerachit
Copy link
Contributor Author

/hold

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

If it is based on this premise, the original PR is indeed available 🙏
Sorry for the trouble

@iQQBot
Copy link
Contributor

iQQBot commented May 25, 2022

But prefer don't change the ENV name 😂

@stale
Copy link

stale bot commented Jun 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jun 10, 2022
@sagor999
Copy link
Contributor

not stale

@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jun 10, 2022
@kylos101 kylos101 added the meta: never-stale This issue can never become stale label Jun 21, 2022
@princerachit
Copy link
Contributor Author

Closing as this is not the right approach to fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants