add imagePullSecrets to sidecar injection template #24365

tariq1890 · 2020-06-03T05:34:38Z

Since imagePullSecrets is a part of the SidecarInjectionSpec, it would be good to illustrate the usage of this through the manifest charts. Moreover, global values are used as settings in the sidecar injection template, so it only makes sense that we use the same pattern for providing the imagePullSecrets via .Values.global

[X] Configuration Infrastructure
[ ] Docs
[X] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[X] User Experience
[ ] Developer Infrastructure

morvencao

Have you tested this case? What if the original pod already has imagePullSecrets?

howardjohn

won't you need to add this secret to every namespace?

We should at least add a test under inject_test.go for this if we are going to add this.

tariq1890 · 2020-06-03T15:33:14Z

Have you tested this case? What if the original pod already has imagePullSecrets?

In this case, it should append to the list of imagePullSecrets. I thought the code already does that?

tariq1890 · 2020-06-03T15:34:29Z

won't you need to add this secret to every namespace?

I think this is OK to assume. If any istio participating workload needs to pull down the istio-proxy image, the pullSecrets will have to be made available in all namespaces

howardjohn · 2020-06-03T15:41:27Z

In this case, it should append to the list of imagePullSecrets. I thought the code already does that?

If you add a test then we won't have to guess 🙂

googlebot · 2020-06-04T08:09:32Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

add imagePullSecrets to sidecar injection template

googlebot · 2020-06-04T08:11:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tariq1890 · 2020-06-04T08:36:45Z

pkg/kube/inject/inject.go

@@ -761,6 +761,8 @@ func IntoObject(sidecarTemplate string, valuesConfig string, revision string, me

 	podSpec.DNSConfig = spec.DNSConfig

+	podSpec.ImagePullSecrets = append(podSpec.ImagePullSecrets, spec.ImagePullSecrets...)


Thanks to the unit tests, I added this bugfix :).

Good call @howardjohn !

tariq1890 · 2020-06-04T15:50:12Z

Thanks for the approve @howardjohn . Do you agree this needs to be cherrypicked to release-1.6?

sdake

LGTM. At some point, @istio/wg-environments-maintainers needs to have a discussion and agreement about "one API" to rule hem all. Currently we have three, and this PR adds to the deprecated API global.yaml. I understand completely why this option is necessary, although at some point, we need to consider a reorg of all of the 3 APIs (values,yaml, IsitoOperator, and MeshConfig) into one API.

The current (fuzzy) agreement as I understand it is to push everything to MeshConfig. I am not convinced this is a good choice as MeshConfig is already poorly organized.

What I'd like to see is one new object, that every component understands, that meets the criteria for a stable API. This viewpoint means deprecating MeshConfig in favor of IstioConfig - which would be a new take on IstioOperator - but with all of the stuff in MeshConfig.

For now, we can make an exception and add to values.yaml - but again, the WG needs to have further discussion and a concrete agreement on one API - used throughout istiod (mostly PIlot), as well as istioctl and the standalone operator and the helm charts.

Cheers,
-steve

sdake · 2020-06-04T17:54:46Z

cc / @rcernich I know you may be doing some work in this area. Also @ostromart is looking into this deeply.

I can bring us together - but would need an initial proposal.

howardjohn · 2020-06-04T17:55:44Z

.global.imagePullSecrets is not a new option, it has existed forever. Just not applied to injection template

sdake · 2020-06-04T17:56:21Z

@tariq1890 I've selected this PR for backport to 1.6.

Cheers,
-steve

istio-testing · 2020-06-04T18:34:34Z

In response to a cherrypick label: #24365 failed to apply on top of branch "release-1.6":

Using index info to reconstruct a base tree...
M	manifests/charts/global.yaml
M	manifests/charts/istio-control/istio-discovery/files/gen-istio.yaml
M	manifests/charts/istio-control/istio-discovery/files/injection-template.yaml
M	manifests/charts/istiod-remote/files/gen-istiod-remote.yaml
M	manifests/charts/istiod-remote/files/injection-template.yaml
M	operator/cmd/mesh/profile-common.go
M	operator/cmd/mesh/testdata/manifest-generate/data-snapshot/charts/istio-control/istio-discovery/files/gen-istio.yaml
M	operator/cmd/mesh/testdata/manifest-generate/data-snapshot/charts/istio-control/istio-discovery/files/injection-template.yaml
M	operator/cmd/mesh/testdata/manifest-generate/output/all_on.golden-show-in-gh-pull-request.yaml
M	operator/cmd/mesh/testdata/manifest-generate/output/flag_output_set_profile.golden.yaml
M	operator/cmd/mesh/testdata/manifest-generate/output/flag_set_values.golden.yaml
M	operator/cmd/mesh/testdata/manifest-generate/output/pilot_default.golden.yaml
M	pkg/kube/inject/inject.go
M	pkg/kube/inject/inject_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/kube/inject/inject_test.go
Auto-merging pkg/kube/inject/inject.go
Auto-merging operator/cmd/mesh/testdata/manifest-generate/output/pilot_default.golden.yaml
Auto-merging operator/cmd/mesh/testdata/manifest-generate/output/flag_set_values.golden.yaml
Auto-merging operator/cmd/mesh/testdata/manifest-generate/output/flag_output_set_profile.golden.yaml
CONFLICT (content): Merge conflict in operator/cmd/mesh/testdata/manifest-generate/output/flag_output_set_profile.golden.yaml
Auto-merging operator/cmd/mesh/testdata/manifest-generate/output/all_on.golden-show-in-gh-pull-request.yaml
CONFLICT (content): Merge conflict in operator/cmd/mesh/testdata/manifest-generate/output/all_on.golden-show-in-gh-pull-request.yaml
Auto-merging operator/cmd/mesh/testdata/manifest-generate/data-snapshot/charts/istio-control/istio-discovery/files/injection-template.yaml
Auto-merging operator/cmd/mesh/testdata/manifest-generate/data-snapshot/charts/istio-control/istio-discovery/files/gen-istio.yaml
CONFLICT (content): Merge conflict in operator/cmd/mesh/testdata/manifest-generate/data-snapshot/charts/istio-control/istio-discovery/files/gen-istio.yaml
Auto-merging operator/cmd/mesh/profile-common.go
Auto-merging manifests/charts/istiod-remote/files/injection-template.yaml
Auto-merging manifests/charts/istiod-remote/files/gen-istiod-remote.yaml
Auto-merging manifests/charts/istio-control/istio-discovery/files/injection-template.yaml
Auto-merging manifests/charts/istio-control/istio-discovery/files/gen-istio.yaml
Auto-merging manifests/charts/global.yaml
error: Failed to merge in the changes.
Patch failed at 0001 add imagePullSecrets to sidecar injection template

istio-testing · 2020-06-04T18:34:36Z

In response to a cherrypick label: new issue created for failed cherrypick: #24437

(cherry picked from commit 2e3459b)

tariq1890 · 2020-06-04T21:51:15Z

#24446

tariq1890 · 2020-06-04T21:51:33Z

Thanks @sdake @howardjohn

(cherry picked from commit 2e3459b)

rcernich · 2020-06-16T15:25:26Z

@sdake, I'm working on a POC that reorganizes the settings in values.yaml based on feature/area of concern. I've also drafted an RFC, which should be ready for review next week.

add imagePullSecrets to sidecar injection template

tariq1890 requested a review from a team as a code owner June 3, 2020 05:34

istio-testing added the size/S label Jun 3, 2020

googlebot added the cla: yes label Jun 3, 2020

istio-policy-bot added area/config area/environments area/user experience labels Jun 3, 2020

morvencao reviewed Jun 3, 2020

View reviewed changes

howardjohn reviewed Jun 3, 2020

View reviewed changes

tariq1890 requested review from costinm, linsun, rshriram and a team as code owners June 4, 2020 08:09

istio-testing added size/XXL and removed size/S labels Jun 4, 2020

googlebot added cla: no and removed cla: yes labels Jun 4, 2020

add imagePullSecrets to sidecar injection template

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: DFC94E4A008B908A

Learn about vigilant mode

ab2aa4d

add imagePullSecrets to sidecar injection template

tariq1890 force-pushed the inject-img-secrets branch from fa559bb to ab2aa4d Compare June 4, 2020 08:11

googlebot added cla: yes and removed cla: no labels Jun 4, 2020

istio-testing added size/XL and removed size/XXL labels Jun 4, 2020

tariq1890 commented Jun 4, 2020

View reviewed changes

howardjohn approved these changes Jun 4, 2020

View reviewed changes

sdake approved these changes Jun 4, 2020

View reviewed changes

sdake added the cherrypick/release-1.6 label Jun 4, 2020

istio-testing merged commit 2e3459b into master Jun 4, 2020

istio-testing deleted the inject-img-secrets branch June 4, 2020 18:34

istio-testing mentioned this pull request Jun 4, 2020

[release-1.6] add imagePullSecrets to sidecar injection template #24437

Closed

ericvn mentioned this pull request Jun 17, 2020

Release notes for 1.6.3 istio/istio.io#7575

Merged

nschhina pushed a commit to nschhina/istio that referenced this pull request Jun 18, 2020

add imagePullSecrets to sidecar injection template (istio#24365)

91ef626

add imagePullSecrets to sidecar injection template

		@@ -761,6 +761,8 @@ func IntoObject(sidecarTemplate string, valuesConfig string, revision string, me

		podSpec.DNSConfig = spec.DNSConfig

		podSpec.ImagePullSecrets = append(podSpec.ImagePullSecrets, spec.ImagePullSecrets...)

add imagePullSecrets to sidecar injection template #24365

add imagePullSecrets to sidecar injection template #24365

Conversation

tariq1890 commented Jun 3, 2020 • edited by istio-policy-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morvencao left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

tariq1890 commented Jun 3, 2020

Uh oh!

tariq1890 commented Jun 3, 2020

Uh oh!

howardjohn commented Jun 3, 2020

Uh oh!

googlebot commented Jun 4, 2020

Uh oh!

googlebot commented Jun 4, 2020

Uh oh!

tariq1890 Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

tariq1890 commented Jun 4, 2020

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

sdake commented Jun 4, 2020

Uh oh!

howardjohn commented Jun 4, 2020

Uh oh!

sdake commented Jun 4, 2020

Uh oh!

istio-testing commented Jun 4, 2020

Uh oh!

istio-testing commented Jun 4, 2020

Uh oh!

tariq1890 commented Jun 4, 2020

Uh oh!

tariq1890 commented Jun 4, 2020

Uh oh!

rcernich commented Jun 16, 2020

Uh oh!

tariq1890 commented Jun 3, 2020 •

edited by istio-policy-bot

Loading