calico
calico copied to clipboard
projectcalico
Fix announcement of /32 and /128 entries for serviceLoadBalancerIPs.
Description
After this patch, single IPs are no longer announced as global routes.
The patch also fixes a bug related to config change, causing one or more route to be withdrawn if changing from for example /32 to /29 in BGPConfiguration. Now setRoutesForKey() assures that routes it thinks are advertised actually do exist.
I also built a version of the 3.21 release with this included: docker.io/unifon/caliconode:v3.21.4-singleip2
Related issues/PRs
fixes https://github.com/projectcalico/calico/issues/6074
Todos
- [ ] Tests
- [ ] Documentation
- [ ] Release note
Release Note
Fix that single-IP entries on BGPConfiguration LoadBalancerIPs were not advertised according to external traffic policy.
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one docs-* label.
docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.
Every PR needs one release-note-* label.
release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.
Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.
I finally had time to upgrade the kubernetes cluster and test this after rebase. Seems to work as intended (both changes).
Do you got a clear idea of what the tests should include? Seems like there's no tests at all for services of type loadbalancer right now, so it's a little bit hard to know where to start.
Do you got a clear idea of what the tests should include? Seems like there's no tests at all for services of type loadbalancer right now, so it's a little bit hard to know where to start.
Not sure why CI didn't want to run but I triggered it again.
@mtryfoss I'm guessing some of this new logic could be tested in https://github.com/projectcalico/calico/blob/master/confd/pkg/backends/calico/routes_test.go. Just create a LoadBalancer service type.
@caseydavenport did you have anything specific tests in mind?
Yep, sorry I somehow missed the original ping on this.
This test in particular is probably pretty close to what we want here: https://github.com/projectcalico/calico/blob/master/confd/pkg/backends/calico/routes_test.go#L477-L480
The main difference being that it makes its assertions on ExternalIPs rather than LoadBalancerIPs.
But making a copy of that and adjusting expectations should do the trick. i.e.,
- Simulate setting the LB IPs from BGPConfiguration to a single /32.
- See that it is not advertised.
- Resync, triggering the route generator to spot the serivce.
- See that the route is now programmed
- Change the endpoints for the service so it no longer matches the test node
- See that the route is withdrawn.
This is the meat of the logic you can base it off of: https://github.com/projectcalico/calico/blob/master/confd/pkg/backends/calico/routes_test.go#L496-L508
Sorry for late reply, but here's my approach which is based on the existing tests.