daemon, config: regenerate endpoint datapath on agent config change

[jaffcheng]

Right now changing some agent options like PolicyAuditMode from
cilium CLI succeeds but doesn't affect datapath, e.g.:

cilium config PolicyAuditMode=true

This patch fixes this by applying agent config changes to endpoint
header file and triggering endpoint datapath regeneration.

Fixes: #13902
Signed-off-by: Jaff Cheng jaff.cheng.sh@gmail.com

[christarazi]

Thanks for the PR. Overall, I think it looks good. A few items to address below.

[christarazi]

LGTM, just a few minor nits to address. Doesn't require another look from me. Can you rebase your PR on lastest master to pick up some CI changes? Currently, we have some Docker rate limiting issues and a rebase would pick up the new mitigations, so that we can run tests against this PR.

[christarazi]

test-me-please

[sayboras]

test-me-please

[jibi]

test-me-please

[jibi]

test-me-please

[jibi]

Changes LGTM, but there are a few tests in the Runtime-4.9 suite failing

[jibi]

test-gke

[jaffcheng]

Thanks for the comments! Added a comment on reusing policyTriggerMetrics and split the change of using Config.Opts.Opts[option.PolicyAuditMode] to a separate commit. Please take another look

[jibi]

test-me-please

[jaffcheng]

test-me-please

Runtime-4.9 failed tests, not sure if related to this PR:
Suite-runtime.RuntimePolicies Test egress with L7 policy to outside cluster
Suite-runtime.RuntimePolicies Extended HTTP Methods tests
Suite-runtime.RuntimePolicies CIDR L3 Policy validates toCIDR

[jibi]

Runtime-4.9 failed tests, not sure if related to this PR:

I don't think this is a flake in the CI as your branch was rebased recently and the Runtime-4.9 test suite is green on master

[aanm]

Just one small nit

[joestringer]

Thanks for following up and investigating the concerns I brought up. I thought a bit longer about the solution here and I think we need to go further to provide strong guarantees and reduce the potential for long blocking mutexes in the agent. More detail below.

[joestringer]

These locking changes make me nervous. It looks very easy to misunderstand the locking patterns and introduce a deadlock. I've looked for about half an hour now and I can't convince myself it's correct (it may be; but even if it is, it looks easy to break in future).

I believe this is attempting to address the comments here:
#13971 (comment)

Furthermore, I think it is trying to address a case like:

1. API request comes in to configure some setting A
   1. Previous configuration is stored
   2. Configuration is applied
   3. Lock is released
   4. Reinitialization is triggered
2. API request comes in to configure some setting B
   1. Configuration is applied concurrently with reinitialization
   2. Lock is released
   3. Reinitialize is triggered
3. The step (1.iv) fails, so we need to undo the attempt at reconfiguration
   1. Overwrite the configuration with the original state, which undoes setting B modifications
4. The step (2.iii) succeeds, but (3) already reverted the changes

Clearly this is not the behaviour we want the API to have so I agree we should address it 👍

That said, I don't think it's a good idea to extend the locking over the entire Reinitialize() and try to enforce various places underneath to grab the right locks at the right time. This extends the locking over a long period, and also forces various locations to have more understanding of the locking because the locks aren't following a simple obvious "lock, read/change, unlock" pattern. In fact, the Reinitialize() path is shelling out to an external script which runs other processes like a compiler and so on, so we have very few guarantees about how long that will take. It could take seconds or even minutes in a CPU-starved environment (not typical, but possible).

The general way we solve this problem in other cases like policyAdd is to have an eventqueue where the events are serialized, then the reconfiguration is handled from there. If the caller wants to wait for the results, there is a channel-based mechanism to listen for the success or failure of the changes. See putPolicy.Handle and its callee Daemon.PolicyAdd() for examples.

Why is this different from just grabbing the lock? It allows serialization of the API requests without forcing the underlying data structures to be locked for a long period. The locking of the underlying structures, if necessary, can be handled briefly from various goroutines without introducing arbitrarily long delays. I believe this is already the case today for data like IPv4NativeRoutingCIDR, which currently has quite granular locking (which this PR is currently proposing to extend to a long-held lock).

With this approach in place, the core logic of a new eventqueue's struct ConfigModifyEvent Handle() function could store the changes, lock, modify, unlock, Reinitialize(), then choose to either Lock+revert+Unlock or return success depending on the result of the reinitialization.

Originally I thought that maybe there's an alternative path, Undoing the changes in the event of a failure without holding the lock the whole time. For simple cases this might be doable to check that the settings are configured how we expect before reverting them in the error handling, but overall the most robust approach would be to follow the EventQueue approach.

[jaffcheng]

Thanks for the detailed write-up! I don't like the locking changes of various places as well. Sorry for not realizing the potential long execution time of Reinitialize and the benefit of using an event queue. Will try the suggested approach.

[aanm]

test-me-please

[joestringer]

Nice work!

[joestringer]

GKE CI job failed, but unclear why due to age. I'll rekick it.

[joestringer]

retest-gke

[joestringer]

I had originally nominated this for v1.9 backport assuming it is a fairly small change and given the proximity to the v1.9.0 release. At this point, I think the changes are structurally a bit more significant and this goes a bit outside the usual bugfixes for the latest release. Unless I hear a strong argument for why this change is safe to backport to v1.9, I will opt to drop that backport proposal and instead we'll include it in the upcoming v1.10 release.