cors: do not forwarded preflight request if the origin is not allowed

[zirain]

Please provide a description of this PR:

API: istio/api#3171

Related to envoyproxy/envoy#33051

[ramaraochavali]

IMO, we should not introduce a behavioural change unless it is a bug fix, at the very least we should have a feature flag or introduce the flag in the api

[zirain]

IMO, we should not introduce a behavioural change unless it is a bug fix, at the very least we should have a feature flag or introduce the flag in the api

I'm asking what's the best choice, feature flag or api change, or let it be?

[howardjohn]

Do we have a motivation for this from users? It seems like (without more context) Envoy made a change and we are just copying it without an user demand?

[zirain]

this is a request from our customer, we fixed it in our fork, and want to backport to upstream.

[hzxuzhonghu]

We cannot simply set it for older proxy

[zirain]

We cannot simply set it for older proxy

if a new pilot feature flag acceptable?

[hzxuzhonghu]

The correct behavior should be not forwarding non matching preflights. Here we should add a check for proxy version, and only set this field for 1.22?

[howardjohn]

why does the envoy side talk about "forwarding" but the PR talks about modifying response headers?

[howardjohn]

But generally if we are making a breaking change compatibilityVersion is the way to do it. But we need agreement that we want to break the behavior for all users

[howardjohn]

Is there prior art in other gateway implementations?

[zirain]

why does the envoy side talk about "forwarding" but the PR talks about modifying response headers?

miss with other stuffs

But generally if we are making a breaking change compatibilityVersion is the way to do it. But we need agreement that we want to break the behavior for all users

compatibilityVersion is good to me, or a pilot feature flag?

Is there prior art in other gateway implementations?

this's a new API, AFAIK only EnvoyGateway change it.

[howardjohn]

I don't mean in Envoy I mean in the broader ecosystem of Cors. So far looks like Kong makes it configurable

[zirain]

I don't mean in Envoy I mean in the broader ecosystem of Cors. So far looks like Kong makes it configurable

After some investigation, nginx base proxy make it configurable, didn't find too much info about HAProxy.

[zirain]

@howardjohn @ramaraochavali @hzxuzhonghu do you agree with use feature flag for this?

[hzxuzhonghu]

I am ok

[ramaraochavali]

I am also OK as long as we have path forward to remove this via a proper API/making it default behaviour. If our long term plan is API, should we just go ahead with API now? What do you plan to set the default value to be?

[howardjohn]

My 2c:

• If we are going to make it an API, do it now
• If we are going to change the default (1) get agreement on this since its a major behavioral change (2) make an env var, on by default, with compatibility version off by default
• If we are never going to make it on by default, do not merge anything. Users can use EnvoyFilter.

[zirain]

Personally, I tend to take ForwardNotMatchingPreflights=false as the default behavior.
But for projects that have been around for a long time, changing the default behavior is an unfriendly thing to do, some users may even rely on current behavior, and request to keep the feature flag(pilot env) exists alway.

As all these trade off, is a API change acceptable? If so, I will do that after 1.22 branch cutted.

[ramaraochavali]

My preference would be API and doing it now (without feature flag after 1.22 branch cut)