I know this needs further polishing on test and possibly documentation, but I opened this to get initial feedback if this look like the right way to do this and whether or not traffic on egressgw that does not match any node should be dropped by default or not (or maybe this behavior should be guarded under a non default option?). I assumed this was the default behavior and had a bad surprise on my home setup (where I use this as some sort of proxy to reach the external world on some specific workloads).

Thanks for the PR @MrFreezeex! Couple of comments below.

First I think 2158822e021025b4c25ac0d054aef1a14de2cbf0 warrants its own PR as that's a separate bug fix.

Next, for the other 2 commits, I'm not opposed to the idea of explicitly dropping traffic selected by a CEGP that doesn't match any node (I think that's desirable) but this will cause some issues during upgrades/downgrades of the agent. For example:

• let's say we have a policy with an excluded CIDR
• Cilium gets upgraded and the agent installs the new bpf programs
• these programs will now interpret the 0.0.0.0 gateway IP as a drop rather than an excluded CIDR, causing a short time window where traffic that should leave the cluster non masqueraded gets dropped
• eventually the egressgw manager updates the bpf maps, and connectivity is restored

One way to work around this would be to introduce the change over a couple of releases:

• 1.x.y release: teach the datapath about 255.255.255.255 being an excluded CIDR while keeping also the existing 0.0.0.0 case and make the agent use 255.255.255.255 as well for excluded CIDRs
• 1.x.y+1 release: switch the 0.0.0.0 case in the datapath to a drop

but I need to think a bit more here (as that could still cause some drops for example when downgrading from 1.x.y) so perhaps we can start with getting the first commit in?

Thanks for the PR @MrFreezeex! Couple of comments below.

First I think 2158822 warrants its own PR as that's a separate bug fix.

Sure I will do that.

One way to work around this would be to introduce the change over a couple of releases:

* `1.x.y` release: teach the datapath about `255.255.255.255` being an excluded CIDR while keeping also the existing `0.0.0.0` case and make the agent use `255.255.255.255` as well for excluded CIDRs

* `1.x.y+1` release: switch the `0.0.0.0` case in the datapath to a drop

but I need to think a bit more here (as that could still cause some drops for example when downgrading from 1.x.y) so perhaps we can start with getting the first commit in?

Migration is a very good point that I indeed didn't have in mind! I am thinking about an alternative solution though, we could just say that 255.255.255.255 would be the blocking special value and keep 0.0.0.0 as the current excluded cidr case. It kinda makes less sense in terms of what the value means but at least this will help getting a smoother upgrade. WDYT?

Thanks for the PR @MrFreezeex! Couple of comments below.
First I think 2158822 warrants its own PR as that's a separate bug fix.

Sure I will do that.

Here is the separated PR for the first commit: #24646

I am thinking about an alternative solution though, we could just say that 255.255.255.255 would be the blocking special value and keep 0.0.0.0 as the current excluded cidr case. It kinda makes less sense in terms of what the value means but at least this will help getting a smoother upgrade. WDYT?

I thought about that as well, but as you pointed out it makes things less clear, so not 100% sure we want to go this way 🤔

I am thinking about an alternative solution though, we could just say that 255.255.255.255 would be the blocking special value and keep 0.0.0.0 as the current excluded cidr case. It kinda makes less sense in terms of what the value means but at least this will help getting a smoother upgrade. WDYT?

I thought about that as well, but as you pointed out it makes things less clear, so not 100% sure we want to go this way thinking

Aren't those values implementation details? We dump them in the CLI commands, but maybe that's our mistake and we should convert them to e.g. block and excluded when showing the map content?

I am thinking about an alternative solution though, we could just say that 255.255.255.255 would be the blocking special value and keep 0.0.0.0 as the current excluded cidr case. It kinda makes less sense in terms of what the value means but at least this will help getting a smoother upgrade. WDYT?

I thought about that as well, but as you pointed out it makes things less clear, so not 100% sure we want to go this way thinking

Aren't those values implementation details? We dump them in the CLI commands, but maybe that's our mistake and we should convert them to e.g. block and excluded when showing the map content?

Yep afaik they are and never shown except with cilium bpf egress list anyway. The issue is more about the scenario described here #24449 (comment) as people using excluded cidr will have a short connection drop at upgrade time if this PR goes as is. Doing the opposite is not a problem for the user it's more a maintainability/readability issue I guess. Like it's easier to conceptualize that 0/no ip (for 0.0.0.0)
would mean block and "1"/broadcast (for 255.255.255.255) would mean excluded/route traffic without egressgw than the opposite. So there is a choice to make between code readability and possibly splitting the change in multiple release (or something else?).

Ok actually I might have another idea. We could add an optional settings disabled by default to block traffic if egressgw doesn't match anything (disabled by default for now). If enabled it will block 0.0.0.0 and if disabled ignore both 0.0.0.0 and 255.255.255.255. And then in a future release you could decide to either keep this option disabled as is so it won't be a breaking change from the current behavior whatsoever or enable it/drop the option.

Doing the opposite is not a problem for the user it's more a maintainability/readability issue I guess. Like it's easier to conceptualize that 0/no ip (for 0.0.0.0)
would mean block and "1"/broadcast (for 255.255.255.255) would mean excluded/route traffic without egressgw than the opposite.

Honestly, if it's just a matter of code readability, then I think that's a job for clear constant names (e.g., const specialExcludedValue = "0.0.0.0") and code comments. I agree ideally the other way around would be better, but I'm not sure it's worth the additional complexity of special upgrade/downgrade handling or new settings.

/test

That worked to fix most of the failures, the only one left is for the new end to end test:

[2023-04-11T12:53:02.290Z] FAIL: Failed waiting for egress policy map entries
[2023-04-11T12:53:02.290Z] Expected
[2023-04-11T12:53:02.291Z]     <*errors.errorString | 0xc00060dc00>: {
[2023-04-11T12:53:02.291Z]         s: "could not ensure egress policy entries count is equal to 2: 4m0s timeout expired",
[2023-04-11T12:53:02.291Z]     }
[2023-04-11T12:53:02.291Z] to be nil
[2023-04-11T12:53:02.291Z] ===================== TEST FAILED =====================
[2023-04-11T12:53:02.291Z] 12:52:48 STEP: Running AfterFailed block for EntireTestsuite K8sDatapathEgressGatewayTest egress gw policy when the gateway is not found
[..]
[2023-04-11T12:53:02.291Z] Fetching command output from pods [cilium-c9gtp cilium-m5j7k]
[2023-04-11T12:53:02.291Z] cmd: kubectl exec -n kube-system cilium-c9gtp -c cilium-agent -- cilium bpf egress list
[2023-04-11T12:53:02.291Z] Exitcode: 0 
[2023-04-11T12:53:02.291Z] Stdout:
[2023-04-11T12:53:02.291Z]  	 

and the reason for that is that the source pods are not deployed (as we are using a separate context for that test).

Moving Context("egress gw policy when the gateway is not found", .. together with the other tests in the doContext function should ensure that everything we need for that test is deployed (and also the new test will run with all the configurations we are testing)

That worked to fix most of the failures, the only one left is for the new end to end test:

[2023-04-11T12:53:02.290Z] FAIL: Failed waiting for egress policy map entries
[2023-04-11T12:53:02.290Z] Expected
[2023-04-11T12:53:02.291Z]     <*errors.errorString | 0xc00060dc00>: {
[2023-04-11T12:53:02.291Z]         s: "could not ensure egress policy entries count is equal to 2: 4m0s timeout expired",
[2023-04-11T12:53:02.291Z]     }
[2023-04-11T12:53:02.291Z] to be nil
[2023-04-11T12:53:02.291Z] ===================== TEST FAILED =====================
[2023-04-11T12:53:02.291Z] 12:52:48 STEP: Running AfterFailed block for EntireTestsuite K8sDatapathEgressGatewayTest egress gw policy when the gateway is not found
[..]
[2023-04-11T12:53:02.291Z] Fetching command output from pods [cilium-c9gtp cilium-m5j7k]
[2023-04-11T12:53:02.291Z] cmd: kubectl exec -n kube-system cilium-c9gtp -c cilium-agent -- cilium bpf egress list
[2023-04-11T12:53:02.291Z] Exitcode: 0 
[2023-04-11T12:53:02.291Z] Stdout:
[2023-04-11T12:53:02.291Z]  	 

and the reason for that is that the source pods are not deployed (as we are using a separate context for that test).

Moving Context("egress gw policy when the gateway is not found", .. together with the other tests in the doContext function should ensure that everything we need for that test is deployed (and also the new test will run with all the configurations we are testing)

Oh yes indeed sorry for that 🤦‍♂️, I was struggling to find the root cause for this, thanks!

/test

Looks good now, thanks for bearing with me through the few iterations and for all the extra tests!

Thanks for bearing with me too and for all your guidance :D.

With regard to part 2: I don't think we need to wait for anything in particular, we can already start getting that into master, and label it for backport only once this PR is backported and released in v1.13

Sure 👍, I can prepare something for it as soon as this gets merged! :)

/test

@jibi Unfortunately this does not apply cleanly to v1.13. Marking with backport/author, assuming you will be able to do the backport to the v1.13 branch.

#24849 is merged, updating the label to done.