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

Rollback creds-init removal in 0.14 branch #2952

Merged
merged 1 commit into from Jul 16, 2020
Merged

Rollback creds-init removal in 0.14 branch #2952

merged 1 commit into from Jul 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jul 16, 2020

Changes

In #2671 I removed the creds-init initContainer from Task Pods so that credentials could be used by containers running with non-root users. The intention was for this change to be free of any side-effects to end-users. Unfortunately a backwards incompatible issue has cropped up with this change:

When a user does not specify the known_hosts field in a creds-init Secret, the credential code will perform an ssh-keyscan of the remote server to get its public key. The problem is that previously we could guarantee ssh-keyscan was available since the code ran in our own creds-init container with our own docker image. Since we've now moved that code into Steps' entrypoint a Step's container is required to provide ssh-keyscan. This is a change in container contract and therefore backwards-incompatible.

In this PR I've reverted the creds-init change for the 0.14 branch rather than attempt to fix the ssh-keyscan issue and possibly introduce more problems.

Before 0.15 I'd like to get a better, backwards-compatible, fix organized. So I plan to leave the creds-init change in place in the master branch for the time being.

Submitter Checklist

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

Release Notes

The removal of the creds-init initContainer that occurred in 0.14.0 has been rolled back. The change introduced an unintended backwards-incompatibility that needs to be resolved.

This reverts commit bbb767c.

In #2671 I removed the creds-init
initContainer from Task Pods so that credentials could be used by containers
running with non-root users.  The intention was for this change to be free
of any side-effects to end-users.  Unfortunately a
[backwards incompatible issue](#2951)
has cropped up with this change:

When a user does not specify the `known_hosts` field in a creds-init Secret,
the credential code will perform an `ssh-keyscan` of the remote server to get
its public key.  The problem is that previously we could guarantee `ssh-keyscan`
was available since the code ran in our own creds-init container with our own
docker image. Since we've now moved that code into Steps' entrypoint the Steps
container is required to provide `ssh-keyscan`.  This is a change in container
contract and therefore backwards-incompatible.

In this PR I've reverted the creds-init change for the 0.14 branch rather than
attempt to fix the `ssh-keyscan` issue and possibly introduce more problems.

Before 0.15 I'd like to get a better backwards-compatible fix organized.
So I plan to leave the creds-init change in place in the `master` branch for
the time being.
@ghost ghost requested a review from vdemeester July 16, 2020 16:04
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 16, 2020
@ghost ghost added the kind/bug Categorizes issue or PR as related to a bug. label Jul 16, 2020
@tekton-robot
Copy link
Collaborator

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 93.1% 84.2% -8.9
pkg/pod/entrypoint.go 84.4% 84.1% -0.2
pkg/pod/pod.go 87.3% 84.5% -2.7
pkg/reconciler/taskrun/taskrun.go 76.5% 76.3% -0.3

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks!
/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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 Jul 16, 2020
@afrittoli
Copy link
Member

So, if I got it right, the plan is rollback on the branch only, and fix on master for v0.15 instead, right?

@vdemeester
Copy link
Member

That's my guess too, @sbwsg are we right ?

@ghost
Copy link
Author

ghost commented Jul 16, 2020

So, if I got it right, the plan is rollback on the branch only, and fix on master for v0.15 instead, right?

Yup, exactly right. I'm working on a fix now that should hopefully be ready in time for 0.15.

@vdemeester
Copy link
Member

So, if I got it right, the plan is rollback on the branch only, and fix on master for v0.15 instead, right?

Yup, exactly right. I'm working on a fix now that should hopefully be ready in time for 0.15.

Let's add the refered issue in the milestone then 👼

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@tekton-robot tekton-robot merged commit 9168151 into tektoncd:release-v0.14.x Jul 16, 2020
@ghost
Copy link
Author

ghost commented Jul 16, 2020

Let's add the refered issue in the milestone then 👼

Good thinking, thanks for doing that!

@ghost ghost added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jul 16, 2020
@vdemeester vdemeester removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jul 27, 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/bug Categorizes issue or PR as related to a bug. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants