Validate AntreaIPAM IP ranges #2995

ksamoray · 2021-11-07T13:25:11Z

Verify that:

There are no overlaps between an IPPool ranges while creating and
updating pools.
Validate that the gateway IP belongs to the same subnet as the IP
range addresses.

Signed-off-by: Kobi Samoray [email protected]

codecov-commenter · 2021-11-07T13:31:02Z

Codecov Report

Merging #2995 (cb5b3b6) into main (b147eb2) will decrease coverage by 6.10%.
The diff coverage is 57.65%.

@@            Coverage Diff             @@
##             main    #2995      +/-   ##
==========================================
- Coverage   60.89%   54.79%   -6.11%     
==========================================
  Files         268      374     +106     
  Lines       26753    51398   +24645     
==========================================
+ Hits        16292    28162   +11870     
- Misses       8655    20786   +12131     
- Partials     1806     2450     +644

Flag	Coverage Δ
e2e-tests	`53.47% <0.00%> (?)`
integration-tests	`35.77% <ø> (?)`
kind-e2e-tests	`48.53% <0.00%> (+0.85%)`	⬆️
unit-tests	`42.30% <84.21%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/ipam/validate.go	`57.95% <57.65%> (-22.05%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (-40.36%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`48.71% <0.00%> (-31.57%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`61.46% <0.00%> (-29.97%)`	⬇️
pkg/controller/egress/controller.go	`61.11% <0.00%> (-27.34%)`	⬇️
.../registry/networkpolicy/clustergroupmember/rest.go	`64.28% <0.00%> (-23.95%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`55.55% <0.00%> (-23.62%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go	`55.93% <0.00%> (-23.59%)`	⬇️
pkg/agent/util/ethtool/ethtool_linux.go	`46.66% <0.00%> (-23.34%)`	⬇️
pkg/agent/util/iptables/lock.go	`60.00% <0.00%> (-21.82%)`	⬇️
... and 336 more

pkg/controller/ipam/validate.go

annakhm

Can IP version validation for each range/cidr be added as well? All ranges should be consistent with IPPoolSpec.IPVersion

annakhm · 2021-11-09T21:52:32Z

pkg/controller/ipam/validate.go

+			return validationResult(false, msg)
+		}
+
+		if allowed {


Can allowed be false at this stage?

It's redundant, I'll remove.

pkg/controller/ipam/validate.go

tnqn · 2021-11-17T05:18:42Z

pkg/controller/ipam/validate.go

+		_, cidr, _ := net.ParseCIDR(r.CIDR)
+		if !cidr.Contains(gateway) {
+			return false, fmt.Sprintf(
+				"Range is invalid. Gateway %s is unreachable from CIDR %s", r.Gateway, r.CIDR)


In my understanding, as the feature requires the Pod CIDR is in the same subnet of Node IP addresses. The CIDR will be a subset of the underlay subnet and the gateway will not be within the CIDR. For example, the underlay subnet is 192.168.0.0/22, the Node CIDR is 192.168.0.0/24, the Pod CIDR is 192.168.1.0/24, but the gateway 192.168.0.1 is not within 192.168.1.0/24.
I think it should check PodCIDR/r.PrefixLength contains the gateway IP.
@GraysonWu @annakhm could you confirm?

I should @gran-vmv, not Grayson.

I think it should be checked.
@annakhm Do you think we need to check it here?

antrea/pkg/ipam/poolallocator/allocator.go

Line 73 in e0e2e07

reservedIPs := []net.IP{net.ParseIP(ipRange.SubnetInfo.Gateway)}

@gran-vmv is my understanding correct? We should check whether PodCIDR/PrefixLength contains the gateway IP, instead of the provided PodCIDR itself, right?

I believe we should check that subnet CIDR defined by SubnetInfo.Gateway/SubnetInfo.PrefixLength should contain the CIDR/range that we are validating.

pkg/controller/ipam/validate.go

tnqn · 2021-11-18T17:02:37Z

pkg/controller/ipam/validate_test.go

+			},
+		},
+		{
+			name: "CREATE operation with CIDR overlap with IP range should not be allowed",


what's the difference between the above one and this one?

1st one intersect while the 2nd one is contained. I added the latter as your review comment above was that the latter case is not covered by the validation. If you think that one of these is redundant, I can remove it.

The two cases look good to me. I just didn't tell the difference from the name of the cases. Could we make the name a bit more explict to explain the difference?

pkg/controller/ipam/validate.go

annakhm · 2021-11-19T17:56:36Z

pkg/controller/ipam/validate.go

+
+func validateIPRange(r crdv1alpha2.SubnetIPRange, poolIPVersion int) (bool, string) {
+	// Validate the integrity of IPs within the IP range
+	gateway := net.ParseIP(r.Gateway)


Just to save 4 lines of code, can we validate GW IP version here and rely on further checks to fail if IP version differs for other attributes?

tnqn

LGTM, a minor comment about test case name.

tnqn · 2021-11-25T03:32:04Z

pkg/controller/ipam/validate_test.go

+			},
+		},
+		{
+			name: "CREATE operation with CIDR overlap with IP range should not be allowed",


The two cases look good to me. I just didn't tell the difference from the name of the cases. Could we make the name a bit more explict to explain the difference?

ksamoray · 2021-11-25T12:01:47Z

/test-all

tnqn

LGTM, will wait for @annakhm's final review before merging.

ksamoray · 2021-11-28T09:23:17Z

/test-all

annakhm · 2021-11-30T22:33:01Z

LGTM

ksamoray · 2021-12-01T12:31:03Z

/test-all

tnqn

LGTM

tnqn · 2021-12-14T11:29:39Z

Sorry, wait too long to merge this PR. Will merge it once CI is recovered.

Verify that: - There are no overlaps between an IPPool ranges while creating and updating pools. - Validate that the gateway IP belongs to the same subnet as the IP range addresses. Signed-off-by: Kobi Samoray <[email protected]>

ksamoray · 2022-02-28T22:14:39Z

@tnqn this has been sitting for a while, mind having a look again (no changes whatsoever, I've just rebased it).

tnqn

Sorry @ksamoray, I missed this one since last comment.

Verify that: - There are no overlaps between an IPPool ranges while creating and updating pools. - Validate that the gateway IP belongs to the same subnet as the IP range addresses. Signed-off-by: Kobi Samoray <[email protected]>

ksamoray requested review from annakhm and salv-orlando November 7, 2021 13:25

ksamoray force-pushed the ipam-validate branch 3 times, most recently from 9126a38 to 560ec3f Compare November 9, 2021 10:12

annakhm reviewed Nov 9, 2021

View reviewed changes

pkg/controller/ipam/validate.go Show resolved Hide resolved

annakhm reviewed Nov 9, 2021

View reviewed changes

ksamoray force-pushed the ipam-validate branch 3 times, most recently from d08ef44 to 87b2e3f Compare November 15, 2021 09:27

ksamoray requested a review from annakhm November 16, 2021 19:12

tnqn reviewed Nov 17, 2021

View reviewed changes

ksamoray force-pushed the ipam-validate branch 2 times, most recently from 8811652 to 85ab7f9 Compare November 18, 2021 12:23

ksamoray requested a review from tnqn November 18, 2021 14:28

tnqn reviewed Nov 18, 2021

View reviewed changes

annakhm reviewed Nov 19, 2021

View reviewed changes

ksamoray force-pushed the ipam-validate branch from 85ab7f9 to 5c82de5 Compare November 24, 2021 09:25

tnqn reviewed Nov 25, 2021

View reviewed changes

ksamoray force-pushed the ipam-validate branch 2 times, most recently from 9800a09 to 9560fd8 Compare November 25, 2021 10:25

tnqn previously approved these changes Nov 26, 2021

View reviewed changes

annakhm previously approved these changes Nov 27, 2021

View reviewed changes

ksamoray dismissed stale reviews from annakhm and tnqn via 9dc67b0 November 28, 2021 09:29

ksamoray force-pushed the ipam-validate branch 2 times, most recently from 9dc67b0 to 5394ae2 Compare November 30, 2021 12:35

ksamoray force-pushed the ipam-validate branch from 5394ae2 to 18fa854 Compare December 14, 2021 07:43

tnqn previously approved these changes Dec 14, 2021

View reviewed changes

annakhm mentioned this pull request Feb 28, 2022

[Antrea-IPAM] Support pre-allocating continuous IPs for StatefulSet #3281

Merged

Validate AntreaIPAM IP ranges

cb5b3b6

Verify that: - There are no overlaps between an IPPool ranges while creating and updating pools. - Validate that the gateway IP belongs to the same subnet as the IP range addresses. Signed-off-by: Kobi Samoray <[email protected]>

ksamoray dismissed tnqn’s stale review via cb5b3b6 February 28, 2022 20:48

ksamoray force-pushed the ipam-validate branch from 18fa854 to cb5b3b6 Compare February 28, 2022 20:48

ksamoray requested a review from tnqn February 28, 2022 22:14

tnqn approved these changes Mar 1, 2022

View reviewed changes

tnqn merged commit b85d2c0 into antrea-io:main Mar 1, 2022

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 1, 2022

ksamoray deleted the ipam-validate branch August 2, 2022 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate AntreaIPAM IP ranges #2995

Validate AntreaIPAM IP ranges #2995

ksamoray commented Nov 7, 2021 •

edited

Loading

codecov-commenter commented Nov 7, 2021 •

edited

Loading

annakhm left a comment

annakhm Nov 9, 2021

ksamoray Nov 10, 2021

tnqn Nov 17, 2021

tnqn Nov 18, 2021

gran-vmv Nov 19, 2021 •

edited

Loading

tnqn Nov 19, 2021

annakhm Nov 19, 2021

tnqn Nov 18, 2021

ksamoray Nov 21, 2021

tnqn Nov 25, 2021

annakhm Nov 19, 2021

tnqn left a comment

tnqn Nov 25, 2021

ksamoray commented Nov 25, 2021

tnqn left a comment

ksamoray commented Nov 28, 2021

annakhm commented Nov 30, 2021

ksamoray commented Dec 1, 2021

tnqn left a comment

tnqn commented Dec 14, 2021

ksamoray commented Feb 28, 2022

tnqn left a comment

Validate AntreaIPAM IP ranges #2995

Validate AntreaIPAM IP ranges #2995

Conversation

ksamoray commented Nov 7, 2021 • edited Loading

codecov-commenter commented Nov 7, 2021 • edited Loading

Codecov Report

annakhm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gran-vmv Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksamoray commented Nov 25, 2021

tnqn left a comment

Choose a reason for hiding this comment

ksamoray commented Nov 28, 2021

annakhm commented Nov 30, 2021

ksamoray commented Dec 1, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Dec 14, 2021

ksamoray commented Feb 28, 2022

tnqn left a comment

Choose a reason for hiding this comment

ksamoray commented Nov 7, 2021 •

edited

Loading

codecov-commenter commented Nov 7, 2021 •

edited

Loading

gran-vmv Nov 19, 2021 •

edited

Loading