proxy: Do not change configured proxy ports

[jrajahalme]

Proxy redirect creation tries another proxy port number in case redirect creation needs to be retried. Do not change the proxy port on retries if the proxy port has already been configured, as configured proxy ports are already being used by proxies for listening.

Previously we had avoided proxy port change on retry for DNS proxies when the DNS port had been explicitly configured. We need to do the same for proxy listeners defined in CiliumEnvoyConfig CRDs. Checking for the 'configured' flag convers both cases, as it is set via SetProxyPort() called by DNS proxy on agent bootstrap, and via AllocateProxyPort() called for CEC CRD listeners.

Fixes: #25969

Keep sync on deployed proxy ports when retrying proxy redirect creation.

[jrajahalme]

/test

[jrajahalme]

All required checks are passing.

[jrajahalme]

@mhofstetter marked this as a release blocker for 1.14, as this may become more easily hit with the non-embedded Envoy.

[mhofstetter]

@mhofstetter marked this as a release blocker for 1.14, as this may become more easily hit with the non-embedded Envoy.

👍

@jrajahalme can you please provide a little bit more context when the issue arises?

My guess is that it occurs for cases where a CEC defines a listener (port gets allocated via AllocateProxyPort) and a CNP defines a L7 policy for this port (which calls CreateOrUpdateRedirect). And this is where the configured port shouldn't be changed.

[christarazi]

It seems @mhofstetter has a more in-depth understanding of this change than I do, so I will request reviews from him. FWIW, at a high level, the change makes sense.

[jrajahalme]

@mhofstetter marked this as a release blocker for 1.14, as this may become more easily hit with the non-embedded Envoy.

👍

@jrajahalme can you please provide a little bit more context when the issue arises?

My guess is that it occurs for cases where a CEC defines a listener (port gets allocated via AllocateProxyPort) and a CNP defines a L7 policy for this port (which calls CreateOrUpdateRedirect). And this is where the configured port shouldn't be changed.

Terminology here is a bit misleading as configured here means that the immediate function call used to create a proxy redirect with that port succeeded. Proxy port allocation (initially and retries, as well as re-create due to endpoint regeneration) is gated with !configured to make sure we do not change a port that has successfully been given to a proxy for listening. Previously, the retry logic was not gated with this same condition, so it was possible for the proxy port to be incremented if an immediate proxy create error occurred. An immediate proxy error could be, e.g., failing to launch the embedded cilium-envoy, maybe due to memory pressure or something.

I refactored the code to only use a single !pp.configured condition in this context to make it clearer that the proxy port increment on retry is related to the proxy port allocation right after (the incremented port number is just a hint toallocatePort() as the incremented port number could be in use already).

[jrajahalme]

/test

[mhofstetter]

Terminology here is a bit misleading as configured here means that the immediate function call used to create a proxy redirect with that port succeeded. Proxy port allocation (initially and retries, as well as re-create due to endpoint regeneration) is gated with !configured to make sure we do not change a port that has successfully been given to a proxy for listening. Previously, the retry logic was not gated with this same condition, so it was possible for the proxy port to be incremented if an immediate proxy create error occurred. An immediate proxy error could be, e.g., failing to launch the embedded cilium-envoy, maybe due to memory pressure or something.

I refactored the code to only use a single !pp.configured condition in this context to make it clearer that the proxy port increment on retry is related to the proxy port allocation right after (the incremented port number is just a hint toallocatePort() as the incremented port number could be in use already).

thanks for the explanation!

[mhofstetter]

Follow up to my question in the PR discussion:

Am i correct that this logic still increments the proxy port in case of an initial redirect creation if an Envoy startup error occurs in createEnvoyRedirect (e.g. memory pressure in case of embedded mode). In this case pp.reservePort() , which would set pp.configured=true, will not be called (only if no error occured).

[mhofstetter]

LGTM, with the one remaining question. but this should be only in cases with CNP listeners - where the retry with a new port is ok.