[FIX] Validate provided addresses are network addresses #1065

abhinavdahiya · 2019-01-15T01:00:33Z

Before this change validation allowed CIDRs with network IP set to any IP address in the range.
This change enforces that CIDR notation use network IP.

example,

...
networking:
  machineCIDR: 192.168.126.10/24
...

The installer now errors on create cluster

FATAL failed to fetch Terraform Variables: failed to load asset "Install Config": invalid "install-config.yaml" file: networking.machineCIDR: Invalid value: "192.168.126.10/24": invalid network address. got 192.168.126.10/24, expecting 192.168.126.0/24

/cc @crawford @wking
xref: https://jira.coreos.com/browse/CORS-961
Fixes #1023

wking · 2019-01-15T01:24:59Z

pkg/ipnet/ipnet.go

@@ -50,7 +50,7 @@ func (ipnet *IPNet) UnmarshalJSON(b []byte) (err error) {
 		return errors.Wrap(err, "failed to Unmarshal string")
 	}

-	ip, net, err := net.ParseCIDR(cidr)
+	_, net, err := net.ParseCIDR(cidr)


I'd like to preserve this return value so you can specify an IP within a subnet as well as the subnet itself.

@wking
i tried to explain my reasoning in commit message 28494bb

before this change

ip0, ipnet0, _ := net.ParseCIDR("192.168.0.10/24") ip0 => "192.168.0.10" ipnet0 = > "192.168.0.0/24" json.Unmarshal([]byte(`"192.168.0.10/24"`), &ipnet1) ipnet1 => "192.168.0.10/24" ipnet0 != ipnet1 when the input was the same...

which i think incorrect for expected bahavior

installer/pkg/ipnet/ipnet.go

Line 16 in 63bdb7f

// IPNet wraps net.IPNet to get CIDR serialization.

with this change

ip0, ipnet0, _ := net.ParseCIDR("192.168.0.10/24") ip0 => "192.168.0.10" ipnet0 = > "192.168.0.0/24" json.Unmarshal([]byte(`"192.168.0.10/24"`), &ipnet1) ipnet1 => "192.168.0.0/24" ipnet0 == ipnet1 when the input was the same...

which i think incorrect for expected bahavior

// IPNet wraps net.IPNet to get CIDR serialization.

I think you're focusing too much on ParseCIDR's returned network, when the IPNet wrapper is primarily wrapping net.IPNet to add CIDR (de)serialization. Matching existing net.IPNet behavior like round-tripping is part of that (e.g. see this play). If you decide you do need the lowest IP in an IPNet range, you can always use yourIPNet.IP.Mask(yourIPNet.Mask) to recover it (as shown in the play). Conversely, if we throw out the IP and jump straight to the masked IP on deserialization, we break round-tripping and there's no way to recover the initial IP offset.

I don't think so.
i think we should minic the behavior net.IPNet String (serialize) and ParseCIDR (deserialize) for our json serialize/deserialize behavior.

i think we should minic the behavior net.IPNet String (serialize)...

Sounds good to me. And also JSON (de)serialization. But see here for where your implementation (at least as of 2a1bf26) diverges for JSON deserialization. While the master implementation (as of 7452eed) matches in all cases.

Moving this commit to #1106.
This is not required to fix the validation issue.

wking · 2019-01-15T01:27:43Z

pkg/ipnet/ipnet_test.go

+	for _, test := range tests {
+		t.Run(test.input, func(t *testing.T) {
+			var ipNetOut IPNet
+			err := json.Unmarshal([]byte(test.input), &ipNetOut)
 			if err != nil {


Instead of switching on the err under test, this should switch on the input test case. Something like:

if test.expErr == "" { assert.NoError(t, err) assert.Equal(t, text.exp, &ipNetOut) } else { asset.EqualError(t, err, test.expErr) }

done with cffbc5e -> 28494bb

wking · 2019-01-22T05:32:32Z

pkg/validate/validate_test.go

-			if tc.valid {
-				assert.NoError(t, err)
+			err = SubnetCIDR(&net.IPNet{IP: ip, Mask: cidr.Mask})
+			if err != nil {


This is still switching on the returned error and not on a testcase property [edit: actually, this is different code, but same issue ;)]. That can cause false negatives, e.g. if ParseCIDR fails to return an expected error, then we'll hit the err == nil case and execute the tautological assert.NoError(t, err). I've pushed git://github.com/wking/openshift-installer.git subnet-cidr-test-fix (4c9deae) with a fixup to switch on the expected error instead. Can you take a look and squash that into your commit if it looks good to you (or tell me why it doesn't look good ;)?

done with df680a1 -> 5c29e90

Before this change validation allowed CIDRs with network IP set to any IP address in the range. This change enforces that CIDR notation use network IP. example, ```yaml ... networking: machineCIDR: 192.168.126.10/24 ... ``` The installer now errors on `create cluster` ```console FATAL failed to fetch Terraform Variables: failed to load asset "Install Config": invalid "install-config.yaml" file: networking.machineCIDR: Invalid value: "192.168.126.10/24": invalid network address. got 192.168.126.10/24, expecting 192.168.126.0/24 ```

abhinavdahiya · 2019-01-22T18:38:21Z

/retest

abhinavdahiya · 2019-01-24T01:24:12Z

ping @crawford @wking

/retest

wking

/lgtm

wking · 2019-02-01T22:41:34Z

/retest

openshift-ci-robot · 2019-02-01T22:41:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

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:

~~OWNERS~~ [abhinavdahiya,wking]

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

wking · 2019-02-02T00:47:26Z

e2e-aws:

test "release-latest" failed: pod release-latest was already deleted

/retest

openshift-ci-robot requested review from crawford and wking January 15, 2019 01:00

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2019

abhinavdahiya changed the title ~~Validate provided addresses are network addresses~~ [FIX] Validate provided addresses are network addresses Jan 15, 2019

wking reviewed Jan 15, 2019

View reviewed changes

abhinavdahiya force-pushed the fix-1023 branch from 85bacba to 2a1bf26 Compare January 15, 2019 17:29

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2019

abhinavdahiya force-pushed the fix-1023 branch from 2a1bf26 to df680a1 Compare January 22, 2019 01:29

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2019

abhinavdahiya mentioned this pull request Jan 22, 2019

pkg/ipnet: fix Unmarshal to match ParseCIDR output #1106

Closed

wking reviewed Jan 22, 2019

View reviewed changes

abhinavdahiya force-pushed the fix-1023 branch from df680a1 to 5c29e90 Compare January 22, 2019 17:20

wking approved these changes Feb 1, 2019

View reviewed changes

openshift-ci-robot assigned wking Feb 1, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019

openshift-merge-robot merged commit 0316257 into openshift:master Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Validate provided addresses are network addresses #1065

[FIX] Validate provided addresses are network addresses #1065

abhinavdahiya commented Jan 15, 2019 •

edited

wking Jan 15, 2019

abhinavdahiya Jan 15, 2019

wking Jan 16, 2019

abhinavdahiya Jan 16, 2019

wking Jan 17, 2019

abhinavdahiya Jan 22, 2019

wking Jan 15, 2019

abhinavdahiya Jan 15, 2019 •

edited

wking Jan 22, 2019 •

edited

abhinavdahiya Jan 22, 2019

abhinavdahiya commented Jan 22, 2019

abhinavdahiya commented Jan 24, 2019

wking left a comment

wking commented Feb 1, 2019

openshift-ci-robot commented Feb 1, 2019

wking commented Feb 2, 2019

[FIX] Validate provided addresses are network addresses #1065

[FIX] Validate provided addresses are network addresses #1065

Conversation

abhinavdahiya commented Jan 15, 2019 • edited

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

abhinavdahiya Jan 15, 2019 • edited

Choose a reason for hiding this comment

wking Jan 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Jan 22, 2019

abhinavdahiya commented Jan 24, 2019

wking left a comment

Choose a reason for hiding this comment

wking commented Feb 1, 2019

openshift-ci-robot commented Feb 1, 2019

wking commented Feb 2, 2019

abhinavdahiya commented Jan 15, 2019 •

edited

abhinavdahiya Jan 15, 2019 •

edited

wking Jan 22, 2019 •

edited