Bug 1918469: Check if VIP IPs overlap with machine CIDR provided during vsphere installation #4754

rna-afk · 2021-03-15T19:52:42Z

If the machineCIDR is specified by the user, the VIP IPs provided
must overlap with the CIDRs provided and the installation fails
if they do not.

Adding a validation check to see if the CIDR is provided and if so,
checks to see if the IPs are within any of the machine CIDRs provided.

openshift-ci-robot · 2021-03-15T19:52:45Z

@rna-afk: This pull request references Bugzilla bug 1918469, which is invalid:

expected the bug to target the "4.8.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1918469: Check if VIP IPs overlap with machine CIDR provided

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/test-infra repository.

rna-afk · 2021-03-15T19:53:54Z

/assign @staebler

rna-afk · 2021-03-15T19:54:23Z

/bugzilla refresh

openshift-ci-robot · 2021-03-15T19:54:30Z

@rna-afk: This pull request references Bugzilla bug 1918469, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jinyunma

Details

In response to this:

/bugzilla refresh

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/test-infra repository.

staebler

Update the PR title and the commit message to reflect that this is for vSphere.

staebler · 2021-03-16T01:23:55Z

pkg/types/vsphere/validation/platform.go

Networking must never be nil. So rather than ignore a nil, at least add an error for it.

staebler · 2021-03-16T01:25:04Z

pkg/types/vsphere/validation/platform.go

Overlap is not the right term. Overlap is used for sets. What you want to say is that the machine CIDR must contain the VIP.

Suggested change

allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "must overlap with the machine CIDRs provided"))

allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "must be contained within one of the machine networks"))

staebler · 2021-03-16T01:34:45Z

pkg/types/vsphere/validation/platform.go

Do all of this contain checking in validateVIPs. That function is already parsing the string into an IP, albeit within the call to validate.IP.

This makes it a bit more complicated for the call to validateVIPs from ValidateForProvisioning. But I would argue that ValidateForProvisioning should not be doing full validation of the VIPs. It should only be validating that the VIPs have been set. If the VIPs are set, then the full validation would have been done doing the initial validation of the install config. If the VIPs are not set, then validateVIPs will fail and will not have anything more to validate beyond that the VIPs are missing anyway. What do you think, @patrickdillon (#3372)?

This makes it a bit more complicated for the call to validateVIPs from ValidateForProvisioning. But I would argue that ValidateForProvisioning should not be doing full validation of the VIPs. It should only be validating that the VIPs have been set. If the VIPs are set, then the full validation would have been done doing the initial validation of the install config. If the VIPs are not set, then validateVIPs will fail and will not have anything more to validate beyond that the VIPs are missing anyway. What do you think, @patrickdillon (#3372)?

I agree we should move this logic to validateVIPs. And if I understand correctly, I am fine to refactor ValidateForProvisioning so it only checks for the presence of the required VIPs fields and does not do the full validation.

To frame the discussion a bit more for @rna-afk vSphere UPI was released before IPI. When UPI was released VIPs were not required in the platform, but they are required for IPI. We cannot require VIPs in the platform as it would break backwards compatibility with UPI. Therefore we added the check here in ValidateForProvisioning to distinguish the validation only run for IPI.

If I understand @staebler's concern correctly, we want to avoid a slippery slope of allowing validation creeping in here, where it does not really belong.

patrickdillon · 2021-03-16T12:57:10Z

pkg/types/vsphere/validation/platform.go

Performance is not going to be an issue here (MachineNetwork will be a very small array) so I would opt for readability rather than optimizing performance. I would suggest losing the boolean flags like apiVIPOverlap and creating a helper function that returns bool if the MachineNetwork contains the VIP. You would call this helper function where you check the flags.

staebler · 2021-03-16T17:58:05Z

pkg/types/vsphere/validation/platform.go

if network == nil { // add an error and return. This should not happen, so let's not make the function more complex with nested ifs to cover the case. } // deal with the happy scenario first if p.APIVIP != "" { ip := net.ParseIP(p.APIVIP) if ip != nil { if !validateIPInCIDR(ip, network.MachineNetwork) { // add error } } else { // add error } } else { // add error } // repeat for IngressVIP

staebler · 2021-03-16T18:00:58Z

pkg/types/vsphere/validation/platform.go

This check should not be done here. In ValidateForProvisioning, the two VIPs are required. The suggestion that I was trying to make was to only validate that both VIPs were provided here rather than doing all of the other validation for the VIPs, since the other validation will necessarily have already been done.

Suggested change

if strings.Join([]string{p.APIVIP, p.IngressVIP}, "") != "" {

allErrs = append(allErrs, validateVIPs(p, network, fldPath)...)

}

if p.APIVIP == "" {

// add error

}

if p.IngressVIP == "" {

// add error

}

patrickdillon · 2021-03-16T19:00:37Z

pkg/types/vsphere/validation/platform.go

validateIPinCIDR -> ipInNetwork
nit: comment could be be slightly improved `ipInNetwork returns true if the given..."

staebler

Sorry to keep piling on with the nits.

staebler · 2021-03-16T23:38:49Z

pkg/types/vsphere/validation/platform.go

network is unused now.

Suggested change

func ValidateForProvisioning(p *vsphere.Platform, network *types.Networking, fldPath *field.Path) field.ErrorList {

func ValidateForProvisioning(p *vsphere.Platform, fldPath *field.Path) field.ErrorList {

staebler · 2021-03-16T23:39:52Z

pkg/types/vsphere/validation/platform.go

Suggested change

// ipInNetwork return true if the given ip is within the machine CIDRs.

// ipInNetwork return true if the given ip is within one of the machine networks.

staebler · 2021-03-16T23:41:32Z

pkg/types/vsphere/validation/platform.go

Suggested change

allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify a VIP for API"))

allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify both API and Ingress VIPs when specifying either"))

staebler · 2021-03-16T23:41:44Z

pkg/types/vsphere/validation/platform.go

Suggested change

allErrs = append(allErrs, field.Required(fldPath.Child("ingressVIP"), "must specify a VIP for Ingress"))

allErrs = append(allErrs, field.Required(fldPath.Child("ingressVIP"), "must specify both API and Ingress VIPs when specifying either"))

staebler

Looks good. Just need to make the unit test expectations match the reality of the recent changes.

If the machineCIDR is specified by the user during cluster installation in vsphere, the VIP IPs provided must be within the CIDRs provided and the installation fails if they do not. Adding a validation check to see if the CIDR is provided and if so, checks to see if the IPs are within any of the machine CIDRs provided. Changes are local to vsphere.

staebler · 2021-03-17T13:50:21Z

/lgtm
/approve

openshift-ci-robot · 2021-03-17T13:50:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [staebler]

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

staebler · 2021-03-17T13:50:51Z

/test e2e-vsphere
/test e2e-vsphere-upi
/hold for vsphere tests

rna-afk · 2021-03-19T02:54:35Z

/retest

staebler · 2021-03-19T17:30:50Z

vsphere e2e jobs passed.
/hold cancel

openshift-bot · 2021-03-19T17:32:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-19T19:16:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-19T21:26:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-19T22:57:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T00:15:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T00:28:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T02:51:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T04:48:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T05:01:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T05:14:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T06:58:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-20T07:24:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

staebler · 2021-03-21T02:23:55Z

These changes are only for vSphere.
/override ci/prow/e2e-aws-upgrade

openshift-ci-robot · 2021-03-21T02:24:14Z

@staebler: Overrode contexts on behalf of staebler: ci/prow/e2e-aws-upgrade

Details

In response to this:

These changes are only for vSphere.
/override ci/prow/e2e-aws-upgrade

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/test-infra repository.

openshift-bot · 2021-03-21T10:55:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-21T11:08:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-21T12:52:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-21T14:36:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-21T14:49:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-21T15:02:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-03-21T16:02:29Z

@rna-afk: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-workers-rhel7	`5d8bba0`	link	`/test e2e-aws-workers-rhel7`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-03-21T16:07:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-03-21T16:13:57Z

@rna-afk: All pull requests linked via external trackers have merged:

openshift/installer#4754

Bugzilla bug 1918469 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1918469: Check if VIP IPs overlap with machine CIDR provided during vsphere installation

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/test-infra repository.

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 15, 2021

openshift-ci-robot requested review from mtnbikenc and patrickdillon March 15, 2021 19:53

openshift-ci-robot assigned staebler Mar 15, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 15, 2021

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Mar 15, 2021

openshift-ci-robot requested a review from jinyunma March 15, 2021 19:54

staebler reviewed Mar 16, 2021

View reviewed changes

patrickdillon reviewed Mar 16, 2021

View reviewed changes

rna-afk changed the title ~~Bug 1918469: Check if VIP IPs overlap with machine CIDR provided~~ Bug 1918469: Check if VIP IPs overlap with machine CIDR provided during vsphere installation Mar 16, 2021

rna-afk force-pushed the vsphere_apivip_validation branch 2 times, most recently from 829b1dc to cebb327 Compare March 16, 2021 17:42

staebler reviewed Mar 16, 2021

View reviewed changes

rna-afk force-pushed the vsphere_apivip_validation branch from cebb327 to 679eb89 Compare March 16, 2021 18:45

patrickdillon reviewed Mar 16, 2021

View reviewed changes

rna-afk force-pushed the vsphere_apivip_validation branch from 679eb89 to ec46621 Compare March 16, 2021 19:14

staebler reviewed Mar 16, 2021

View reviewed changes

rna-afk force-pushed the vsphere_apivip_validation branch from ec46621 to 13a5ba5 Compare March 17, 2021 00:56

staebler reviewed Mar 17, 2021

View reviewed changes

rna-afk force-pushed the vsphere_apivip_validation branch from 13a5ba5 to 5d8bba0 Compare March 17, 2021 12:35

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2021

openshift-merge-robot merged commit f1a1e5a into openshift:master Mar 21, 2021

staebler mentioned this pull request Mar 22, 2021

Revert "Check if VIP IPs overlap with machine CIDR provided during vsphere installation" #4779

Merged

	allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "must overlap with the machine CIDRs provided"))
	allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "must be contained within one of the machine networks"))

-	if strings.Join([]string{p.APIVIP, p.IngressVIP}, "") != "" {
-		allErrs = append(allErrs, validateVIPs(p, network, fldPath)...)
-	}
+	if p.APIVIP == "" {
+		// add error
+	}
+	if p.IngressVIP == "" {
+		// add error
+	}

	func ValidateForProvisioning(p vsphere.Platform, network types.Networking, fldPath *field.Path) field.ErrorList {
	func ValidateForProvisioning(p vsphere.Platform, fldPath field.Path) field.ErrorList {

	// ipInNetwork return true if the given ip is within the machine CIDRs.
	// ipInNetwork return true if the given ip is within one of the machine networks.

	allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify a VIP for API"))
	allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify both API and Ingress VIPs when specifying either"))

Bug 1918469: Check if VIP IPs overlap with machine CIDR provided during vsphere installation #4754

Bug 1918469: Check if VIP IPs overlap with machine CIDR provided during vsphere installation #4754

Uh oh!

Conversation

rna-afk commented Mar 15, 2021

Uh oh!

openshift-ci-robot commented Mar 15, 2021

Uh oh!

rna-afk commented Mar 15, 2021

Uh oh!

rna-afk commented Mar 15, 2021

Uh oh!

openshift-ci-robot commented Mar 15, 2021

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

staebler commented Mar 17, 2021

Uh oh!

openshift-ci-robot commented Mar 17, 2021

Uh oh!

staebler commented Mar 17, 2021

Uh oh!

rna-afk commented Mar 19, 2021

Uh oh!

staebler commented Mar 19, 2021

Uh oh!

openshift-bot commented Mar 19, 2021

Uh oh!

openshift-bot commented Mar 19, 2021

Uh oh!

openshift-bot commented Mar 19, 2021

Uh oh!

openshift-bot commented Mar 19, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

Uh oh!

openshift-bot commented Mar 20, 2021

patrickdillon Mar 16, 2021 •

edited

Loading

openshift-ci bot commented Mar 21, 2021 •

edited

Loading