Skip to content
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.

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

pkg/k8s: delete toCIDRSets for more than 2 endpoints #11901

Merged
merged 1 commit into from Jun 4, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Jun 4, 2020

In case a toServices selected a service that contained more than 1
endpoint, the generated rules could never be deleted. This can easily be
reproducible by adding one more endpoint to the unit test of the
deleteToCidrFromEndpoint function.

Fixes: bae09f7 ("Move ToCIDR gen logic to k8s package")
Signed-off-by: André Martins andre@cilium.io

Avoid duplication of generated toCIDRs when using a toServices based CNP (or CCNP)

The PR for the v1.6 branch is made separately since the code is slightly different: #11900

@aanm aanm added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. needs-backport/1.7 labels Jun 4, 2020
@aanm aanm requested a review from a team June 4, 2020 19:39
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 4, 2020
@aanm aanm force-pushed the pr/avoid-duplicate-cidrsets branch from b07f933 to a4af651 Compare June 4, 2020 19:50
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm wondering if we should extend the end of this test to further validate behaviour after deletion (and also re-add).

pkg/k8s/rule_translate.go Outdated Show resolved Hide resolved
pkg/k8s/rule_translate.go Outdated Show resolved Hide resolved
In case a toServices selected a service that contained more than 1
endpoint, the generated rules could never be deleted. This can easily be
reproducible by adding one more endpoint to the unit test of the
deleteToCidrFromEndpoint function.

Fixes: bae09f7 ("Move ToCIDR gen logic to k8s package")
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm requested a review from jrajahalme June 4, 2020 20:13
@aanm aanm force-pushed the pr/avoid-duplicate-cidrsets branch from a4af651 to 89c03d4 Compare June 4, 2020 20:13
@coveralls
Copy link

coveralls commented Jun 4, 2020

Coverage Status

Coverage increased (+0.04%) to 36.954% when pulling 89c03d4 on pr/avoid-duplicate-cidrsets into f5a7682 on master.

@aanm
Copy link
Member Author

aanm commented Jun 4, 2020

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.6 in 1.6.9 Jun 4, 2020
@joestringer
Copy link
Member

v1.6 backport for reference: #11900

@jrajahalme
Copy link
Member

jrajahalme commented Jun 4, 2020

The lone CI fail is a known flake (#11895)

@joestringer joestringer merged commit 90217c1 into master Jun 4, 2020
1.8.0 automation moved this from In progress to Merged Jun 4, 2020
@joestringer joestringer deleted the pr/avoid-duplicate-cidrsets branch June 4, 2020 23:16
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.9 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 6, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
1.6.9
Backport done to v1.6
1.7.5
Backport pending to v1.7
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

5 participants