OpenStack: add clusterOSImage validations #3964

Fedosin · 2020-07-27T16:29:47Z

This commit adds validations that custom Glance image specified by clusterOSImage config parameter exists and has Active status.

Fedosin · 2020-07-27T17:38:33Z

/hold

Fedosin · 2020-07-27T18:04:19Z

/test unit

Fedosin · 2020-07-28T09:13:53Z

/label platform/openstack

Fedosin · 2020-07-30T13:06:27Z

/retest

Fedosin · 2020-07-30T14:29:39Z

/hold cancel

Fedosin · 2020-07-30T19:22:11Z

/test e2e-openstack

pierreprinetti · 2020-08-24T15:34:53Z

pkg/asset/installconfig/openstack/validation/cloudinfo.go

+
+func (ci *CloudInfo) getImage(imageName string) (*images.Image, error) {
+	if imageName == "" {
+		return &images.Image{}, nil


Question: Why do you return a pointer to an empty image? I expected to see return nil nil here

This is an absolutely useless check and should be removed. when we call getImage we check that the image name is non-empty.
Thank you for pointing at this!

jstuever · 2020-10-09T16:43:51Z

/uncc

iamemilio · 2020-10-09T18:03:16Z

/lgtm

openshift-ci-robot · 2020-10-09T19:03:32Z

@Fedosin: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-workers-rhel7	`1734b5d`	link	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-metal-ipi	`1734b5d`	link	`/test e2e-metal-ipi`
ci/prow/e2e-ovirt	`1734b5d`	link	`/test e2e-ovirt`
ci/prow/e2e-libvirt	`1734b5d`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard.

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.

Fedosin · 2020-10-12T10:13:38Z

/test e2e-aws

abhinavdahiya · 2020-10-14T17:46:48Z

pkg/asset/installconfig/openstack/validation/cloudinfo.go

+	imagePtr := ic.OpenStack.ClusterOSImage
+	if imagePtr != "" {
+		if _, err := url.ParseRequestURI(imagePtr); err != nil {
+			ci.OSImage, err = ci.getImage(imagePtr)


this err here is not handled

oh... my bad :( thanks for noticing!

abhinavdahiya · 2020-10-14T17:48:58Z

pkg/asset/installconfig/openstack/validation/platform.go

+	// If the user input is an http address, then skip the validation
+	if _, err := url.ParseRequestURI(p.ClusterOSImage); err == nil {


this comment does not match what is being done here, it's checking for a valid URI which includes any scheme not just http.

if you support http and https only, add validation for that here.

yeah, agree. but we also support 'file' to get images from the local file system.

abhinavdahiya · 2020-10-14T17:49:32Z

pkg/asset/installconfig/openstack/validation/platform.go

+		return
+	}
+
+	// Image should exist


can you expand here why image should exist or where it should exist?

abhinavdahiya · 2020-10-14T17:50:21Z

pkg/asset/installconfig/openstack/validation/platform.go

+
+	// Image should be active
+	if ci.OSImage.Status != images.ImageStatusActive {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, "image is not active"))


Suggested change

allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, "image is not active"))

allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, "image must be active but it was %s"))

provide details on the failed image.

abhinavdahiya · 2020-10-14T17:51:51Z

pkg/asset/installconfig/openstack/validation/platform_test.go

+		expectedError  bool
+		expectedErrMsg string // NOTE: this is a REGEXP


expectedError bool is just duplication of the requirement from expectedErrMsg string, if expectedErrMsg is not empty the error must match that regex else when empty there should be no error.

abhinavdahiya · 2020-10-14T22:12:04Z

pkg/asset/installconfig/openstack/validation/platform.go

+		switch uri.Scheme {
+		case "http", "https", "file":
+		default:
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, fmt.Sprintf("URL scheme should be either http(s) or file but it is '%v'", uri.Scheme)))


Go has fmt directive %q which will quote the string

abhinavdahiya · 2020-10-14T22:12:22Z

pkg/asset/installconfig/openstack/validation/platform.go

+
+	// Image should have "active" status
+	if ci.OSImage.Status != images.ImageStatusActive {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, fmt.Sprintf("OS image must be active but its status is '%s'", ci.OSImage.Status)))


Go has fmt directive %q which will quote the string

abhinavdahiya · 2020-10-14T22:13:21Z

/approve

openshift-ci-robot · 2020-10-14T22:13:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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]

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

openshift-merge-robot · 2020-10-14T22:17:14Z

@Fedosin: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-crc	`58e8e6e`	link	`/test e2e-crc`
ci/prow/e2e-metal-ipi	`58e8e6e`	link	`/test e2e-metal-ipi`
ci/prow/e2e-libvirt	`58e8e6e`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard.

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.

iamemilio · 2020-10-15T00:06:08Z

/lgtm

Previously we validate overriden Glance images during generation of tfvars variables. This is not a correct place to do it, so we implemented the check in the "validation" module: openshift#3964 To prevent code duplication we should remove the legacy code.

openshift-ci-robot requested review from adduarte and jstuever July 27, 2020 16:30

Fedosin force-pushed the clusterosimage branch from 6aa4fdd to b5e44d2 Compare July 27, 2020 16:54

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

Fedosin force-pushed the clusterosimage branch from b5e44d2 to 87ab46d Compare July 27, 2020 18:00

openshift-ci-robot added the platform/openstack label Jul 28, 2020

Fedosin force-pushed the clusterosimage branch from 87ab46d to 00e4728 Compare July 30, 2020 10:40

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

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2020

pierreprinetti reviewed Aug 24, 2020

View reviewed changes

Fedosin force-pushed the clusterosimage branch from 00e4728 to 1734b5d Compare October 9, 2020 14:32

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2020

openshift-ci-robot removed the request for review from jstuever October 9, 2020 16:43

openshift-ci-robot assigned iamemilio Oct 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2020

abhinavdahiya reviewed Oct 14, 2020

View reviewed changes

Fedosin added 2 commits October 14, 2020 21:41

OpenStack: add clusterOSImage validations

ebba700

Add github.com/gophercloud/utils/openstack/imageservice to vendors

58e8e6e

Fedosin force-pushed the clusterosimage branch from 1734b5d to 58e8e6e Compare October 14, 2020 19:42

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

abhinavdahiya reviewed Oct 14, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2020

openshift-merge-robot merged commit 0dfd15c into openshift:master Oct 15, 2020

Fedosin mentioned this pull request Oct 16, 2020

OpenStack: remove legacy image validation #4277

Merged

mandre mentioned this pull request Jan 12, 2021

[ShiftStack] 4.7 RNs openshift/openshift-docs#28504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenStack: add clusterOSImage validations #3964

OpenStack: add clusterOSImage validations #3964

Fedosin commented Jul 27, 2020 •

edited

Fedosin commented Jul 27, 2020

Fedosin commented Jul 27, 2020

Fedosin commented Jul 28, 2020

Fedosin commented Jul 30, 2020

Fedosin commented Jul 30, 2020

Fedosin commented Jul 30, 2020

pierreprinetti Aug 24, 2020

Fedosin Oct 9, 2020

jstuever commented Oct 9, 2020

iamemilio commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020 •

edited

Fedosin commented Oct 12, 2020

abhinavdahiya Oct 14, 2020

Fedosin Oct 14, 2020

abhinavdahiya Oct 14, 2020

Fedosin Oct 14, 2020

abhinavdahiya Oct 14, 2020

Fedosin Oct 14, 2020

abhinavdahiya Oct 14, 2020

abhinavdahiya Oct 14, 2020

Fedosin Oct 14, 2020

abhinavdahiya Oct 14, 2020

Fedosin Oct 14, 2020

abhinavdahiya Oct 14, 2020

abhinavdahiya Oct 14, 2020

abhinavdahiya commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

openshift-merge-robot commented Oct 14, 2020

iamemilio commented Oct 15, 2020

		// If the user input is an http address, then skip the validation
		if _, err := url.ParseRequestURI(p.ClusterOSImage); err == nil {

	allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, "image is not active"))
	allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterOSImage"), p.ClusterOSImage, "image must be active but it was %s"))

		expectedError bool
		expectedErrMsg string // NOTE: this is a REGEXP

OpenStack: add clusterOSImage validations #3964

OpenStack: add clusterOSImage validations #3964

Conversation

Fedosin commented Jul 27, 2020 • edited

Fedosin commented Jul 27, 2020

Fedosin commented Jul 27, 2020

Fedosin commented Jul 28, 2020

Fedosin commented Jul 30, 2020

Fedosin commented Jul 30, 2020

Fedosin commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstuever commented Oct 9, 2020

iamemilio commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020 • edited

Fedosin commented Oct 12, 2020

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

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 commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

openshift-merge-robot commented Oct 14, 2020

iamemilio commented Oct 15, 2020

Fedosin commented Jul 27, 2020 •

edited

openshift-ci-robot commented Oct 9, 2020 •

edited