endpoint: Do not override deny entries with proxy redirects

[jrajahalme]

Use DenyPreferredInsert instead of directly manipulating policy map state to make sure deny entries are not overridden by new proxy redirect entries.

Prior to this fix it was possible for a proxy redirect to be pushed onto the policy map when it should have been overridden by a deny at least in these cases:

• L3-only deny with L3/L4 redirect: No redirect should be added as the L3 is denied, the new test case verifies this
• L3-only deny with L4-only redirect: L4-only redirect should be added and an L3/L4 deny should also be added, but the L3/L4 deny is only added by deny preferred insert, and is missed when the map is manipulated directly.

It is clear that in the latter case the addition of the redirect can not be completely blocked, so we can't fix this by making AllowsL4 more restrictive. But also in the former case it is possible that the deny rule only covers a subset of security identities, while the redirect rule covers some of the same security identities, but also some more that should not be blocked. Hence the correct fix here is to leave AllowsL4 to be L3-independent, and cover these cases with deny preferred insert instead of adding redirect entries to the map directly.

This PR also contains a related change that allows a redirect entry to be updated, maybe with a changed proxy port. I've not seen evidence that this is currently fixing a bug, but it feels like a real possibility.

Fixes: #12716

Fixed proxy redirect policy implementation when any deny rule prevents them.

[jrajahalme]

/test

[jrajahalme]

This is a bug in the Cilium redirect creation logic that needs to be fixed for both deny and auth rules to be properly handled. This current form of the PR seems to break Agent restart FQDN tests, so some more investigation is needed on why that happens.

Update: Figured out that it is natural for new L3-specific DNS proxy redirects to generate no MapState as the proxy has yet to allocate any identities. Removed the check for the number of entries generated and removed the removal of redirects that do not generate any MapState (earlier I had assumed that would only happen if deny entry overrides all the MapState for the new redirect).

[jrajahalme]

Removed unrelated change/cleanup to make both reviews and backports easier.

[jrajahalme]

Fixed commit message.

[jrajahalme]

Added additional needs-backport labels

[jrajahalme]

/test

[christarazi]

Is it expected that DenyPreferredInsertWithChanges will insert into insertedDesiredMapState?

[jrajahalme]

Previously we were inserting into insertedDesiredMapState only when the key/entry did not exist previously, now DenyPreferredInsertWithChanges inserts into insertedDesiredMapState also when the there was a previous entry with the same key. This does not matter, as where these are used in the revert code below, we first delete all inserted keys, then add back the old values, if they existed, from updatedDesiredMapState. End result is that same before and after, previously the revert code directly replaced the new value with the old one, now the new value is deleted first, and the old one added back after.

[jrajahalme]

Hmm. if there were old values also for (e.g., L3L4) entries created by DenyPreferredInsertWithChanges then the old values of those can not be reverted back in case of a failure with the current code in the PR. I may have to change adds and deletes arguments to DenyPreferredInsertWithChanges to be MapState instead of just Keys to make DenyPreferredInsertWithChanges revertible. Let me try that!

[christarazi]

typo:

Suggested change

	// Merging owners has the effect of keeping the shered entry alive as long as one of the owners exists.
	// Merging owners has the effect of keeping the shared entry alive as long as one of the owners exists.

[jrajahalme]

Fixed, thanks!

[christarazi]

IIUC, this is not changing functional behavior and we are just adding an extra condition for explicitness for the if-statement, correct?

[jrajahalme]

Right now I think that is the case, yes. I was thinking that if the proxy port of an existing entry needs to be updated, then this code path should allow for that. I think this is currently not happening, but we are replacing direct manipulation of the redirect entry in bpf.go with the DenyPreferredInsertWithChanges that will end up here for a non-deny entry, so I thought it would be best to allow for proxy port to be updated by that call, if it ever needs to happen.

[jrajahalme]

Updated with a new 1st commit that exports DenyPreferredInsertWithChanges() and makes it revertible, including unit testing.