Use podTemplate ImagePullSecrets for Entrypoint Image Lookup #4496

Yongxuanzhang · 2022-01-19T03:14:24Z

Changes

This PR aims at closing the #3799.
Prior to this commit, we don't use imagePullSecrets from podTemplate to fetch secrets and resolve entrypoints for images. This commit aims at passing the imagePullSecrets to k8schain so it can append the imagePullSecrets into key chain.

This PR also adds unit test for EntrypointCache.get() implementation and improves the test coverage from 0% to 58%.

Note that the controller's service account has the access to secrets. If it is deprecated or the access is removed, the Rolebinding is required for users to avoid errors when calling k8schain. See comments in #3799.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

This PR is fixing an issue where entrypoint image lookup did not have access to or ignored the secrets specified in the podTemplate.

Yongxuanzhang · 2022-01-19T05:03:01Z

/test pull-tekton-pipeline-integration-tests

wlynch · 2022-01-25T19:33:44Z

/assign @wlynch

wlynch

Impl looks reasonable - though there aren't really tests for the remote behavior. :(

The kubeclient should be easy enough to fake out (https://pkg.go.dev/k8s.io/client-go/kubernetes/fake), but I'm not sure off the top of my head how we would fake out the remote containerregistry call.

pipeline/pkg/remote/oci/resolver_test.go

Lines 263 to 268 in 4186dc9

    
           ref, err := test.CreateImageWithAnnotations(fmt.Sprintf("%s/testociresolve/%s", u.Host, tc.name), tc.mapper, tc.objs...) 
        
           if err != nil { 
        
           	t.Fatalf("could not push image: %#v", err) 
        
           } 
        
           resolver := oci.NewResolver(ref, authn.DefaultKeychain)

something we can copy for this?

/cc @imjasonh In case you have any other ideas.

pkg/pod/entrypoint_lookup_impl.go

tekton-robot · 2022-01-27T16:39:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

imjasonh

Looks good, and thanks for adding tests! Just a few small comments, otherwise lgtm.

docs/podtemplates.md

imjasonh · 2022-01-27T17:02:24Z

docs/podtemplates.md

@@ -95,6 +95,40 @@ Pod templates support fields listed in the table below.
 	</tbody>
 </table>

+## Use <code>imagePullSecrets</code> to lookup entrypoint
+
+If no command is configured in <code>task</code> and <code>imagePullSecrets</code> is configured in <code>podTemplate</code>, the Tekton Controller will look up the entrypoint of image with <code>imagePullSecrets</code>. If the Tekton controller's service account is not granted the access to secrets in different namespace. You need to grant the access via RoleBinding:


Is this necessary, since the SA already has cluster-wide read access to secrets?

If this is meant to document what's necessary for folks that explicitly disable Tekton's cluster-wide secrets access, I think we can just assume that users sophisticated/paranoid enough to do that are able to figure out what else they'd need to do to re-enable it for required secrets.

This is in case the access may be removed in the future, Billy told me that it might be removed so it would be better to document it.

pkg/pod/entrypoint_lookup_impl_test.go

imjasonh · 2022-01-27T17:06:02Z

pkg/pod/entrypoint_lookup_impl_test.go

+	fakeclient "k8s.io/client-go/kubernetes/fake"
+)
+
+func TestGet(t *testing.T) {


I'm always glad to have more tests, and this should ensure the code is at least exercised, but it doesn't currently really ensure that the specified imagePullSecret is used when making requests to the registry. I think it's worth keeping, but maybe we should document its limitations so we don't get a false sense of security here.

I have been working on trying to address. Please let me know if the latest change makes sense or not. 😄
Thanks!

tekton-robot · 2022-02-01T02:44:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

tekton-robot · 2022-02-01T03:15:22Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

tekton-robot · 2022-02-01T15:29:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

tekton-robot · 2022-02-01T15:38:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

tekton-robot · 2022-02-01T15:46:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	54.5%	54.5

tekton-robot · 2022-02-01T16:12:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	56.4%	56.4

pkg/pod/entrypoint_lookup_impl_test.go

wlynch · 2022-02-01T20:00:33Z

pkg/pod/entrypoint_lookup_impl_test.go

+			username: "",
+			password: "",
+		},
+	} {


Might also be worth testing a secret that exists but isn't authorized, no secret (in case empty string is different than nil), and a secret that does exist at all.

pkg/pod/entrypoint_lookup_impl_test.go

tekton-robot · 2022-02-01T20:10:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	56.4%	56.4

tekton-robot · 2022-02-01T20:11:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	56.4%	56.4

tekton-robot · 2022-02-01T20:21:03Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	56.4%	56.4

Yongxuanzhang · 2022-02-03T20:26:34Z

/test pull-tekton-pipeline-alpha-integration-tests

imjasonh

This change looks great! Thanks for working on this. 👍

docs/podtemplates.md

imjasonh · 2022-02-04T15:44:00Z

pkg/pod/entrypoint_lookup_impl_test.go

+		basicSecret:      generateSecret(u.Host, username, password),
+		imagePullSecrets: []corev1.LocalObjectReference{},
+		wantErr:          true,
+	}} {


The secret is created from .docker/config.json, the file is written only when user has successfully login.

FWIW, this isn't technically true. You can edit that file to contain any username/password, and many tools implement "login" simply be editing that file without validating that the username/password is correct at all.

If there are improvements we can make to that keychain, especially if it helps Tekton users, we should consider them. I'll happily approve :)

tekton-robot · 2022-02-04T15:55:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	58.2%	58.2

tekton-robot · 2022-02-04T16:17:07Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	58.2%	58.2

wlynch

/lgtm

🎉

tekton-robot · 2022-02-04T20:17:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint_lookup_impl.go	0.0%	58.2%	58.2

Yongxuanzhang · 2022-02-04T20:49:45Z

/lgtm

🎉

Sorry the squash cleared the review, may you review it again. 😄

wlynch

/lgtm

Yongxuanzhang · 2022-02-08T18:32:52Z

/assign @bobcatfish

imjasonh · 2022-02-08T18:49:04Z

/lgtm

pritidesai · 2022-02-10T19:20:24Z

hey @imjasonh will you be able to approve it, @wlynch has lgtmed this 🙏

pritidesai · 2022-02-10T19:39:06Z

Please kindly let me know if I need to add any release note.

@Yongxuanzhang please add the release notes. This PR is fixing an issue where entrypoint image lookup did not have access to or ignored the secrets specified in the podTemplate. The users can opt in by creating appropriate role binding to allow image lookup using these secrets.

Yongxuanzhang · 2022-02-11T15:17:52Z

Please kindly let me know if I need to add any release note.

@Yongxuanzhang please add the release notes. This PR is fixing an issue where entrypoint image lookup did not have access to or ignored the secrets specified in the podTemplate. The users can opt in by creating appropriate role binding to allow image lookup using these secrets.

Hi, @pritidesai Thank you for reminding me of the release note!

imjasonh · 2022-02-11T15:50:08Z

Thanks!

/approve

tekton-robot · 2022-02-11T15:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imjasonh, wlynch

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:

~~OWNERS~~ [imjasonh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Yongxuanzhang · 2022-02-11T16:52:30Z

Fixes #3799

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 19, 2022

tekton-robot requested review from afrittoli and pritidesai January 19, 2022 03:14

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 19, 2022

This comment has been minimized.

Sign in to view

Yongxuanzhang force-pushed the use-imagepullsecrets branch from b4a2ac5 to 5c0891e Compare January 25, 2022 01:36

Yongxuanzhang marked this pull request as ready for review January 25, 2022 02:05

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2022

tekton-robot assigned wlynch Jan 25, 2022

wlynch reviewed Jan 26, 2022

View reviewed changes

pkg/pod/entrypoint_lookup_impl.go Outdated Show resolved Hide resolved

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 27, 2022

imjasonh reviewed Jan 27, 2022

View reviewed changes

Yongxuanzhang force-pushed the use-imagepullsecrets branch from a38f423 to 54b36e7 Compare February 1, 2022 15:34

Yongxuanzhang requested a review from wlynch February 1, 2022 16:38

wlynch requested changes Feb 1, 2022

View reviewed changes

imjasonh reviewed Feb 4, 2022

View reviewed changes

wlynch reviewed Feb 4, 2022

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2022

use podtemplate imagepullsecrets to resolve entrypoint

af80197

Yongxuanzhang force-pushed the use-imagepullsecrets branch from 8fca8ee to af80197 Compare February 4, 2022 20:14

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2022

Yongxuanzhang requested a review from wlynch February 4, 2022 20:48

wlynch approved these changes Feb 7, 2022

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2022

tekton-robot assigned bobcatfish Feb 8, 2022

tekton-robot assigned imjasonh Feb 8, 2022

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Feb 11, 2022

Yongxuanzhang requested a review from imjasonh February 11, 2022 15:18

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2022

tekton-robot merged commit c6bdf38 into tektoncd:main Feb 11, 2022

Yongxuanzhang mentioned this pull request Feb 11, 2022

Entrypoint Image Lookup does not use PodTemplate ImagePullSecrets #3799

Closed

	ref, err := test.CreateImageWithAnnotations(fmt.Sprintf("%s/testociresolve/%s", u.Host, tc.name), tc.mapper, tc.objs...)
	if err != nil {
	t.Fatalf("could not push image: %#v", err)
	}

	resolver := oci.NewResolver(ref, authn.DefaultKeychain)

Use podTemplate ImagePullSecrets for Entrypoint Image Lookup #4496

Use podTemplate ImagePullSecrets for Entrypoint Image Lookup #4496

Conversation

Yongxuanzhang commented Jan 19, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

This comment has been minimized.

Yongxuanzhang commented Jan 19, 2022

wlynch commented Jan 25, 2022

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 27, 2022

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Jan 27, 2022

Choose a reason for hiding this comment

Yongxuanzhang Feb 1, 2022

Choose a reason for hiding this comment

imjasonh Jan 27, 2022

Choose a reason for hiding this comment

Yongxuanzhang Feb 1, 2022

Choose a reason for hiding this comment

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

wlynch Feb 1, 2022

Choose a reason for hiding this comment

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

tekton-robot commented Feb 1, 2022

Yongxuanzhang commented Feb 3, 2022

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Feb 4, 2022

Choose a reason for hiding this comment

tekton-robot commented Feb 4, 2022

tekton-robot commented Feb 4, 2022

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 4, 2022

Yongxuanzhang commented Feb 4, 2022

wlynch left a comment

Choose a reason for hiding this comment

Yongxuanzhang commented Feb 8, 2022

imjasonh commented Feb 8, 2022

pritidesai commented Feb 10, 2022

pritidesai commented Feb 10, 2022

Yongxuanzhang commented Feb 11, 2022 • edited Loading

imjasonh commented Feb 11, 2022

tekton-robot commented Feb 11, 2022

Yongxuanzhang commented Feb 11, 2022

Yongxuanzhang commented Jan 19, 2022 •

edited

Loading

Yongxuanzhang commented Feb 11, 2022 •

edited

Loading