Skip to content
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 secrets for pulling custom connector images #20272

Merged
merged 8 commits into from
Dec 9, 2022
Merged

Conversation

xiaohansong
Copy link
Contributor

What

Add secrets for pulling custom connector images

How

Adding an optional config field and pass it along to the pod creation process. Pod can accept them as a list and can pull images from both repositories if configured.

@xiaohansong xiaohansong requested a review from pmossman December 8, 2022 23:02
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Dec 8, 2022
@xiaohansong xiaohansong temporarily deployed to more-secrets December 8, 2022 23:03 — with GitHub Actions Inactive
@xiaohansong xiaohansong temporarily deployed to more-secrets December 8, 2022 23:03 — with GitHub Actions Inactive
@@ -466,6 +466,8 @@ public interface Configs {
*/
String getJobKubeMainContainerImagePullSecret();

Optional<String> getCustomImagePullSecret();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of adding a brand new config, it would make more sense for our OSS users if we instead updated the existing config to support a list.

For an OSS user who wants to use custom images, they're supposed to use the existing config, so having a second config will add confusion.

Basically, we just need to add support for defining multiple image pull secrets, so that Cloud can use two private image repositories instead of being limited to one.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, good call! I'll fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Sorry to make you rework it all 😅

Copy link
Contributor Author

@xiaohansong xiaohansong Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed - and retested on dev, this should work! (though some other bugs were detected, PR coming in...)

@xiaohansong xiaohansong temporarily deployed to more-secrets December 8, 2022 23:44 — with GitHub Actions Inactive
@xiaohansong xiaohansong temporarily deployed to more-secrets December 8, 2022 23:44 — with GitHub Actions Inactive
@xiaohansong xiaohansong requested a review from pmossman December 9, 2022 00:39
Copy link
Contributor

@pmossman pmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @xiaohansong! I left a few nits about making variable names plural. I was wondering if we should change the environment variable from JOB_KUBE_MAIN_CONTAINER_IMAGE_PULL_SECRET to JOB_KUBE_MAIN_CONTAINER_IMAGE_PULL_SECRETS but it might not be worth it since it'd be a breaking change for OSS deployments that use it (and we'd need to update the environment variables in Cloud too, though I'm more concerned with breaking changes in OSS)

I think renaming our code variables and method to be plural while leaving the environment variable alone is a good middle ground, at least in my opinion.

@xiaohansong xiaohansong temporarily deployed to more-secrets December 9, 2022 01:20 — with GitHub Actions Inactive
@xiaohansong xiaohansong temporarily deployed to more-secrets December 9, 2022 01:20 — with GitHub Actions Inactive
@xiaohansong xiaohansong merged commit d48d497 into master Dec 9, 2022
@xiaohansong xiaohansong deleted the xiaohan/dockerpull branch December 9, 2022 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants