Remove the need of anyuid permission on OpenShift #45394

jwendell · 2023-06-09T22:07:41Z

Remove the hard-coded usages of runAsUser and runAsGroup in charts
Where necessary, replace them with a helm field .ProxyUID and .ProxyGID.
When running in OpenShift, these fields will be set at runtime based on annotations present on the pod's namespace.
When not on OpenShift (strictly speaking, when such annotations are not present), these values fallback to the current, defauls value of 1337.

With this change we can remove the extra steps related to anyuid in the OpenShift setup page.

jacob-delgado

LGTM, but see my concern about adding the kubeclient to the webhook. I'll defer that to the networking maintainers. This would definitely help users of OpenShift so I'm in favor.

pkg/kube/inject/openshift.go

pkg/kube/inject/webhook.go

go.mod

manifests/charts/istio-control/istio-discovery/templates/deployment.yaml

manifests/charts/istio-operator/templates/deployment.yaml

pilot/docker/Dockerfile.proxyv2

pkg/kube/client.go

pkg/kube/inject/inject.go

pkg/kube/inject/webhook.go

howardjohn · 2023-06-12T16:36:05Z

I am a bit worried these changes may have some subtle negative impact on non-openshift users. This is a bit vague since I don't have any concrete criticisms, more of the unknown-unknown of the template changes

howardjohn · 2023-06-12T16:38:22Z

Does openshift block the UIDs at Deployment level or only Pod level?

jwendell · 2023-06-12T17:22:30Z

I am a bit worried these changes may have some subtle negative impact on non-openshift users. This is a bit vague since I don't have any concrete criticisms, more of the unknown-unknown of the template changes

The general idea is to remove hardcoded values in RunAs fields in the deployments. If this field is absent, Kubernetes will honor the USER value of the container image. OpenShift will automatically assign a value derived from an range defined in a namespace annotation.

If we change the USER field in the base proxy image to be 1337 - which we did in this PR - we can remove the RunAs declarations in some places. Kubernetes will use 1337 and OpenShift will assign a value at runtime.

jwendell · 2023-06-12T17:22:48Z

Does openshift block the UIDs at Deployment level or only Pod level?

Replicaset level.

howardjohn · 2023-06-15T22:35:42Z

I am a bit worried these changes may have some subtle negative impact on non-openshift users. This is a bit vague since I don't have any concrete criticisms, more of the unknown-unknown of the template changes

The general idea is to remove hardcoded values in RunAs fields in the deployments. If this field is absent, Kubernetes will honor the USER value of the container image. OpenShift will automatically assign a value derived from an range defined in a namespace annotation.

If we change the USER field in the base proxy image to be 1337 - which we did in this PR - we can remove the RunAs declarations in some places. Kubernetes will use 1337 and OpenShift will assign a value at runtime.

The issue is if users are currently running the images without explicitly setting runas, they are now changed from 0 to 1337. See #45394 (comment) - I am 100% certain this will break some of our users.

To be fair - its a good end result, we don't want users running as root. But I am a bit worried about the breakage.

Does openshift even need it? Can we just keep 1337 hardcoded as-is, and then override with the max UID range on openshift as added in this PR?

manifests/charts/gateways/istio-ingress/templates/deployment.yaml

pilot/docker/Dockerfile.proxyv2

- Remove the hard-coded usages of `runAsUser` and `runAsGroup` in charts - Where necessary, replace them with a helm field `.ProxyUID` and `.ProxyGID`. - When running in OpenShift, these fields will be set at runtime based on annotations present on the pod's namespace. - When not on OpenShift (strictly speaking, when such annotations are not present), these values fallback to the current, defauls value of 1337. With this change we can remove the extra steps related to `anyuid` in the [OpenShift setup page](https://istio.io/v1.17/docs/setup/platform-setup/openshift/).

jwendell · 2023-08-25T14:39:08Z

I reverted the + USER 1337:1337 change I had done in proxy dockerfile. Hopefully changes now are safe.

howardjohn · 2023-08-25T15:21:28Z

manifests/charts/gateways/istio-egress/templates/deployment.yaml

@@ -53,10 +53,11 @@ spec:
    spec:
 {{- if not $gateway.runAsRoot }}
      securityContext:
+{{- if not (eq .Values.global.platform "openshift") }}


Its slightly awkward/inconsistent that we have an "openshift" profile and an "openshift" platform, and some values are controlled by one vs the other.

But I think ok since otherwise we would need to add some more values here

howardjohn · 2023-08-25T15:26:54Z

@jwendell you have a do-not-merge, LGTM to merge when you are ready

kfaseela · 2023-08-30T08:23:15Z

@jwendell : I am trying to run integration tests locally on my mac, and starts getting the below error today. This used to work before. Do you think it is related?

2023-08-30T08:16:27.591923Z	error	Pod injection failed: failed to run injection template: template: inject:88:9: executing "inject" at <.ProxyUID>: can't evaluate field ProxyUID in type *inject.SidecarTemplateData
2023-08-30T08:16:27.996153Z	info	Sidecar injection request for echo1-5-51122/a-v1-7868db5759-***** (actual name not yet known)
2023-08-30T08:16:27.998146Z	error	Invalid template: template: inject:88:9: executing "inject" at <.ProxyUID>: can't evaluate field ProxyUID in type *inject.SidecarTemplateData

kfaseela · 2023-08-30T09:16:21Z

@jwendell : I am trying to run integration tests locally on my mac, and starts getting the below error today. This used to work before. Do you think it is related?

2023-08-30T08:16:27.591923Z	error	Pod injection failed: failed to run injection template: template: inject:88:9: executing "inject" at <.ProxyUID>: can't evaluate field ProxyUID in type *inject.SidecarTemplateData
2023-08-30T08:16:27.996153Z	info	Sidecar injection request for echo1-5-51122/a-v1-7868db5759-***** (actual name not yet known)
2023-08-30T08:16:27.998146Z	error	Invalid template: template: inject:88:9: executing "inject" at <.ProxyUID>: can't evaluate field ProxyUID in type *inject.SidecarTemplateData

ignore, things are working fine when I use latest test image.

These instructions are no longer needed after these changes istio/istio#45394

* Remove anyuid instructions for OpenShift These instructions are no longer needed after these changes istio/istio#45394 * Removed blank line that caused failure

…e lines it attempts to remove have been removed as part of istio/istio#45394. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>

This patch is not needed anymore as we removed support for v1.20 - the lines it attempts to remove have been removed as part of istio/istio#45394. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>

- Remove the hard-coded usages of `runAsUser` and `runAsGroup` in charts - Where necessary, replace them with a helm field `.ProxyUID` and `.ProxyGID`. - When running in OpenShift, these fields will be set at runtime based on annotations present on the pod's namespace. - When not on OpenShift (strictly speaking, when such annotations are not present), these values fallback to the current, defauls value of 1337. With this change we can remove the extra steps related to `anyuid` in the [OpenShift setup page](https://istio.io/v1.17/docs/setup/platform-setup/openshift/).

…f-anyuid-openshift Remove the need of anyuid permission on OpenShift (istio#45394)

jwendell requested review from a team, costinm, linsun, howardjohn, stevenctl, ericvn and hzxuzhonghu as code owners June 9, 2023 22:07

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 9, 2023

jacob-delgado reviewed Jun 9, 2023

View reviewed changes

pkg/kube/inject/openshift.go Show resolved Hide resolved

pkg/kube/inject/webhook.go Outdated Show resolved Hide resolved

howardjohn reviewed Jun 12, 2023

View reviewed changes

jwendell force-pushed the remove-anyuid branch from dd59479 to de1c529 Compare June 15, 2023 18:29

jwendell added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 15, 2023

howardjohn reviewed Jul 11, 2023

View reviewed changes

manifests/charts/gateways/istio-ingress/templates/deployment.yaml Show resolved Hide resolved

pilot/docker/Dockerfile.proxyv2 Outdated Show resolved Hide resolved

taneem-ibrahim mentioned this pull request Jul 13, 2023

Enhance UID selection for storage-initializer container opendatahub-io/kserve#26

Closed

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged labels Jul 18, 2023

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Aug 11, 2023

jwendell force-pushed the remove-anyuid branch from de1c529 to 624864d Compare August 25, 2023 14:35

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 25, 2023

istio-policy-bot removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Aug 25, 2023

howardjohn approved these changes Aug 25, 2023

View reviewed changes

jwendell removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 25, 2023

istio-testing merged commit ef9d57e into istio:master Aug 25, 2023
16 of 27 checks passed

rh-tokeefe added a commit to rh-tokeefe/istio.io that referenced this pull request Aug 31, 2023

Remove anyuid instructions for OpenShift

a591cd8

These instructions are no longer needed after these changes istio/istio#45394

rh-tokeefe mentioned this pull request Aug 31, 2023

Remove anyuid instructions for OpenShift istio/istio.io#13795

Merged

jewertow mentioned this pull request Sep 4, 2023

manifests: fix setting proxy UID and GID in gateway templates on OpenShift #46835

Closed

hemendrateli mentioned this pull request Dec 14, 2023

Istio ingress not able to bind on port 80 #48364

Closed

2 tasks

dgn mentioned this pull request Apr 5, 2024

Remove the patch that deleted fixed UIDs from istiod charts istio-ecosystem/sail-operator#23

Merged

jwendell mentioned this pull request Jun 4, 2024

OSSM-5698 Rebase injection proxy UID/GID maistra/istio#1009

Closed

12 tasks

veer051 pushed a commit to veer051/istio that referenced this pull request Jul 5, 2024

Merge pull request istio#195 from jacob-delgado/cherry-pick-removal-o…

4843a2f

…f-anyuid-openshift Remove the need of anyuid permission on OpenShift (istio#45394)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the need of anyuid permission on OpenShift #45394

Remove the need of anyuid permission on OpenShift #45394

jwendell commented Jun 9, 2023

jacob-delgado left a comment

howardjohn commented Jun 12, 2023

howardjohn commented Jun 12, 2023

jwendell commented Jun 12, 2023

jwendell commented Jun 12, 2023

howardjohn commented Jun 15, 2023

jwendell commented Aug 25, 2023

howardjohn Aug 25, 2023

howardjohn commented Aug 25, 2023

kfaseela commented Aug 30, 2023

kfaseela commented Aug 30, 2023

Remove the need of anyuid permission on OpenShift #45394

Remove the need of anyuid permission on OpenShift #45394

Conversation

jwendell commented Jun 9, 2023

jacob-delgado left a comment

Choose a reason for hiding this comment

howardjohn commented Jun 12, 2023

howardjohn commented Jun 12, 2023

jwendell commented Jun 12, 2023

jwendell commented Jun 12, 2023

howardjohn commented Jun 15, 2023

jwendell commented Aug 25, 2023

howardjohn Aug 25, 2023

Choose a reason for hiding this comment

howardjohn commented Aug 25, 2023

kfaseela commented Aug 30, 2023

kfaseela commented Aug 30, 2023