Better error reporting/catching in agent on nativeRoutingCIDR

[jibi]

See individual commits

[qmonnet]

Looks good!

[bmcustodio]

@jibi could you please rebase now that #16469 has been merged?

[jibi]

@jibi could you please rebase now that #16469 has been merged?

@bmcustodio done ✔️

I think there are still some issues with the helm documentation as for example the monitor key is getting the documentation for ipv4NativeRoutingCIDR (since that entire block is commented out)

[bmcustodio]

I think there are still some issues with the helm documentation as for example the monitor key is getting the documentation for ipv4NativeRoutingCIDR (since that entire block is commented out)

Thank you 👍 I'm going to check that one out!

[jibi]

test-me-please

[ldelossa]

Looks good to me.

I question the use of cidr.MustParseCIDR causing a panic while the rest of error conditions in config.go perform a normal logged error and exit the application. But this isn't a change in your PR.

[jibi]

Looks good to me.

I question the use of cidr.MustParseCIDR causing a panic while the rest of error conditions in config.go perform a normal logged error and exit the application. But this isn't a change in your PR.

Makes sense 👍 perhaps as a follow up we can replace MustParseCIDR which something that returns an error rather than panicking (and move the v4 range check inside that function)

[ldelossa]

Looks good to me.
I question the use of cidr.MustParseCIDR causing a panic while the rest of error conditions in config.go perform a normal logged error and exit the application. But this isn't a change in your PR.

Makes sense perhaps as a follow up we can replace MustParseCIDR which something that returns an error rather than panicking (and move the v4 range check inside that function)

That's my thinking, but low hanging fruit IMO. Nice to have.

[borkmann]

retest-1.21-4.9

[tklauser]

While backporting for 1.10, I noticed that this PR was marked needs-backport/1.10.

However, it deprecates an existing option in the first commit (("daemon: rename native-routing-cidr option to ipv4-native-routing-cidr")), which is probably not what we want to do in a stable release?

Given that the deprecation notice was added to the 1.11 upgrade notes, do we really want to backport the full PR to 1.10? Or was the intention just to backport the second commit (config: add validation for IPv4NativeRoutingCIDR)?

[joestringer]

@tklauser thanks for keeping an eye out for this, I agree that change should not be made in v1.10 as it can cause upgrade impact or break existing deployments during a point release.

@borkmann @jibi what was the intent here?

I'll drop the backport label until we figure this out.

[jibi]

@tklauser thanks for keeping an eye out for this, I agree that change should not be made in v1.10 as it can cause upgrade impact or break existing deployments during a point release.

@borkmann @jibi what was the intent here?

I'll drop the backport label until we figure this out.

@joestringer I marked this for backport as I thought the upgrade impact was minimal (i.e. only a warning that --native-routing-cidr is deprecated and that --ipv4-native-routing-cidr should be used instead).

What I just realized is that this has a non trivial downgrade impact: if for example a user starts on 1.10.2 following the documentation (using the new --ipv4-native-routing-cidr) and then downgrades to 1.10.1, the downgrade will break their setup as the flag does not exist in that release. So I agree with the decision of dropping the label (sorry for the noise)

[joestringer]

FWIW taking a look back over, while this could potentially catch some misconfiguration from users in a few circumstances, I don't think this PR really reaches the threshold where we should be backporting it. There is a cost of performing the backport and some degree of risk we take on. In this case I think it's OK for users to pick up these improvements with the next release.

For reference, the backport criteria definition is here: https://docs.cilium.io/en/latest/contributing/release/backports/#backport-criteria-for-current-minor-release.