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

🐛 Envtest: Allow creating objects with privileged container specs #708

Merged

Conversation

alvaroaleman
Copy link
Member

@alvaroaleman alvaroaleman commented Dec 3, 2019

Currently it is not possible to use envtest to create things with privileged containers in them, because the --allow-privileged flag on the kube-apiserver defaults to false. Attempting to do so causes a Deployment.apps \"deployment-name\" is invalid: spec.template.spec.containers[0].securityContext.privileged: Forbidden: disallowed by cluster policy".

This PR changes that.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 3, 2019
@alvaroaleman
Copy link
Member Author

/assign @DirectXMan12

@DirectXMan12
Copy link
Contributor

Not to nitpick this too much, but is there a compelling reason to make this default vs just overriding it when you need it by adding to the API server flags?

@alvaroaleman
Copy link
Member Author

Not to nitpick this too much, but is there a compelling reason to make this default vs just overriding it when you need it by adding to the API server flags?

Yeah, thats how I got my tests going :) Mostly adding this to share, before I ran into this I was not aware that such a flag exists on the kube-apisever and I am reasonable sure I have never worked with a cluster that kept it at the default.

@DirectXMan12
Copy link
Contributor

ah, ok. Perhaps we should add this to FAQs? Not that it makes much different having it here or not because you can't actually run pods with envtest ;-)

@alvaroaleman
Copy link
Member Author

ah, ok. Perhaps we should add this to FAQs? Not that it makes much different having it here or not because you can't actually run pods with envtest ;-)

It seems simpler to me to just add it by default, because it is a de-facto default and required for anything that uses it as part of a PodSpecTemplate, like Deployments, Statefulsets and Daemonsets.

A common use-case for it is e.G. CNI or CSI.
I disagree, I think it does make a difference because this doesn't just prevent privileged pod creation, it already prevents creation of anything that has PodSpecTemplate

@DirectXMan12
Copy link
Contributor

Ack, ok. I'm editing the title to avoid freaking anyone out, but that seems like a reasonable argument

@DirectXMan12 DirectXMan12 changed the title 🐛 Envtest: Allow creating privileged containers 🐛 Envtest: Allow creating objects with privileged container specs Dec 9, 2019
@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 9, 2019
@DirectXMan12
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019
@DirectXMan12
Copy link
Contributor

whoops. Can you get rid of boolptr? I hate that library an unhealthy amount.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2019
@alvaroaleman
Copy link
Member Author

whoops. Can you get rid of boolptr? I hate that library an unhealthy amount.

Done. What makes you prefer func myTypePtr() everywhere if I may ask?

@DirectXMan12
Copy link
Contributor

oh, I usually just do

definitelyTrue := true
...
thingThatNeedsABoolPointer: &definitelyTrue

or somesuch.

It's mostly that it's yet another dependency that will arbitrarily break in incompatible ways at some point in the future, and I really don't want to have to deal with incompatibilities while upgrading cause of a few calls to BoolPtr or something.

It's not actually as huge a deal as I make it sound, and half of my grumbling is probably because I really just want to write Some(true) and be done with it :-P

@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, DirectXMan12

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

@alvaroaleman
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019
@k8s-ci-robot k8s-ci-robot merged commit efbeb27 into kubernetes-sigs:master Dec 10, 2019
@alvaroaleman alvaroaleman deleted the envtest-allow-privileged branch December 10, 2019 06:53
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
fix a name changing issue and a missing scaffolding
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants