Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

I think it should be added to label API. Honestly, i donot know this label until now.

@howardjohn this change is quite impactful. Even I wasn't aware of label being supported (not sure when it was added). Almost every user injected sidecar proxies has these annotations. I don't think we can ever remove this annotation or its support. Will marking this annotation deprecated causing istioctl checks to go crazy and still giving warnings?

Additionally, I don't see sidecar.istio.io/inject label documented here https://preliminary.istio.io/latest/docs/reference/config/labels/

I am also concerned that the new injection label isn't very useful...yet...

@howardjohn for the issue I discussed with you yesterday, could you confirm with me the behavior for pod injection annotation and label?

scenarios: result

1. pod annotation injection=true + namespace (nothing): pod injected (with default revision)

2. pod annotation injection=false + namespace (injection enabled): pod NOT injected

3. pod label injection=canary + namespace (nothing): pod injected (with canary cp)

4. pod label injection=canary + namespace (rev prod): pod injected (with prod cp)

5. pod label injection=no label + namespace(rev prod): pod injected (with prod cp)??

Note: above 1 and 2 are intuitive, no one needs to read any docs to understand it. above 4 and 5 are not intuitive.

1. pod annotation injection=true + namespace (nothing): pod injected (with default revision)

No, its not injected.

2. pod annotation injection=false + namespace (injection enabled): pod NOT injected

It IS calling Istiod, but Istiod triggers a NOP

3. pod label injection=canary + namespace (nothing): pod injected (with canary cp)

Yes

4. pod label injection=canary + namespace (rev prod): pod injected (with prod cp)

Yes

5. pod label injection=no label + namespace(rev prod): pod injected (with prod cp)??

Yes

Note: above 1 and 2 are intuitive, no one needs to read any docs to understand it. above 4 and 5 are not intuitive.

I don't disagree, but I don't think that has any bearing on if the label vs annotation. The label is strictly better than the annotation. With the annotation, calls ALWAYS go to Istiod, and are filtered on istiod side. With the label, the calls are filtered before they hit the API server.

This distinction is critical - without it, allowing injection in a namespace ties all pods availability in that namespace to istiod. For example, if we had an injected namespace but wanted to disable injection on a pod, with the annotation the pod would break without Istiod; with the label it will not.

I do agree label is better from implementation perspective and how much it ties to Istiod up running. Given 1 doesn't work today, I believe most users are using 2. What is the alternative for same behavior as 2 using labels, e.g. ns has injection enabled but I want to disable it on a particular pod in that ns?

What is the alternative for same behavior as 2 using labels, e.g. ns has injection enabled but I want to disable it on a particular pod in that ns?

You just replace the annotation with the label. The behavior is strictly better, as the call never even hits Istiod. For example, you could istio-system as enabled and istiod as disabled. With a label, no problems. With an annotation, if Istiod ever goes to 0 replicas it will never recover. You should probably not do this anyways, just an example...

Maybe I'm missing something, if I replace annotation with label:

scenario 6: pod label as no injection (something like sidecar.istio.io/inject=disabled, can I do this?), and namespace label as injection with prod, result: pod will not be injected?

If this can work, I am okay with deprecation. we don't support 4 and 5 with annotation today afaik.

@linsun I think labels are strictly better for any admission wehook logic. Using annotations was an odd choice to begin with. If we retain all functionality I am fine deprecating as long as we meet these goals:

• Never retire annotations
• Define behavior if annotations & labels are both defined and may be even conflicting?
• Does istioctl start giving many warnings which makes users stop using the tool.

Since the migration path here possibly involves changing every deployment spec for a user, we don't want to force them to go through this migration for no real benefits if their current use case is being met.

Never retire annotations

Strongly agree, its like 2 lines of code anyways so NBD

Define behavior if annotations & labels are both defined and may be even conflicting?

If either is "false" it will not inject. The annotation being "true" doesn't really do anything anyways so that is pretty much the only case

Does istioctl start giving many warnings which makes users stop using the tool.

I don't know what too many warnings is... it already gives a lot for unlabeled namespaces and port names. I have a PR to make it only warning if the label is not there (ie if you have annotation+label it will not warn since you are probably doing it for backwards compatibility, etc). If there is anything else we can do let me know and I can try to work with the UX team.
Maybe some sort of collapsing: INFO (Pod foo.bar, httpbin.default, + 100 more) Using deprecated annotation ... would make sense? And some --verbose flag or whatever to get the full thing?

@linsun @nrjpoddar should we move forward with this?

@linsun @nrjpoddar should we move forward with this for 1.12?

@linsun @nrjpoddar should we move forward with this for 1.13?

@linsun @nrjpoddar should we move forward with this for 1.13?

Do we have tooling that can indicate this deprecation? I'm ok with deprecating it as long as we have tooling to indicate it and docs explaining how to migrate. I'm assuming that we won't be removing support as this is widely used?

LGTM.

@nrjpoddar can you click the 'approve' button?