Add a pod-level opt-out for ambient DNS proxying, in preparation for enabling that by default globally

[bleggett]

Relates to istio/istio#49829

This is the API side, putting it up for comment/TOC approval with a hold, impl pending.

[istio-policy-bot]

😊 Welcome @bleggett! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[hzxuzhonghu]

what is the specific case requiring this?

If there is, shouldn't this be per node rather than per pod

[bleggett]

@hzxuzhonghu we haven't encountered an in the wild use case for this yet.

We want to turn on DNS proxying by default globally for ambient. There are some concerns that since ambient DNS capture (like sidecar DNS capture) does change DNS resolution behavior, this may break obscure/fragile apps.

That's the only concern we have with enabling it by default really, and since we can't really fix that potential incompatibility, proposing this as an escape valve in case those particular apps exist and need an opt-out.

It is true that no one has reported issues yet, but that is likely because DNS capture is not enabled by default. It would be good to have this in place before we do that, as an Alpha label.

If no one uses it, we can drop it. If they do, we can promote it.

If people would rather wait until we turn ambient DNS capture on by default and get a concrete report of a user with a flaky app requiring the opt-out, I am fine with that, as well.

[linsun]

Thanks! This is helpful to understand the motivation for adding this annotation.

[howardjohn]

LGTM overall, one small comment

[costinm]

I think the model used by Istio - you need to modify your app deployment so it is not broken - does not work well for ambient. Many users install apps with 'helm install' and not all of them have ways to add annotations (and users may not have an easy time finding them). So I am pretty much against using annotations or labels on the Pod - using a separate resource would work much better. We did this for a couple of things in mesh config and in Sidecar CR.

…

On Mon, Nov 18, 2024 at 9:05 AM Ben Leggett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In annotation/annotations.yaml <#3361 (comment)>: > @@ -573,3 +573,13 @@ annotations: hidden: true resources: - Pod + + - name: ambient.istio.io/bypass-dns-capture It's a good point, I had originally intended to keep this explicitly simple/obvious as opt-out only. But in-or-out is also doable. If we want that (or think we might in the future) I am OK with something generic like ambient.istio.io/dns-capture that maybe only supports a single value of false to start with. — Reply to this email directly, view it on GitHub <#3361 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2UEYDCFHKFRSE6GT5L2BIM43AVCNFSM6AAAAABRZQQLPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBTGIZDCNZSG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[bleggett]

I think the model used by Istio - you need to modify your app deployment so it is not broken - does not work well for ambient. Many users install apps with 'helm install' and not all of them have ways to add annotations (and users may not have an easy time finding them). So I am pretty much against using annotations or labels on the Pod - using a separate resource would work much better. We did this for a couple of things in mesh config and in Sidecar CR.
…
On Mon, Nov 18, 2024 at 9:05 AM Ben Leggett @.> wrote: @.* commented on this pull request. ------------------------------ In annotation/annotations.yaml <#3361 (comment)>: > @@ -573,3 +573,13 @@ annotations: hidden: true resources: - Pod + + - name: ambient.istio.io/bypass-dns-capture It's a good point, I had originally intended to keep this explicitly simple/obvious as opt-out only. But in-or-out is also doable. If we want that (or think we might in the future) I am OK with something generic like ambient.istio.io/dns-capture that maybe only supports a single value of false to start with. — Reply to this email directly, view it on GitHub <#3361 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2UEYDCFHKFRSE6GT5L2BIM43AVCNFSM6AAAAABRZQQLPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBTGIZDCNZSG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Some overlap with the convo here, and we can talk on Wed: istio/istio#53886 (comment)

Many users install apps with 'helm install' and not all of them have ways to add annotations (and users may not have an easy time finding them).

So I am pretty much against using annotations or labels on the Pod - using a separate resource would work much better. We did this for a couple of things in mesh config and in Sidecar CR.

I agree using pod-level config has downsides, but the inverse of that is that users deploying workloads/pods may not have access to mutate a separate Istio resource either, and may not have an easy time of finding where/how to do that either, and requiring one-off opt-outs like this to be centralized doesn't simplify things for users, it just means in most cases they won't be able to easily opt-out.

It's the old app owner != cluster owner != istio owner problem.

This is, for example, why ambient dataplane exclusion at the pod level currently overrides the namespace level.

[costinm]

On Tue, Nov 19, 2024 at 11:46 AM Ben Leggett ***@***.***> wrote:

I think the model used by Istio - you need to modify your app deployment so it is not broken - does not work well for ambient. Many users install apps with 'helm install' and not all of them have ways to add annotations (and users may not have an easy time finding them). So I am pretty much against using annotations or labels on the Pod - using a separate resource would work much better. We did this for a couple of things in mesh config and in Sidecar CR. … <#m_-7990094170032508031_> On Mon, Nov 18, 2024 at 9:05 AM Ben Leggett *@*.*> wrote: @.** commented on this pull request. ------------------------------ In annotation/annotations.yaml <#3361 (comment) <#3361 (comment)>>: > @@ -573,3 +573,13 @@ annotations: hidden: true resources: - Pod + + - name: ambient.istio.io/bypass-dns-capture It's a good point, I had originally intended to keep this explicitly simple/obvious as opt-out only. But in-or-out is also doable. If we want that (or think we might in the future) I am OK with something generic like ambient.istio.io/dns-capture that maybe only supports a single value of false to start with. — Reply to this email directly, view it on GitHub <#3361 (comment) <#3361 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2UEYDCFHKFRSE6GT5L2BIM43AVCNFSM6AAAAABRZQQLPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBTGIZDCNZSG4 . You are receiving this because you are subscribed to this thread.Message ID: *@*.***> Some overlap with the convo here, and we can talk on Wed: istio/istio#53886 (comment) <istio/istio#53886 (comment)> Many users install apps with 'helm install' and not all of them have ways to add annotations (and users may not have an easy time finding them). So I am pretty much against using annotations or labels on the Pod - using a separate resource would work much better. We did this for a couple of things in mesh config and in Sidecar CR. I agree, but the inverse of that is that users deploying workloads/pods may not have access to mutate a separate Istio resource either, and may not have an easy time of finding where/how to do that either, and requiring one-off opt-outs like this to be centralized doesn't simplify things for users, it just means in most cases they won't be able to easily opt-out.

Not sure what this means - there is a doc indicating what to do in order to opt out. They need to find that doc - if it says 'add an annotation' or 'create a CR' - it's the same kind of work. If they don't have access to the CR ( RBAC ) - that's a feature, not a bug - it means the admin has decided pod owners are not allowed. They can block annotations with OPA as well - so 'not having access if the admin so decides' is the same. It doesn't have to be centralized. I know the miss-named Sidecar would be confused - so perhaps we need another resource, or use gateway - I think remaining stuck on 'you can't create new CRDs - but keep adding annotations/labels is just fine' is not ideal.

…

Message ID: ***@***.***>

[bleggett]

Okie dokie - In the ambient WG today I think what we settled on for the autoEnroll feature also makes a lot of sense here:

• Rather than use labels for this, have a ~reasonably centralized config with user defined match selectors.

I think that will satisfy most of the concerns raised here, and needing to do that for autoEnroll makes it pretty reasonable to do for this as well. I'll close this, and have a go at doing it like that.

[bleggett]

I originally closed this because at the time I was intending to do autoEnroll istio/istio#53886

which would use a selector-based API. That selector based API would allow operators to write selectors that disallowed pod-level opt-outs (not currently possible in ambient, since all traffic capture APIs allow pod-level opt-outs)

But I think we're tabling autoEnroll and the selector-style API for ambient enrollment that goes with it, so reopening this to just do pod-level DNS opt-outs, like the current ambient enrollment mechanism supports for general traffic.

A uniform control API that supports top-down policy enforcement with no pod-level opt-outs is (I believe) a good idea, but if we don't have that for ambient traffic capture generally at this time, there's no reason to have it for DNS traffic capture uniquely at this time either.

[howardjohn]

note is stale