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

Add all securityContext fields in injected containers #17427

Merged
merged 2 commits into from Dec 28, 2019

Conversation

@rlenglet
Copy link
Member

rlenglet commented Sep 26, 2019

Fixes #17318

[x] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

@rlenglet rlenglet requested a review from istio/wg-environments-maintainers as a code owner Sep 26, 2019
@googlebot googlebot added the cla: yes label Sep 26, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Sep 26, 2019

/test pilot-e2e-envoyv2-v1alpha3

@rlenglet rlenglet changed the title Add all securityContext fields in injected containers [WIP] Add all securityContext fields in injected containers Sep 27, 2019
@rlenglet rlenglet force-pushed the add-all-security-context-fields branch from 21e1770 to 495e055 Sep 27, 2019
@rlenglet rlenglet requested review from geeknoid, howardjohn, linsun, rshriram and istio/wg-user-experience-maintainers as code owners Sep 27, 2019
@istio-testing istio-testing added size/XXL and removed size/M labels Sep 27, 2019
@rlenglet rlenglet force-pushed the add-all-security-context-fields branch 2 times, most recently from f56a615 to 188e736 Sep 27, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Sep 27, 2019

/test lint

@jammerful

This comment has been minimized.

Copy link

jammerful commented Sep 30, 2019

@rlenglet I'm interested in seeing this PR merged, as right now istio doesn't work with Pod Security Policy even with the istio cni installed. Do you have a sense of when this might be merged and when this might make it into a release? Thanks.

@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Sep 30, 2019

@jammerful you can apply a similar change into your deployment's injection template. You don't have to wait for a new release to apply this change locally. I'm expecting to ship this in 1.3.3, around mid-October.

@jammerful

This comment has been minimized.

Copy link

jammerful commented Sep 30, 2019

@rlenglet I was trying to avoid having to change the config map for the side car injector, though I will test that this changed does allow my pod security policy to be used.
On a side note, I stumbled upon re-invocation of mutating admission controllers, is it correct if the pod security policy's mutating admission controller is marked a reinvocationPolicy: "IfNeeded" the istio side car injector's and the pod security policy's mutating admission controllers will work together allowing, technically not requiring setting the fields in this PR explicitly? That being said I still like the fields being set explicitly as it is deterministic.

@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Sep 30, 2019

On a side note, I stumbled upon re-invocation of mutating admission controllers, is it correct if the pod security policy's mutating admission controller is marked a reinvocationPolicy: "IfNeeded" the istio side car injector's and the pod security policy's mutating admission controllers will work together allowing, technically not requiring setting the fields in this PR explicitly?

That is very interesting, thanks. However, users may have other admission controllers that could trigger a re-invocation, so I think we can't avoid setting the fields.

@jammerful

This comment has been minimized.

Copy link

jammerful commented Oct 1, 2019

That is very interesting, thanks. However, users may have other admission controllers that could trigger a re-invocation, so I think we can't avoid setting the fields.

Yup this is still needed, as I noted this ensures that istio always runs with the minimal set of permissions (principle of least privilege) no matter what the users kubernetes configuration is (that is what I meant by "deterministic"). In any case, please let me know if you need any help with this, I would be happy to help out to get this over the line.

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Oct 6, 2019

lgtm, mention me once its not WIP if you need an approval

@ericvn

This comment has been minimized.

Copy link
Contributor

ericvn commented Oct 19, 2019

Note that this PR is missing the cherrypick label for 1.4. I assume it should be added there as well.

@thecodejunkie

This comment has been minimized.

Copy link

thecodejunkie commented Oct 28, 2019

Running into this issue as well, in a PSP enabled cluster with automatic sidecar injection. Unable to validate against any PSP -> ReplicaSet fails to create pods. Will this be fixed for 1.4?

@rlenglet rlenglet force-pushed the add-all-security-context-fields branch 2 times, most recently from bdb1c97 to 507d556 Dec 28, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Dec 28, 2019

/test e2e-dashboard_istio

@rlenglet rlenglet changed the title [WIP] Add all securityContext fields in injected containers Add all securityContext fields in injected containers Dec 28, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Dec 28, 2019

The automated tests are passing.
And I manually tested that this PR allows running pods with automatic injection and Istio CNI enabled with a very restrictive PodSecurityPolicy.

@rlenglet rlenglet removed the request for review from geeknoid Dec 28, 2019
@rlenglet rlenglet changed the title Add all securityContext fields in injected containers [WIP] Add all securityContext fields in injected containers Dec 28, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Dec 28, 2019

I tested that this simpler capabilities combination works:

        securityContext:
          capabilities:
            add:
            - NET_ADMIN
            - NET_RAW
            drop:
            - ALL

I will update the PR.

@rlenglet rlenglet force-pushed the add-all-security-context-fields branch from 6c3fff9 to 2b9638d Dec 28, 2019
@rlenglet rlenglet changed the title [WIP] Add all securityContext fields in injected containers Add all securityContext fields in injected containers Dec 28, 2019
@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Dec 28, 2019

@howardjohn @istio/wg-environments-maintainers @istio/wg-user-experience-maintainers This is ready for review. I am still working on the matching istio/installer PR. Thanks!

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 28, 2019

@rlenglet: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
e2e-bookInfoTests-trustdomain_istio 188e736 link /test e2e-bookInfoTests-trustdomain

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. I understand the commands that are listed here.

@rlenglet

This comment has been minimized.

Copy link
Member Author

rlenglet commented Dec 28, 2019

/test pilot-multicluster-e2e_istio

@istio-testing istio-testing merged commit 99c0ec6 into master Dec 28, 2019
25 of 26 checks passed
25 of 26 checks passed
tide Not mergeable.
Details
cla/google All necessary CLAs are signed
e2e-bookInfoTests-envoyv2-v1alpha3_istio Job succeeded.
Details
e2e-dashboard_istio Job succeeded.
Details
e2e-mixer-no_auth_istio Job succeeded.
Details
e2e-simpleTests-cni_istio Job succeeded.
Details
e2e-simpleTests-distroless_istio Job succeeded.
Details
e2e-simpleTestsMinProfile_istio Job succeeded.
Details
e2e-simpleTests_istio Job succeeded.
Details
gencheck_istio Job succeeded.
Details
integ-galley-k8s-tests_istio Job succeeded.
Details
integ-galley-local-tests_istio Job succeeded.
Details
integ-istioio-k8s-tests_istio Job succeeded.
Details
integ-mixer-k8s-tests_istio Job succeeded.
Details
integ-new-install-k8s-tests_istio Job succeeded.
Details
integ-pilot-k8s-tests_istio Job succeeded.
Details
integ-pilot-local-tests_istio Job succeeded.
Details
integ-security-k8s-tests_istio Job succeeded.
Details
integ-security-local-tests_istio Job succeeded.
Details
integ-telemetry-k8s-tests_istio Job succeeded.
Details
istio_e2e_cloudfoundry_istio Job succeeded.
Details
lint_istio Job succeeded.
Details
pilot-e2e-envoyv2-v1alpha3_istio Job succeeded.
Details
pilot-multicluster-e2e_istio Job succeeded.
Details
release-test_istio Job succeeded.
Details
unit-tests_istio Job succeeded.
Details
@istio-testing istio-testing deleted the add-all-security-context-fields branch Dec 28, 2019
@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 28, 2019

In response to a cherrypick label: #17427 failed to apply on top of branch "release-1.3":

Applying: Update injection unit tests
Using index info to reconstruct a base tree...
A	pkg/kube/inject/testdata/inject/app_probe/hello-probes-with-flag-set-in-annotation.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/hello-probes-with-flag-unset-in-annotation.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/hello-probes.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/hello-readiness.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/https-probes.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/named_port.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/one_container.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/ready_live.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/ready_only.yaml.injected
A	pkg/kube/inject/testdata/inject/app_probe/two_container.yaml.injected
A	pkg/kube/inject/testdata/inject/auth.cert-dir.yaml.injected
A	pkg/kube/inject/testdata/inject/auth.non-default-service-account.yaml.injected
A	pkg/kube/inject/testdata/inject/auth.yaml.injected
A	pkg/kube/inject/testdata/inject/cronjob-with-app.yaml.injected
A	pkg/kube/inject/testdata/inject/cronjob.yaml.injected
A	pkg/kube/inject/testdata/inject/daemonset.yaml.injected
A	pkg/kube/inject/testdata/inject/deploymentconfig-multi.yaml.injected
A	pkg/kube/inject/testdata/inject/deploymentconfig.yaml.injected
A	pkg/kube/inject/testdata/inject/enable-core-dump.yaml.injected
A	pkg/kube/inject/testdata/inject/format-duration.yaml.injected
A	pkg/kube/inject/testdata/inject/frontend.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-always.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-config-map-name.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-ignore.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-mtls-not-ready.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-multi.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-namespace.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-never.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-proxy-override.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-template-in-values.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-tproxy-debug.yaml.injected
A	pkg/kube/inject/testdata/inject/hello-tproxy.yaml.injected
A	pkg/kube/inject/testdata/inject/hello.yaml.injected
A	pkg/kube/inject/testdata/inject/job.yaml.injected
A	pkg/kube/inject/testdata/inject/kubevirtInterfaces.yaml.injected
A	pkg/kube/inject/testdata/inject/kubevirtInterfaces_list.yaml.injected
A	pkg/kube/inject/testdata/inject/list-frontend.yaml.injected
A	pkg/kube/inject/testdata/inject/list.yaml.injected
A	pkg/kube/inject/testdata/inject/multi-container.yaml.injected
A	pkg/kube/inject/testdata/inject/multi-init.yaml.injected
A	pkg/kube/inject/testdata/inject/pod-with-app.yaml.injected
A	pkg/kube/inject/testdata/inject/pod.yaml.injected
A	pkg/kube/inject/testdata/inject/replicaset.yaml.injected
A	pkg/kube/inject/testdata/inject/replicationcontroller.yaml.injected
A	pkg/kube/inject/testdata/inject/statefulset.yaml.injected
A	pkg/kube/inject/testdata/inject/status_annotations.yaml.injected
A	pkg/kube/inject/testdata/inject/status_params.yaml.injected
A	pkg/kube/inject/testdata/inject/traffic-annotations-empty-includes.yaml.injected
A	pkg/kube/inject/testdata/inject/traffic-annotations-wildcards.yaml.injected
A	pkg/kube/inject/testdata/inject/traffic-annotations.yaml.injected
A	pkg/kube/inject/testdata/inject/traffic-params-empty-includes.yaml.injected
A	pkg/kube/inject/testdata/inject/traffic-params.yaml.injected
A	pkg/kube/inject/testdata/webhook/daemonset.yaml.injected
A	pkg/kube/inject/testdata/webhook/deploymentconfig-multi.yaml.injected
A	pkg/kube/inject/testdata/webhook/deploymentconfig.yaml.injected
A	pkg/kube/inject/testdata/webhook/frontend.yaml.injected
A	pkg/kube/inject/testdata/webhook/hello-config-map-name.yaml.injected
A	pkg/kube/inject/testdata/webhook/hello-mtls-not-ready.yaml.injected
A	pkg/kube/inject/testdata/webhook/hello-multi.yaml.injected
A	pkg/kube/inject/testdata/webhook/hello-probes.yaml.injected
A	pkg/kube/inject/testdata/webhook/job.yaml.injected
A	pkg/kube/inject/testdata/webhook/list-frontend.yaml.injected
A	pkg/kube/inject/testdata/webhook/list.yaml.injected
A	pkg/kube/inject/testdata/webhook/multi-init.yaml.injected
A	pkg/kube/inject/testdata/webhook/replicaset.yaml.injected
A	pkg/kube/inject/testdata/webhook/replicationcontroller.yaml.injected
A	pkg/kube/inject/testdata/webhook/resource_annotations.yaml.injected
A	pkg/kube/inject/testdata/webhook/statefulset.yaml.injected
A	pkg/kube/inject/testdata/webhook/status_annotations.yaml.injected
A	pkg/kube/inject/testdata/webhook/traffic-annotations-empty-includes.yaml.injected
A	pkg/kube/inject/testdata/webhook/traffic-annotations-wildcards.yaml.injected
A	pkg/kube/inject/testdata/webhook/traffic-annotations.yaml.injected
A	pkg/kube/inject/testdata/webhook/user-volume.yaml.injected
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/kube/inject/testdata/webhook/hello-mtls-not-ready.yaml.injected deleted in HEAD and modified in Update injection unit tests. Version Update injection unit tests of pkg/kube/inject/testdata/webhook/hello-mtls-not-ready.yaml.injected left in tree.
CONFLICT (modify/delete): pkg/kube/inject/testdata/inject/hello-mtls-not-ready.yaml.injected deleted in HEAD and modified in Update injection unit tests. Version Update injection unit tests of pkg/kube/inject/testdata/inject/hello-mtls-not-ready.yaml.injected left in tree.
Auto-merging pilot/pkg/kube/inject/testdata/webhook/user-volume.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/traffic-annotations.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/traffic-annotations-wildcards.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/traffic-annotations-empty-includes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/status_annotations.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/statefulset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/resource_annotations.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/replicationcontroller.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/replicaset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/multi-init.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/list.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/list-frontend.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/job.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/hello-probes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/hello-multi.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/hello-config-map-name.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/frontend.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/deploymentconfig.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/deploymentconfig-multi.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/webhook/daemonset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/traffic-params.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/traffic-params-empty-includes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/traffic-annotations.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/traffic-annotations-wildcards.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/traffic-annotations-empty-includes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/status_params.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/status_annotations.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/statefulset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/replicationcontroller.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/replicaset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/pod.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/pod-with-app.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/multi-init.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/multi-container.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/list.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/list-frontend.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/kubevirtInterfaces_list.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/kubevirtInterfaces.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/job.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-tproxy.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-tproxy-debug.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-template-in-values.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-proxy-override.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-never.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-namespace.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-multi.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-ignore.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-config-map-name.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/hello-always.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/frontend.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/format-duration.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/enable-core-dump.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/deploymentconfig.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/deploymentconfig-multi.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/daemonset.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/cronjob.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/cronjob-with-app.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/auth.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/auth.non-default-service-account.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/auth.cert-dir.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/two_container.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/ready_only.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/ready_live.yaml.injected
CONFLICT (content): Merge conflict in pilot/pkg/kube/inject/testdata/inject/app_probe/ready_live.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/one_container.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/named_port.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/https-probes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/hello-readiness.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/hello-probes.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/hello-probes-with-flag-unset-in-annotation.yaml.injected
Auto-merging pilot/pkg/kube/inject/testdata/inject/app_probe/hello-probes-with-flag-set-in-annotation.yaml.injected
error: Failed to merge in the changes.
Patch failed at 0002 Update injection unit tests

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 28, 2019

In response to a cherrypick label: new pull request created: #19832

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.