Add feature flag to disable creds-init #3379

ghost · 2020-10-13T15:01:06Z

Prior to this commit Tekton scans service accounts attached to taskruns
and mounts secrets matching a specific format into all the Steps of a
Task. Then the entrypoint goes through those secrets and copies them
to the user's home directory so they can be used by git, docker, etc

This commit adds a feature-flag, disable-creds-init, that lets users
turn off the service account scanning behaviour. The default is "false"
so the creds-init behaviour remains enabled. Disabling creds-init can be
desirable for a lot of reasons:

The process is opaque and difficult to debug
It's not particularly well suited for non-root, multi-UID tasks.
Only a limited set of credential types are supported

Warning: Disabling creds-init will probably break PipelineResources.

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Release Notes

Tekton's built-in credential mechanism can now be disabled by setting the `disable-creds-init` feature-flag to "true".

tekton-robot · 2020-10-13T15:02:56Z

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/creds_init.go	91.7%	88.1%	-3.6
pkg/pod/pod.go	87.3%	87.4%	0.1

tekton-robot · 2020-10-13T15:21:33Z

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/creds_init.go	91.7%	90.5%	-1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

ghost · 2020-10-13T15:36:44Z

/kind feature

ghost · 2020-10-13T16:00:51Z

/test pull-tekton-pipeline-integration-tests

tekton-robot · 2020-10-13T16:16: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/creds_init.go	91.7%	92.9%	1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

tekton-robot · 2020-10-13T18:23: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/creds_init.go	91.7%	92.9%	1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

ghost · 2020-10-13T18:50:03Z

/test pull-tekton-pipeline-integration-tests

jerop · 2020-10-13T18:35:15Z

pkg/pod/creds_init.go

+	if cfg != nil && cfg.FeatureFlags != nil && cfg.FeatureFlags.DisableCredsInit == true {
+		return nil, nil, nil, nil
+	}


what do you think about using g.FeatureFlags.DisableCredsInit directly instead of comparing with true, like this:

if cfg != nil && cfg.FeatureFlags != nil && cfg.FeatureFlags.DisableCredsInit { return nil, nil, nil, nil }

jerop · 2020-10-13T18:41:15Z

docs/auth.md

+4. Tasks with Steps that run as non-root, particularly those where the UIDs
+change from Step to Step, can log more warning messages than those without,
+creating noise.


is there a way to clarify reason 4, it isn't clear to me what without is referring to

jerop · 2020-10-13T18:44:33Z

docs/auth.md

+1. Credentials must now be passed explicitly to Tasks either with Workspaces,
+environment variables (using `envFrom` in your Steps and a Task param to
+specify a Secret), or a custom volume and volumeMount definition.


could be helpful to reference the documentation for Workspaces, envFrom etc

jerop · 2020-10-13T18:47:31Z

pkg/pod/creds_init.go

+	if cfg != nil && cfg.FeatureFlags != nil && cfg.FeatureFlags.DisableCredsInit == true {
+		return nil, nil
+	}


same as above on using g.FeatureFlags.DisableCredsInit directly:

if cfg != nil && cfg.FeatureFlags != nil && cfg.FeatureFlags.DisableCredsInit { return nil, nil }

jerop · 2020-10-13T19:09:11Z

pkg/pod/pod.go

 		// Mount /tekton/creds with a fresh volume for each Step. It needs to
 		// be world-writeable and empty so creds can be initialized in there. Cant
 		// guarantee what UID container runs with.


may be useful to update this to mention that it mounts fresh volume for each step except when creds-init is disabled

tekton-robot · 2020-10-14T13:04: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/creds_init.go	91.7%	92.9%	1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

ghost · 2020-10-14T13:08:40Z

@jerop thanks a lot for the feedback! I've made the changes you requested.

Prior to this commit Tekton scans service accounts attached to taskruns and mounts secrets matching a specific format into all the Steps of a Task. Then the entrypoint goes through those secrets and copies them to the user's home directory so they can be used by `git`, `docker`, etc This commit adds a feature-flag, `disable-creds-init`, that lets users turn off the service account scanning behaviour. The default is "false" so the creds-init behaviour remains enabled. Disabling creds-init can be desirable for a lot of reasons: 1. The process is opaque and difficult to debug 2. It's not particularly well suited for non-root, multi-UID tasks. 3. Only a limited set of credential types are supported Warning: Disabling creds-init will largely prevent Git and Image PipelineResources from working.

tekton-robot · 2020-10-20T13:05:02Z

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/creds_init.go	91.7%	92.9%	1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

ghost · 2020-10-20T13:18:29Z

/test pull-tekton-pipeline-go-coverage
/test tekton-pipeline-unit-tests

tekton-robot · 2020-10-20T13:25: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/creds_init.go	91.7%	92.9%	1.2
pkg/pod/pod.go	87.3%	87.4%	0.1

tekton-robot · 2020-11-03T13:45:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

jerop · 2020-11-03T14:31:24Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 13, 2020

tekton-robot requested review from afrittoli and vdemeester October 13, 2020 15:01

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 13, 2020

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 Oct 13, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 13, 2020

jerop reviewed Oct 13, 2020

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2020

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2020

ghost added this to the Pipelines v0.18 milestone Oct 20, 2020

vdemeester approved these changes Nov 3, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2020

tekton-robot assigned jerop Nov 3, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2020

tekton-robot merged commit 42715eb into tektoncd:master Nov 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flag to disable creds-init #3379

Add feature flag to disable creds-init #3379

ghost commented Oct 13, 2020 •

edited by ghost

Loading

tekton-robot commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

ghost commented Oct 13, 2020

ghost commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

ghost commented Oct 13, 2020

jerop Oct 13, 2020

jerop Oct 13, 2020

jerop Oct 13, 2020

jerop Oct 13, 2020

jerop Oct 13, 2020

tekton-robot commented Oct 14, 2020

ghost commented Oct 14, 2020

tekton-robot commented Oct 20, 2020

ghost commented Oct 20, 2020

tekton-robot commented Oct 20, 2020

tekton-robot commented Nov 3, 2020

jerop commented Nov 3, 2020

Add feature flag to disable creds-init #3379

Add feature flag to disable creds-init #3379

Conversation

ghost commented Oct 13, 2020 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

ghost commented Oct 13, 2020

ghost commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

tekton-robot commented Oct 13, 2020

ghost commented Oct 13, 2020

jerop Oct 13, 2020

Choose a reason for hiding this comment

jerop Oct 13, 2020

Choose a reason for hiding this comment

jerop Oct 13, 2020

Choose a reason for hiding this comment

jerop Oct 13, 2020

Choose a reason for hiding this comment

jerop Oct 13, 2020

Choose a reason for hiding this comment

tekton-robot commented Oct 14, 2020

ghost commented Oct 14, 2020

tekton-robot commented Oct 20, 2020

ghost commented Oct 20, 2020

tekton-robot commented Oct 20, 2020

tekton-robot commented Nov 3, 2020

jerop commented Nov 3, 2020

ghost commented Oct 13, 2020 •

edited by ghost

Loading