Skip to content

egressgw: Stop CEGP parsing in case of non-empty invalid EgressIP #32868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 7, 2024
Merged

egressgw: Stop CEGP parsing in case of non-empty invalid EgressIP #32868

merged 2 commits into from
Jun 7, 2024

Conversation

pippolo84
Copy link
Member

EgressIP field of CiliumEgressGatewayPolicy spec is optional, but if specified, it is used to SNAT egress traffic. Being an optional parameter, no error is logged in case the conversion to netip.Addr fails, and the field is silently ignored.

To inform the user of the failure in setting the requested egress IP, log a warning in case of an invalid non-empty egress IP.

@pippolo84 pippolo84 added release-note/misc This PR makes changes that have no direct user impact. feature/egress-gateway Impacts the egress IP gateway feature. labels Jun 4, 2024
@pippolo84 pippolo84 requested a review from a team as a code owner June 4, 2024 09:16
@pippolo84 pippolo84 requested a review from jschwinger233 June 4, 2024 09:16
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cegp-invalid-egress-ip-warn branch from 6f092fb to abb1e2c Compare June 4, 2024 09:25
@julianwiedmann
Copy link
Member

hmm, how does this match up with https://github.com/pippolo84/cilium/blob/355f2ffef29ad30fbdfcbf3ff20b5356e7bc994e/pkg/egressgateway/policy.go#L224 ?

@pippolo84
Copy link
Member Author

hmm, how does this match up with https://github.com/pippolo84/cilium/blob/355f2ffef29ad30fbdfcbf3ff20b5356e7bc994e/pkg/egressgateway/policy.go#L224 ?

That parsing swallows the error without giving the user any sign about a possible mistake in specifying the address. With this change, ParseCEGP still won't break up in case of an error, but then the user will be informed through the warning that the manager will ignore the field, despite being non-empty.

An alternative might be to stop the parsing with an error in case EgressIP is both non empty and invalid. But this would introduce a non backward-compatible change that might be harmful. In the end, a warning should be the best trade off.

@julianwiedmann
Copy link
Member

hmm, how does this match up with https://github.com/pippolo84/cilium/blob/355f2ffef29ad30fbdfcbf3ff20b5356e7bc994e/pkg/egressgateway/policy.go#L224 ?

That parsing swallows the error without giving the user any sign about a possible mistake in specifying the address. With this change, ParseCEGP still won't break up in case of an error, but then the user will be informed through the warning that the manager will ignore the field, despite being non-empty.

Ah and that's ok, because deriveFromPolicyGatewayConfig() later does an additional gc.egressIP.IsValid() check.

I think we can be more fool-proof, and move that specific IsValid() check into ParseCEGP() instead - so any potential user of the gc.egressIP field understands that it wasn't set with a valid IP.

An alternative might be to stop the parsing with an error in case EgressIP is both non empty and invalid. But this would introduce a non backward-compatible change that might be harmful. In the end, a warning should be the best trade off.

I think we can definitely expect some sanity from the user, as the egressIP has a kubebuilder validation pattern attached to it. So rejecting an invalid egressIP in ParseCEGP() feels doable?

@pippolo84
Copy link
Member Author

I think we can definitely expect some sanity from the user, as the egressIP has a kubebuilder validation pattern attached to it. So rejecting an invalid egressIP in ParseCEGP() feels doable?

Yeah, looking at the kubebuilder pattern I can't find any case where an invalid IP can pass the validation, so it is probably enough to reject the address and fail the entire parsing if, for some reason, something that the netip package doesn't like ends up there.

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cegp-invalid-egress-ip-warn branch 2 times, most recently from 2287eda to ce8a372 Compare June 4, 2024 13:24
@pippolo84 pippolo84 changed the title egressgw: Log a warning in case of non-empty invalid EgressIP egressgw: Stop CEGP parsing in case of non-empty invalid EgressIP Jun 4, 2024
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cegp-invalid-egress-ip-warn branch from ce8a372 to bdfb95e Compare June 4, 2024 17:09
@julianwiedmann
Copy link
Member

I think we can definitely expect some sanity from the user, as the egressIP has a kubebuilder validation pattern attached to it. So rejecting an invalid egressIP in ParseCEGP() feels doable?

Yeah, looking at the kubebuilder pattern I can't find any case where an invalid IP can pass the validation, so it is probably enough to reject the address and fail the entire parsing if, for some reason, something that the netip package doesn't like ends up there.

I'll do you one better - could we also update the kubebuilder to use +kubebuilder:validation:Format=ipv4, and get rid of that ugly regex? 😄

@pippolo84
Copy link
Member Author

I'll do you one better - could we also update the kubebuilder to use +kubebuilder:validation:Format=ipv4, and get rid of that ugly regex? 😄

Definitely! 👍

@pippolo84 pippolo84 requested review from a team as code owners June 5, 2024 08:34
@pippolo84 pippolo84 requested review from learnitall and squeed June 5, 2024 08:34
@julianwiedmann
Copy link
Member

/test

julianwiedmann
julianwiedmann approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thank you!

@julianwiedmann
Copy link
Member

💡 The ipsec-e2e fail needs a rebase to pick up #31757.

pippolo84 added 2 commits June 7, 2024 11:04
@pippolo84
egressgw: Stop CEGP parsing in case of non-empty invalid EgressIP
f3652aa 
EgressIP field of CiliumEgressGatewayPolicy spec is optional, but if
specified, it is used to SNAT egress traffic.  Being an optional
parameter, no error is logged in case the conversion to netip.Addr
fails, and the field is silently ignored.

To inform the user of the failure in setting the requested egress IP,
fail the CEGP parsing in case of an invalid non-empty egress IP.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: cegp: Use ipv4 as validation format for EgressIP
d2f9aa9 
Instead of relying on a regex based kubebuilder validation pattern, use
the ipv4 format to validate EgressIP field in CEGP.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cegp-invalid-egress-ip-warn branch from e2bcfba to d2f9aa9 Compare June 7, 2024 09:04
@pippolo84
Copy link
Member Author

/test

@pippolo84
Copy link
Member Author

Conformance Ingress failure tracked here, rerunning

@julianwiedmann julianwiedmann enabled auto-merge June 7, 2024 13:00
@julianwiedmann julianwiedmann added this pull request to the merge queue Jun 7, 2024
Merged via the queue into cilium:main with commit 4dc5898 Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@jschwinger233 jschwinger233 Awaiting requested review from jschwinger233 jschwinger233 is a code owner automatically assigned from cilium/egress-gateway

@learnitall learnitall Awaiting requested review from learnitall learnitall is a code owner automatically assigned from cilium/api

Assignees
No one assigned
Labels
feature/egress-gateway Impacts the egress IP gateway feature. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pippolo84 @julianwiedmann @squeed