add injectionSelectors #2212

liuxu623 · 2022-01-20T08:06:29Z

InjectionSelectors is a list of Kubernetes selectors that specify the set of namespaces, Istio will create istio-ca-root-cert configmap in selected namespaces.If omitted, Istio will create istio-ca-root-cert configmap in all namespaces except kubernetes system namespaces.

Fix istio/istio#31115

istio-testing · 2022-01-20T08:06:53Z

Hi @liuxu623. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Arhell

/ok-to-test

istio-policy-bot · 2022-01-28T10:31:08Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

kfaseela · 2022-02-25T09:22:04Z

@liuxu623 you need a rebase on this PR as well?

liuxu623 · 2022-02-25T09:47:09Z

@liuxu623 you need a rebase on this PR as well?

@kfaseela Please help me review code, thank you.

kfaseela · 2022-02-25T09:56:51Z

@liuxu623 you need a rebase on this PR as well?

@kfaseela Please help me review code, thank you.

sure..will do.. but considering this is an API change, I assume you would have already discussed this in one of the working group meetings, and got a consensus for this enhancement?

liuxu623 · 2022-02-25T10:26:35Z

@liuxu623 you need a rebase on this PR as well?

@kfaseela Please help me review code, thank you.

sure..will do.. but considering this is an API change, I assume you would have already discussed this in one of the working group meetings, and got a consensus for this enhancement?

No, I didn't discuss this with working group yet...

howardjohn · 2022-03-09T16:59:41Z

mesh/v1alpha1/config.proto

@@ -1063,8 +1063,31 @@ message MeshConfig {
  // configured globally via this field.
  istio.networking.v1alpha3.HTTPRetry default_http_retry_policy = 62;

+  // A list of Kubernetes selectors that specify the set of namespaces,
+  // Istio will create istio-ca-root-cert configmap in selected namespaces.


If this is only for the istio-ca-root-cert configmap it seems like the name namespaceSelectors is far too broad?

Maybe this could become injectionSelectors and additionally control whether the sidecar injector accepts an injection request from a namespace?

That is already defined by the mutating webhook; a mesh config setting cannot actually control it at all (unless it becomes an operator for the Webhook)

I understand, but since scoping the istio-ca-root-cert configmap will allow deploying multiple distinct istiod instances, there may then also be multiple mutating webhook configs that each invoke /inject on the same pod but different webhook services.
An istiod instance with injectionSelector could return false at injectRequired(), similar to the handling for "special" k8s namespace at https://github.com/istio/istio/blob/098cc47dfcbdca202208bd241d9dee35cd230a4d/pkg/kube/inject/inject.go#L201-L205, if the pod namespace does not match the selector.
I would not currently expect that turning it into an operator for the webhook will be necessary

They should use revisions so they only match their own distinct istio.io/rev.

if we want to define a full multitenancy proposal then I would recommended we have a holistic design doc on how this will work.

An istiod instance with injectionSelector could return false at injectRequired()

This is much worse than the current Webhook scoping of istio.io/rev, since now ALL istiods are points of failure for a pod starting. Doing it in webhook selector -- which already exists using revision -- filters at the k8s level and does not add single point of failures

@howardjohn John, do you think there is a way with existing methods to solve the isolation of config-map creation, so that I don't have to add an additional namespaceSelector like this.

I don't understand how istio.io/rev doesn't solve this. It was explicitly designed to handle the case if ensuring different namespace use different control planes for injection. Can you elaborate?

Yes, revisions DO solve parts of the problem. But if we are talking multi-tenancy, each tenant should have its own rootCA and control a subset of namespaces both with and without revisions. This means that the same revision could occur in each tenant and yet be distinct/unrelated. Effectively, xSelectors (placeholder) should represent a namespace boundary beyond which a particular control plane instance should not make changes (s.a. creating config map or injecting proxies).

istio.io/rev can isolate differenet verison istiod，but it seem couldn't isolate multi same verison istiod.
It does, there is no reason they need to be different versions.

This means that the same revision could occur in each tenant and yet be distinct/unrelated.

I don't agree this is valid use case. Kubernetes is fundamentally not entirely multitenant. There are shared resources that Istio depends on - CRDs and webhooks. There must be something distinguishing these between tenants, and some higher level admin ensuring these do not conflict. In Istio, that is revision. If you would like to introduce a different mechanism for this I would suggest a design doc holistically describing how multitenancy is expected to work.

You are right, CR consumption must also be limited along with the aforementioned CM creation and proxy injection. That is why I had expressed interest when extending discovery selectors to CRs was proposed in istio/istio#36639. But probably it will be better if both are part of the same config toggle instead of having to fine-tune several. And webhooks of course need some attention. I will try to see if I can condense all that into a design doc when I have some time..

In addition to what John mentioned about revisions - assuming we can support multiple instances of the same version of Istiod in the cluster (one for each tenant), each with different revision, would it make sense to make the istio-ca-root-cert config map revision based - i.e istio-ca-root-cert-revision. This would still install configmaps in all namespaces, but each tenant would have its own root of trust?

hzxuzhonghu · 2022-03-11T03:19:40Z

Do you have a high-level design? There are too many factors to consider for multi-tenants, we need a design.

istio-testing · 2022-03-16T22:03:40Z

@liuxu623: PR needs rebase.

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.

kfaseela · 2022-09-14T17:24:57Z

Do you have a high-level design? There are too many factors to consider for multi-tenants, we need a design.

https://docs.google.com/document/d/1y4liRJbQW0NCMeQtqMma46flVqs-izV1/

istio-policy-bot · 2024-05-15T23:35:50Z

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-09-14. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

liuxu623 requested review from ericvn, howardjohn, linsun, louiscryan, nrjpoddar and smawson as code owners January 20, 2022 08:06

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 20, 2022

istio-testing added the needs-ok-to-test label Jan 20, 2022

liuxu623 mentioned this pull request Jan 20, 2022

[WIP] support multi Istio deployments in same k8s cluster istio/istio#36888

Closed

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 20, 2022

liuxu623 force-pushed the namesapceSelectors branch from 3baca62 to f4a9e5d Compare January 21, 2022 02:54

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 21, 2022

Arhell reviewed Jan 28, 2022

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jan 28, 2022

liuxu623 force-pushed the namesapceSelectors branch from f4a9e5d to 207bea7 Compare January 28, 2022 10:43

istio-testing 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 Jan 28, 2022

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 16, 2022

liuxu623 force-pushed the namesapceSelectors branch from 207bea7 to bf50c54 Compare February 25, 2022 09:27

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2022

liuxu623 force-pushed the namesapceSelectors branch from bf50c54 to 0e3ffd4 Compare February 25, 2022 09:34

istio-testing 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 25, 2022

liuxu623 force-pushed the namesapceSelectors branch from 0e3ffd4 to 58595f1 Compare February 25, 2022 11:21

howardjohn reviewed Mar 9, 2022

View reviewed changes

add injectionSelectors

987b564

liuxu623 changed the title ~~add namespaceSelectors~~ add injectionSelectors Mar 10, 2022

liuxu623 force-pushed the namesapceSelectors branch from 58595f1 to 987b564 Compare March 10, 2022 12:02

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 16, 2022

skonto mentioned this pull request May 23, 2022

[WIP][POC] Add multiple Kourier support knative-extensions/net-kourier#730

Closed

arkodg mentioned this pull request Jul 1, 2022

Append revision to istio-ca-root-cert istio/istio#39739

Closed

howardjohn removed the needs-rebase Indicates a PR needs to be rebased before being merged label May 15, 2024

istio-policy-bot closed this May 15, 2024

istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add injectionSelectors #2212

add injectionSelectors #2212

liuxu623 commented Jan 20, 2022 •

edited

istio-testing commented Jan 20, 2022

Arhell left a comment

istio-policy-bot commented Jan 28, 2022

kfaseela commented Feb 25, 2022

liuxu623 commented Feb 25, 2022

kfaseela commented Feb 25, 2022

liuxu623 commented Feb 25, 2022

howardjohn Mar 9, 2022

emike922 Mar 9, 2022

howardjohn Mar 9, 2022

emike922 Mar 9, 2022

howardjohn Mar 9, 2022

liuxu623 Mar 11, 2022

emike922 Mar 11, 2022

howardjohn Mar 11, 2022

emike922 Mar 11, 2022

shankgan Mar 16, 2022 •

edited

hzxuzhonghu commented Mar 11, 2022

istio-testing commented Mar 16, 2022

kfaseela commented Sep 14, 2022

istio-policy-bot commented May 15, 2024

add injectionSelectors #2212

add injectionSelectors #2212

Conversation

liuxu623 commented Jan 20, 2022 • edited

istio-testing commented Jan 20, 2022

Arhell left a comment

Choose a reason for hiding this comment

istio-policy-bot commented Jan 28, 2022

kfaseela commented Feb 25, 2022

liuxu623 commented Feb 25, 2022

kfaseela commented Feb 25, 2022

liuxu623 commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shankgan Mar 16, 2022 • edited

Choose a reason for hiding this comment

hzxuzhonghu commented Mar 11, 2022

istio-testing commented Mar 16, 2022

kfaseela commented Sep 14, 2022

istio-policy-bot commented May 15, 2024

liuxu623 commented Jan 20, 2022 •

edited

shankgan Mar 16, 2022 •

edited