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.

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

Aggregate overlapping allowed source ranges #6807

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Aggregate overlapping allowed source ranges #6807

merged 2 commits into from
Aug 30, 2024

Conversation

zarvd
Copy link
Contributor

@zarvd zarvd commented Aug 12, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR introduces a prefix tree to aggregate overlapping allowed source ranges. Without it, customers have to merge the CIDR manually; otherwise, the Azure NSG operation will fail.

Which issue(s) this PR fixes:

Fixes #6775

Special notes for your reviewer:

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/cloud-provider-azure/pkg/provider/loadbalancer/iputil
BenchmarkAggregatePrefixes/IPv4-100-10             72164             16502 ns/op  // with 100 IPv4 CIDRs
BenchmarkAggregatePrefixes/IPv6-100-10             71418             16706 ns/op
BenchmarkAggregatePrefixes/IPv4-IPv6-200-10        22771             52537 ns/op
BenchmarkAggregatePrefixes/IPv4-1000-10             7576            160325 ns/op  // with 1k IPv4 CIDRs
BenchmarkAggregatePrefixes/IPv6-1000-10             7062            159379 ns/op
BenchmarkAggregatePrefixes/IPv4-IPv6-2000-10        2426            494645 ns/op
BenchmarkAggregatePrefixes/IPv4-10000-10             537           2214890 ns/op
BenchmarkAggregatePrefixes/IPv6-10000-10             538           2220426 ns/op
BenchmarkAggregatePrefixes/IPv4-IPv6-20000-10        192           6245388 ns/op // with 20k IPv4 + IPv6 CIDRs
BenchmarkPrefixTree_Add/IPv4-10                  8045304               151.6 ns/op
BenchmarkPrefixTree_Add/IPv6-10                  4094680               289.5 ns/op
BenchmarkPrefixTree_List/IPv4-10                  568363              1909 ns/op
BenchmarkPrefixTree_List/IPv6-10                  547520              2339 ns/op
PASS
ok      sigs.k8s.io/cloud-provider-azure/pkg/provider/loadbalancer/iputil       18.471s

I guess the performance of this operation is acceptable for now. It’s extremely rare to find a service that allows more than 20,000 CIDRs anyway.

Does this PR introduce a user-facing change?

Aggregate overlapping allowed source ranges

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 12, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @zarvd. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 12, 2024
@nilo19
Copy link
Contributor

nilo19 commented Aug 13, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2024
@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch 3 times, most recently from 3c418b5 to 54d1b8c Compare August 14, 2024 02:38
@zarvd
Copy link
Contributor Author

zarvd commented Aug 14, 2024

/retest

@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch from 54d1b8c to f875524 Compare August 15, 2024 03:15
@zarvd
Copy link
Contributor Author

zarvd commented Aug 15, 2024

/retest

1 similar comment
@zarvd
Copy link
Contributor Author

zarvd commented Aug 15, 2024

/retest

@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch 4 times, most recently from 60716b2 to 739d43b Compare August 20, 2024 08:24
@nilo19
Copy link
Contributor

nilo19 commented Aug 21, 2024

Can we add an e2e?

@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch from 739d43b to c345c2c Compare August 21, 2024 05:22
zarvd
zarvd commented Aug 21, 2024
View reviewed changes
tests/e2e/network/network_security_group.go
@@ -619,6 +640,21 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
allowedIPv6Ranges = []string{
"2c0f:fe40:8000::/48", "2c0f:feb0::/43",
}

// The overlapping IP ranges will be aggregated after reconciled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nilo19 Added some overlapping CIDRs to the existing cases. Let’s see how it goes.

@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch 2 times, most recently from 5982eae to b21011e Compare August 21, 2024 14:46
@zarvd
Copy link
Contributor Author

zarvd commented Aug 22, 2024

/retest

1 similar comment
@zarvd
Copy link
Contributor Author

zarvd commented Aug 22, 2024

/retest

@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch from b21011e to b278399 Compare August 23, 2024 07:52
@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch 9 times, most recently from 266ab0c to 4845c29 Compare August 30, 2024 00:49
@zarvd zarvd force-pushed the fix/loadbalancer/overlapping-source-ranges branch from 4845c29 to 6b01530 Compare August 30, 2024 05:09
feiskyer
feiskyer approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, zarvd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2024
@feiskyer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8095ff0 into kubernetes-sigs:master Aug 30, 2024
18 checks passed
@zarvd zarvd deleted the fix/loadbalancer/overlapping-source-ranges branch August 30, 2024 08:46
This was referenced Aug 31, 2024
Merged
Merged
Merged
@zarvd zarvd mentioned this pull request Sep 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer approved these changes

@nilo19 nilo19 Awaiting requested review from nilo19

Assignees

@feiskyer feiskyer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OverlappingSubnetsNotPermittedInSecurityRule occurs when using overlapping CIDR as allowed source ranges
4 participants
@zarvd @k8s-ci-robot @nilo19 @feiskyer