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.

Sign up for GitHub

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

Jump to bottom

daemon: Bind /run/secrets/ in so we can see SA tokens #370

Merged
merged 1 commit into from
Feb 4, 2019
Merged

daemon: Bind /run/secrets/ in so we can see SA tokens #370

merged 1 commit into from
Feb 4, 2019

Conversation

cgwalters
Copy link
Member

Because we chroot() we need to ensure that we can still see
the service account tokens injected into our pod.

Closes: #358

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 2, 2019
@cgwalters
Copy link
Member Author

(Not tested locally yet, just tossing this up as I was thinking about a fix)

runcom
runcom reviewed Feb 2, 2019
View reviewed changes
pkg/daemon/daemon.go Outdated
// BindPodMounts ensures that the daemon can still see e.g. /run/secrets/kubernetes.io
// service account tokens after chrooting. This function must be called before chroot.
func (dn *Daemon) BindPodMounts() error {
targetSecrets := dn.rootMount + "/run/secrets"
Copy link
Member

Choose a reason for hiding this comment

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

nit: filepath.Join

@runcom
Copy link
Member

runcom commented Feb 2, 2019 •
edited

Just a utopic point here, cause of the complexity maybe, but is there any way we can add a test for this to make sure we won't regress in the future? (my understanding is also that this was working before). I'm seeing the errors everywhere on my clusters as well though.

@cgwalters
Copy link
Member Author

This appears to not have worked, I still see the error in the mcd logs in this PR.

but is there any way we can add a test for this to make sure we won't regress in the future?

Yeah...I think we could scrape the MCD logs in make test-e2e.

(my understanding is also that this was working before)

Not sure...but I doubt it, I suspect the rebase added an error message? Nothing in how the MCD works here changed recently.

@cgwalters cgwalters changed the title daemon: Bind /run/secrets/ in so we can see SA tokens WIP: daemon: Bind /run/secrets/ in so we can see SA tokens Feb 3, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 3, 2019
@jlebon
Copy link
Member

jlebon commented Feb 4, 2019

Yeah, I think that makes sense. Actually, given that we mount the whole rootfs and chroot, we're not really making use of the other mountpoints defined in the daemonset either. We should probably document that.

Related: #6

@jlebon
Copy link
Member

jlebon commented Feb 4, 2019

#377

@cgwalters
daemon: Bind /run/secrets/ in so we can see SA tokens
2cef241 
Because we `chroot()` we need to ensure that we can still see
the service account tokens injected into our pod.

Closes: openshift#358
@cgwalters
Copy link
Member Author

OK, figured it out I think - we needed --rbind since the secrets are also mount points.

@cgwalters cgwalters changed the title WIP: daemon: Bind /run/secrets/ in so we can see SA tokens daemon: Bind /run/secrets/ in so we can see SA tokens Feb 4, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019
@jlebon
Copy link
Member

jlebon commented Feb 4, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2019
@cgwalters
Copy link
Member Author 

$ oc logs -c test pods/e2e-aws-op
clusterversion.config.openshift.io/version condition met
go test -timeout 20m -v${WHAT:+ -run="$WHAT"} ./test/e2e/
=== RUN   TestMCDToken
--- PASS: TestMCDToken (2.24s)
=== RUN   TestOperatorLabel
--- PASS: TestOperatorLabel (0.04s)
PASS
ok      github.com/openshift/machine-config-operator/test/e2e   2.298s

🎊

ashcrow
ashcrow approved these changes Feb 4, 2019
View reviewed changes
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, jlebon

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 [ashcrow,cgwalters,jlebon]

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

yuqi-zhang
yuqi-zhang approved these changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

/lgtm
Confirm fixes: #358

@ashcrow
Copy link
Member

ashcrow commented Feb 4, 2019 

level=fatal msg="failed to initialize the cluster: timed out waiting for the condition"

/retest

@openshift-merge-robot openshift-merge-robot merged commit 80407c7 into openshift:master Feb 4, 2019
@cgwalters cgwalters mentioned this pull request Mar 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@runcom runcom runcom left review comments

@ashcrow ashcrow ashcrow approved these changes

@yuqi-zhang yuqi-zhang yuqi-zhang approved these changes

@jlebon jlebon Awaiting requested review from jlebon

Assignees

@ashcrow ashcrow

@jlebon jlebon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@cgwalters @runcom @jlebon @openshift-ci-robot @ashcrow @yuqi-zhang @openshift-merge-robot