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

Make NET_ADMIN check a warning, add PSP check #2958

Merged
merged 1 commit into from
Jun 20, 2019
Merged

Conversation

siggy
Copy link
Member

@siggy siggy commented Jun 18, 2019

linkerd check validates whether PSP's exist, and if the caller has the
NET_ADMIN capability. This check was previously failing if NET_ADMIN
was not found, even in the case where the PSP admission controller was
not running. Related, linkerd install now includes a PSP, so
linkerd check should also validate that the caller can create PSP's.

Modify the NET_ADMIN check to warn, but not fail, if PSP's are found
but the caller does not have NET_ADMIN. Update the warning message to
mention that this is only a problem if the PSP admission controller is
running (and will only be a problem during injection, since #2920
handles control plane installation by adding its own PSP).

Also introduce a check to validate the caller can create PSP's.

Fixes #2884, #2849

Signed-off-by: Andrew Seigner [email protected]

@siggy siggy requested review from alpeb and ihcsim June 18, 2019 08:24
@siggy siggy self-assigned this Jun 18, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Jun 18, 2019

Integration test results for 4065ab8: fail 😕
Log output: https://gist.github.com/ec038226ee569631f3d0b53ee71c4584

@siggy siggy force-pushed the siggy/check-psp branch from 4065ab8 to d30e90d Compare June 18, 2019 11:30
@l5d-bot
Copy link
Collaborator

l5d-bot commented Jun 18, 2019

Integration test results for d30e90d: success 🎉
Log output: https://gist.github.com/63d530ceadfca18a1eed99eef6afc518

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍 🚢

@grampelberg
Copy link
Contributor

With the control plane PSP, NET_ADMIN isn't required for install but it is still required for workloads right? Maybe there should be a check in inject?

@grampelberg
Copy link
Contributor

Another idea, as we're moving towards auto-inject being the one true way (tm), and auto-inject can return errors. What about checking to see if the attached service account has a PSP that includes NET_ADMIN and returning an error if it doesn't (assuming CNI isn't there).

This all feels like 100% a separate issue and shouldn't block the merge of this specific PR as most of my confusion seems to have stemmed from me making an invalid assumption about what the check was actually validating.

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Also +1 to @grampelberg suggestion on updating the proxy injector in a separate PR to account for service account PSP + RBAC (using the can-i API if there is one?). This PR only ensures that whoever/whatever is running linkerd check --pre has permission to create PSP. The workload's service account will also need to have the same permission to start new pods.

`linkerd check` validates whether PSP's exist, and if the caller has the
`NET_ADMIN` capability. This check was previously failing if `NET_ADMIN`
was not found, even in the case where the PSP admission controller was
not running. Related, `linkerd install` now includes a PSP, so
`linkerd check` should also validate that the caller can create PSP's.

Modify the `NET_ADMIN` check to warn, but not fail, if PSP's are found
but the caller does not have `NET_ADMIN`. Update the warning message to
mention that this is only a problem if the PSP admission controller is
running (and will only be a problem during injection, since #2920
handles control plane installation by adding its own PSP).

Also introduce a check to validate the caller can create PSP's.

Fixes #2884, #2849

Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy force-pushed the siggy/check-psp branch from d30e90d to 19407e9 Compare June 19, 2019 13:04
@siggy siggy changed the title Remove NET_ADMIN check, introduce PSP check Make NET_ADMIN check a warning, add PSP check Jun 19, 2019
@siggy
Copy link
Member Author

siggy commented Jun 19, 2019

@grampelberg @ihcsim Thanks for the feedback re: proxy injector checking for PSP, I've filed #2968 to track.

I've put the NET_ADMIN check back into this PR, so the overall diff is now smaller. Since NET_ADMIN is still required for injection, I thought it prudent to continue validating this in linkerd check. To solve #2884 and #2849 specifically, I've changed the NET_ADMIN check into a warning, to inform the user that there may be a problem if the PSP admission controller is running (if it's not running, it's a non-issue).

@ihcsim
Copy link
Contributor

ihcsim commented Jun 19, 2019

I'm not sure if it's necessary to check for NET_ADMIN during linkerd check --pre, since linkerd install and linkerd install config will create the control plane PSP with that capability. I wonder if adding back this check will lead users to think that they must have a PSP with NET_ADMIN prior to installation.

Also, did your change leave out the CNI-enabled condition for this check?

@siggy
Copy link
Member Author

siggy commented Jun 20, 2019

@ihcsim You're right that linkerd install and linkerd install config will work regardless of whether the NET_ADMIN check passes, since the control plane creates a PSP anyway. I left the NET_ADMIN check in place because that capability is still required for proxy injection. To account for this, I've changed the NET_ADMIN check from an error to a warning (linkerd check still succeeds), and updated the message to read:

pre-kubernetes-capability
-------------------------
‼ has NET_ADMIN capability
    found 1 PodSecurityPolicies, but none provide NET_ADMIN, proxy injection will fail if the PSP admission controller is running
    see https://linkerd.io/checks/#pre-k8s-cluster-net-admin for hints

...
Status check results are √

Re: the cni check, I've left the existing behavior, where this command skips the NET_ADMIN check:

linkerd check --linkerd-cni-enabled --pre

@siggy siggy merged commit 2528e3d into master Jun 20, 2019
@siggy siggy deleted the siggy/check-psp branch June 20, 2019 15:58
@l5d-bot
Copy link
Collaborator

l5d-bot commented Jun 20, 2019

Integration test results for 19407e9: fail 😕
Log output: https://gist.github.com/42f511c3a793e45fab86f841f80894b6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NET_ADMIN capability issue with linkerd --pre check
5 participants