enhancements: add SEP for revision tag support

[dgn]

No description provided.

[sridhargaddam]

Thanks @dgn for the SEP.

[sridhargaddam]

LGTM, but will wait if others have any review comments.

[FilipB]

LGTM

[fjglira]

lgtm

[luksa]

Can someone confirm that there are two separate approaches to creating tags in upstream istio?

1. istioctl tag set (this creates the MutatingWebhookConfiguration directly)
2. deploying the istiod chart via Helm and setting values.revisionTags (this creates the MutatingWebhookConfiguration along with all the other resources in this chart)

In the current proposal, we'll be creating the webhook through this second mechanism. But this doesn't feel right. A tag is not really part of a control plane; it's a separate thing that points to one. The lifecycle of the tag is (and should be) completely separate from the control plane. If we use the values.revisionTags approach, then the webhook is tied to the lifecycle of the control plane and we need to update the control plane every time the user changes the tag (I already explained this in one of my other comments where I pointed out that we'd have to reconcile both the old and the new IstioRevision).

[dgn]

Can someone confirm that there are two separate approaches to creating tags in upstream istio?

1. `istioctl tag set` (this creates the MutatingWebhookConfiguration directly)

2. deploying the istiod chart via Helm and setting `values.revisionTags` (this creates the MutatingWebhookConfiguration along with all the other resources in this chart)

In the current proposal, we'll be creating the webhook through this second mechanism. But this doesn't feel right. A tag is not really part of a control plane; it's a separate thing that points to one. The lifecycle of the tag is (and should be) completely separate from the control plane. If we use the values.revisionTags approach, then the webhook is tied to the lifecycle of the control plane and we need to update the control plane every time the user changes the tag (I already explained this in one of my other comments where I pointed out that we'd have to reconcile both the old and the new IstioRevision).

istioctl tag set just performs a helm render and filters for the MutatingWebhookConfiguration and then only deploys that. I had similar behavior in mind (in fact, I attempted importing the existing function but it didn't quite work because the charts are named differently in our repo). I'm all for keeping lifecycles separate, so I think I'll go with your suggestion of a IstioRevisionTag resource and put that into writing. The name uniqueness I would try and solve using status (and showing an error there if it's not unique). Does that work for you?

[nrfox]

Is this true in all deployment models? For Primary-Remote would you create the tag only on the primary cluster and still be able to label namespaces on the remote cluster with the tag name?

[dgn]

I think in Primary-Remote setups you'd need to create them in the Remote cluster and I'm not sure how they work in that case tbh. It's a good question

[nrfox]

Should we add support for the default tag specifically to this list? It enables support for istio-injection=enabled but also other functions in the cluster like resource validation and status writing.

[nrfox]

nit:

Suggested change

We will need to update the sail-operators mechanism to detect revisions that are being used. Today, we only look at the `istio.io/rev` label's value to check which revisions are in use. But when revision tags are used, those values will be mere aliases, so we have to improve our detection mechanism. The most correct way is probably to look at the revision annotation on the pods that is set by Istio during injection. That requires inspecting every pod in the cluster, though. Another way could be to resolve the tags - the sail-operator knows which revisions the tags point to, after all - but only if tags are exclusively managed by the sail-operator.

We will need to update the Sail-Operator's mechanism to detect in-use revisions. Today, we only look at the `istio.io/rev` label's value to check which revisions are in use. But when revision tags are used, those values will be mere aliases, so we have to improve our detection mechanism. The most correct way is probably to look at the revision annotation on the pods that is set by Istio during injection. That requires inspecting every pod in the cluster, though. Another way could be to resolve the tags - the sail-operator knows which revisions the tags point to, after all - but only if tags are exclusively managed by the sail-operator.

[nrfox]

Need to cover the case where both istio-injection and istio.io/rev are provided as well.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@5476420). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #212   +/-   ##
=======================================
  Coverage        ?   76.34%           
=======================================
  Files           ?       36           
  Lines           ?     1864           
  Branches        ?        0           
=======================================
  Hits            ?     1423           
  Misses          ?      368           
  Partials        ?       73           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.