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

Mark failure to copy cred to user home as warning #3395

Merged
merged 1 commit into from Oct 19, 2020
Merged

Mark failure to copy cred to user home as warning #3395

merged 1 commit into from Oct 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2020

Contributes to #3399

Changes

Prior to this commit a message could be seen in step logs
when tekton failed to copy a credential from /tekton/creds
to /tekton/home or $HOME (if disable-home-override feature
flag is "true"). That message can be confusing because it
appears to be an error when in actual fact it's a warning.

The message appears when Steps inside a Task run with
varying UIDs - one UID may copy the credentials first
and then subsequent steps will attempt to copy over them.
If a Step running as root, for examples, copies creds
into /tekton/home, other non-root Steps will not be able
to utilize those credentials. Unfortunately this can manifest
in our own PipelineResources where the Steps they inject
use a mix of root & non-root base images.

The message can also appear if a user has explicltly
mounted credentials (via workspace or volumemount) in
/tekton/home and has also attached a service account
to the task with creds-init credentials on it.

The message serves as an indicator of a potential problem but
not 100% guarantee that any issues in a Step are related to it.

This commit updates the message to include a "warning:"
prefix to indicate that it might (though not always)
be a potential source of issues in your Step.

This commit also adds documentation to docs/auth.md to help
users diagnose the impact of this message (including when
they can safely ignore it).

Submitter Checklist

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

Release Notes

A message related to copying credentials has been more clearly marked as a warning to reduce confusion when it appears in Step logs.

/kind cleanup

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 15, 2020
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 15, 2020
@afrittoli
Copy link
Member

/test check-pr-has-kind-label

@ghost
Copy link
Author

ghost commented Oct 16, 2020

/hold

This needs some additional documentation added in auth.md to help users searching for more info on the error message.

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2020
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 16, 2020
@ghost
Copy link
Author

ghost commented Oct 16, 2020

/remove-kind cleanup
/kind documentation

@tekton-robot tekton-robot added kind/documentation Categorizes issue or PR as related to documentation. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 16, 2020
@ghost
Copy link
Author

ghost commented Oct 16, 2020

/hold cancel

I've added a section to our ever-growing auth.md to try and capture the reasons this warning appears and how to diagnose it.

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2020
Prior to this commit a message could be seen in step logs
when tekton failed to copy a credential from /tekton/creds
to /tekton/home or $HOME (if disable-home-override feature
flag is "true"). That message can be confusing because it
appears to be an error when in actual fact it's a warning.

The message appears when Steps inside a Task run with
varying UIDs - one UID may copy the credentials first
and then subsequent steps will attempt to copy over them.
If a Step running as root, for examples, copies creds
into /tekton/home, other non-root Steps will not be able
to utilize those credentials.

The message can also appear if a user has explicltly
mounted credentials (via workspace or volumemount) in
/tekton/home and has _also_ attached a service account
to the task with creds-init credentials on it.

The message serves as an indicator of a potential problem but
not 100% guarantee that any issues in a Step are related to it.

This commit updates the message to include a "warning:"
prefix to indicate that it might (though not always)
be a potential source of issues in your Step.

This commit also adds documented to docs/auth.md to help
users diagnose the impact of this message (including when
they can safely ignore it).
@ghost ghost added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. labels Oct 16, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link
Collaborator

[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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2020
@dlorenc
Copy link
Contributor

dlorenc commented Oct 19, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2020
@vdemeester
Copy link
Member

/retest

@tekton-robot tekton-robot merged commit 330c356 into tektoncd:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants