ambient: introduce a new "ingress mode" #53476
Conversation
howardjohn
added
the
release-notes-none
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
| } | ||
| } | ||
| } | ||
| // Also look for chains we jump to, even if we have no rules in them |
There was a problem hiding this comment.
@bleggett @leosarra this is maybe dumb but seemed like the best option. WDYT?
I was worried about removing the -J ISTIO_PRERT since we could easily introduce bugs here if we add some rules to the chain (as a dev, I would probably not think to make sure the jump rule was added!), and I was worried our cleanup logic would not handle it well
There was a problem hiding this comment.
I was worried about removing the -J ISTIO_PRERT since we could easily introduce bugs here if we add some rules to the chain
Might be worth splitting rule creation into ingress/egress and categorically skipping the former in this scenario.
If we are actually not hooking ingress at all, we will never have rules in PREROUTING anyway/it should be a bug if we do.
There was a problem hiding this comment.
My concern was we maybe would... we do for DNS for example. Not in nat, though, so maybe moot
There was a problem hiding this comment.
Ah, I missed DNS is doing that. That's annoying.
I still think we should be fine if we just decompose this func better: inbound/outbound/DNS and that will go a ways towards de-footgunning.
I was worried about removing the -J ISTIO_PRERT since we could easily introduce bugs here if we add some rules to the chain (as a dev, I would probably not think to make sure the jump rule was added!),
Our logic should probably be smart enough to dynamically not-create the jump if the jump dest is empty, but I'm fine leaving it in place for now, jumps to empty chains are strictly harmless.
| @@ -214,8 +214,13 @@ func (cfg *IptablesConfigurator) CreateInpodRules(log *istiolog.Scope, hostProbe | |||
| return nil | |||
| } | |||
|
|
|||
| func (cfg *IptablesConfigurator) appendInpodRules(hostProbeSNAT, hostProbeV6SNAT netip.Addr) *builder.IptablesRuleBuilder { | |||
| func (cfg *IptablesConfigurator) appendInpodRules(hostProbeSNAT, hostProbeV6SNAT netip.Addr, ingressMode bool) *builder.IptablesRuleBuilder { | |||
There was a problem hiding this comment.
It might be more reasonable at this point to split this into appendInpodInbound/OutboundRules
| } | ||
| } | ||
| } | ||
| // Also look for chains we jump to, even if we have no rules in them |
There was a problem hiding this comment.
I was worried about removing the -J ISTIO_PRERT since we could easily introduce bugs here if we add some rules to the chain
Might be worth splitting rule creation into ingress/egress and categorically skipping the former in this scenario.
If we are actually not hooking ingress at all, we will never have rules in PREROUTING anyway/it should be a bug if we do.
howardjohn
force-pushed
the
ambient/ingress-mode
branch
from
056229f to
5e01a4e
Compare
istio-testing
removed
the
do-not-merge/work-in-progress
howardjohn
force-pushed
the
ambient/ingress-mode
branch
from
a2fcbf7 to
c50e3c4
Compare
The intent here is to better facilitate running an ingress workload in the mesh. The ideal flow is `internet client --> ingress pod --> ztunnel --> <mesh destination>`. Today, we have `client --> ztunnel --> ingress --> ztunnel`; this should work, but its not really necessary. This is done by utilizing an existing annotation, `traffic.sidecar.istio.io/excludeInboundPorts: "*"`. This is the ONLY value supported here; we do not (and IMO should not) support more customization. I used the same annotation, rather than a new one, since its a fairly common config to have on ingress today, so this may make adoption easier.
howardjohn
force-pushed
the
ambient/ingress-mode
branch
from
c50e3c4 to
6b4755e
Compare
|
/retest |
Added in istio in istio/istio#53476
Added in istio in istio/istio#53476
Added in istio in istio/istio#53476
The intent here is to better facilitate running an ingress workload in
the mesh. The ideal flow is
internet client --> ingress pod --> ztunnel --> <mesh destination>.Today, we have
client --> ztunnel --> ingress --> ztunnel; this shouldwork, but its not really necessary.
This is done by utilizing an existing annotation,
traffic.sidecar.istio.io/excludeInboundPorts: "*".This is the ONLY value supported here; we do not (and IMO should not)
support more customization. I used the same annotation, rather than a
new one, since its a fairly common config to have on ingress today, so
this may make adoption easier.