*: Make libvirt support completely conditional (behind TAGS=libvirt)

wking · wking · commit 432f28355a75 · 2018-12-21T13:09:29.000-08:00
Previously, destroy support was behind TAGS=libvirt_destroy and create support was always built in. But since 3fb4400 (terraform/plugins: add `libvirt`, `aws`, `ignition`, `openstack` to KnownPlugins, 2018-12-14, #919), the bundled libvirt Terraform provider has also been behind libvirt_destroy. That leads to cluster creation failing with: $ openshift-install create cluster ... ERROR Missing required providers. ERROR ERROR The following provider constraints are not met by the currently-installed ERROR provider plugins: ERROR ERROR * libvirt (any version) ERROR ERROR Terraform can automatically download and install plugins to meet the given ERROR constraints, but this step was skipped due to the use of -get-plugins=false ERROR and/or -plugin-dir on the command line. ... With this commit, folks trying to 'create cluster' without libvirt compiled in will get: FATAL failed to fetch Common Manifests: failed to load asset "Install Config": invalid "install-config.yaml" file: platform: Invalid value: types.Platform{AWS:(*aws.Platform)(nil), Libvirt:(*libvirt.Platform)(0xc4209511f0), OpenStack:(*openstack.Platform)(nil)}: platform must be one of: aws, openstack before we get to Terraform. Now that the build tag guards both creation and deletion, I've renamed it from 'libvirt_destroy' to the unqualified 'libvirt'. I've also adjusted the install-config validation testing to use regular expressions so we can distinguish between failures because libvirt was not compiled in as a valid platform and failures because some portion of the libvirt configuration was broken. In order to get stable error messages for comparison, I've added some strings.Sort calls for various allowed-value string-slice computations.
diff --git a/docs/dev/libvirt-howto.md b/docs/dev/libvirt-howto.md
@@ -202,21 +202,21 @@ This step allows installer and users to resolve cluster-internal hostnames from
 ## Build and run the installer
 
 With [libvirt configured](#install-and-enable-libvirt), you can proceed with [the usual quick-start](../../README.md#quick-start).
-Set `TAGS` when building if you need `destroy cluster` support for libvirt; this is not enabled by default because it requires [cgo][]:
+Set `TAGS=libvirt` to add support for libvirt; this is not enabled by default because libvirt is [development only](../../README.md#supported-platforms).
 
 ```sh
-TAGS=libvirt_destroy hack/build.sh
+TAGS=libvirt hack/build.sh
 ```
 
 ## Cleanup
 
-If you compiled with `libvirt_destroy`, you can use:
+To remove resources associated with your cluster, run:
 
 ```sh
 openshift-install destroy cluster
 ```
 
-If you did not compile with `libvirt_destroy`, you can use [`virsh-cleanup.sh`](../../scripts/maintenance/virsh-cleanup.sh), but note it will currently destroy *all* libvirt resources.
+You can also use [`virsh-cleanup.sh`](../../scripts/maintenance/virsh-cleanup.sh), but note that it will currently destroy *all* libvirt resources.
 
 ### Firewall
 
@@ -341,7 +341,6 @@ If your issue is not reported, please do.
 [arch_firewall_superuser]: https://superuser.com/questions/1063240/libvirt-failed-to-initialize-a-valid-firewall-backend
 [brokenmacosissue201]: https://github.com/openshift/installer/issues/201
 [bugzilla_libvirt_race]: https://bugzilla.redhat.com/show_bug.cgi?id=1576464
-[cgo]: https://golang.org/cmd/cgo/
 [issues_libvirt]: https://github.com/openshift/installer/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+libvirt
 [libvirt_selinux_issues]: https://github.com/dmacvicar/terraform-provider-libvirt/issues/142#issuecomment-409040151
 [tfprovider_libvirt_race]: https://github.com/dmacvicar/terraform-provider-libvirt/issues/402#issuecomment-419500064
diff --git a/hack/build.sh b/hack/build.sh
@@ -59,7 +59,7 @@ dev)
 	exit 1
 esac
 
-if (echo "${TAGS}" | grep -q 'libvirt_destroy')
+if (echo "${TAGS}" | grep -q 'libvirt')
 then
 	export CGO_ENABLED=1
 fi
diff --git a/images/nested-libvirt/Dockerfile b/images/nested-libvirt/Dockerfile
@@ -3,7 +3,7 @@
 FROM openshift/origin-release:golang-1.10 AS build
 WORKDIR /go/src/github.com/openshift/installer
 COPY . .
-RUN TAGS=libvirt_destroy hack/build.sh
+RUN TAGS=libvirt hack/build.sh
 
 FROM centos:7
 COPY --from=build /go/src/github.com/openshift/installer/bin/openshift-install /bin/openshift-install
diff --git a/pkg/destroy/libvirt/libvirt.go b/pkg/destroy/libvirt/libvirt.go
@@ -1,4 +1,4 @@
-// +build libvirt_destroy
+// +build libvirt
 
 package libvirt
 
diff --git a/pkg/destroy/libvirt/register.go b/pkg/destroy/libvirt/register.go
@@ -1,4 +1,4 @@
-// +build libvirt_destroy
+// +build libvirt
 
 package libvirt
 
diff --git a/pkg/terraform/exec/plugins/libvirt.go b/pkg/terraform/exec/plugins/libvirt.go
@@ -1,4 +1,4 @@
-// +build libvirt_destroy
+// +build libvirt
 
 package plugins
 
diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go
@@ -1,6 +1,8 @@
 package validation
 
 import (
+	"sort"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	"github.com/openshift/installer/pkg/types/aws"
@@ -39,6 +41,7 @@ var (
 			validValues[i] = r
 			i++
 		}
+		sort.Strings(validValues)
 		return validValues
 	}()
 )
diff --git a/pkg/types/installconfig.go b/pkg/types/installconfig.go
@@ -14,7 +14,6 @@ var (
 	// alphabetical order.
 	PlatformNames = []string{
 		aws.Name,
-		libvirt.Name,
 		openstack.Name,
 	}
 )
diff --git a/pkg/types/installconfig_libvirt.go b/pkg/types/installconfig_libvirt.go
@@ -0,0 +1,14 @@
+// +build libvirt
+
+package types
+
+import (
+	"sort"
+
+	"github.com/openshift/installer/pkg/types/libvirt"
+)
+
+func init() {
+	PlatformNames = append(PlatformNames, libvirt.Name)
+	sort.Strings(PlatformNames)
+}
diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go
@@ -3,6 +3,8 @@ package validation
 import (
 	"fmt"
 	"net"
+	"sort"
+	"strings"
 
 	netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -108,12 +110,14 @@ func validateMachinePools(pools []types.MachinePool, fldPath *field.Path, platfo
 
 func validatePlatform(platform *types.Platform, fldPath *field.Path, openStackValidValuesFetcher openstackvalidation.ValidValuesFetcher) field.ErrorList {
 	allErrs := field.ErrorList{}
-	activePlatform := ""
+	activePlatform := platform.Name()
+	i := sort.SearchStrings(types.PlatformNames, activePlatform)
+	if i == len(types.PlatformNames) || types.PlatformNames[i] != activePlatform {
+		allErrs = append(allErrs, field.Invalid(fldPath, platform, fmt.Sprintf("must specify one of the platforms (%s)", strings.Join(types.PlatformNames, ", "))))
+	}
 	validate := func(n string, value interface{}, validation func(*field.Path) field.ErrorList) {
-		if activePlatform != "" {
+		if n != activePlatform {
 			allErrs = append(allErrs, field.Invalid(fldPath, platform, fmt.Sprintf("must only specify a single type of platform; cannot use both %q and %q", activePlatform, n)))
-		} else {
-			activePlatform = n
 		}
 		allErrs = append(allErrs, validation(fldPath.Child(n))...)
 	}
@@ -128,8 +132,5 @@ func validatePlatform(platform *types.Platform, fldPath *field.Path, openStackVa
 			return openstackvalidation.ValidatePlatform(platform.OpenStack, f, openStackValidValuesFetcher)
 		})
 	}
-	if activePlatform == "" {
-		allErrs = append(allErrs, field.Invalid(fldPath, platform, "must specify one of the platforms"))
-	}
 	return allErrs
 }
diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go
@@ -54,12 +54,11 @@ func TestValidateInstallConfig(t *testing.T) {
 	cases := []struct {
 		name          string
 		installConfig *types.InstallConfig
-		valid         bool
+		expectedError string
 	}{
 		{
 			name:          "minimal",
 			installConfig: validInstallConfig(),
-			valid:         true,
 		},
 		{
 			name: "missing name",
@@ -68,7 +67,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.ObjectMeta.Name = ""
 				return c
 			}(),
-			valid: false,
+			expectedError: `^metadata.name: Required value: cluster name required$`,
 		},
 		{
 			name: "missing cluster ID",
@@ -77,7 +76,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.ClusterID = ""
 				return c
 			}(),
-			valid: false,
+			expectedError: `^clusterID: Required value: cluster ID required$`,
 		},
 		{
 			name: "invalid ssh key",
@@ -86,7 +85,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.SSHKey = "bad-ssh-key"
 				return c
 			}(),
-			valid: false,
+			expectedError: `^sshKey: Invalid value: "bad-ssh-key": ssh: no key found$`,
 		},
 		{
 			name: "invalid base domain",
@@ -95,7 +94,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.BaseDomain = ".bad-domain."
 				return c
 			}(),
-			valid: false,
+			expectedError: `^baseDomain: Invalid value: "\.bad-domain\.": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '\.', and must start and end with an alphanumeric character \(e\.g\. 'example\.com', regex used for validation is '\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\(\\\.\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\)\*'\)$`,
 		},
 		{
 			name: "invalid network type",
@@ -104,7 +103,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.Type = "bad-type"
 				return c
 			}(),
-			valid: false,
+			expectedError: `^networking.type: Unsupported value: "bad-type": supported values: "Calico", "Kuryr", "OVNKubernetes", "OpenshiftSDN"$`,
 		},
 		{
 			name: "invalid service cidr",
@@ -113,7 +112,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.ServiceCIDR = ipnet.IPNet{}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^networking\.serviceCIDR: Invalid value: ipnet\.IPNet{IPNet:net\.IPNet{IP:net\.IP\(nil\), Mask:net\.IPMask\(nil\)}}: must use IPv4$`,
 		},
 		{
 			name: "invalid cluster network cidr",
@@ -122,7 +121,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.ClusterNetworks[0].CIDR = "bad-cidr"
 				return c
 			}(),
-			valid: false,
+			expectedError: `^networking\.clusterNetworks\[0]\.cidr: Invalid value: "bad-cidr": invalid CIDR address: bad-cidr$`,
 		},
 		{
 			name: "overlapping cluster network cidr",
@@ -131,7 +130,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.ClusterNetworks[0].CIDR = "10.0.0.0/24"
 				return c
 			}(),
-			valid: false,
+			expectedError: `^networking\.clusterNetworks\[0]\.cidr: Invalid value: "10\.0\.0\.0/24": cluster network CIDR must not overlap with serviceCIDR$`,
 		},
 		{
 			name: "cluster network host subnet length too large",
@@ -141,7 +140,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Networking.ClusterNetworks[0].HostSubnetLength = 9
 				return c
 			}(),
-			valid: false,
+			expectedError: `^networking\.clusterNetworks\[0]\.hostSubnetLength: Invalid value: 0x9: cluster network host subnet length must not be greater than CIDR length$`,
 		},
 		{
 			name: "missing master machine pool",
@@ -154,7 +153,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^machines: Required value: must specify a machine pool with a name of 'master'$`,
 		},
 		{
 			name: "missing worker machine pool",
@@ -167,7 +166,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^machines: Required value: must specify a machine pool with a name of 'worker'$`,
 		},
 		{
 			name: "duplicate machine pool",
@@ -178,7 +177,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				})
 				return c
 			}(),
-			valid: false,
+			expectedError: `^machines\[2]: Duplicate value: types\.MachinePool{Name:"master", Replicas:\(\*int64\)\(nil\), Platform:types\.MachinePoolPlatform{AWS:\(\*aws\.MachinePool\)\(nil\), Libvirt:\(\*libvirt\.MachinePool\)\(nil\), OpenStack:\(\*openstack\.MachinePool\)\(nil\)}}$`,
 		},
 		{
 			name: "invalid machine pool",
@@ -189,7 +188,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				})
 				return c
 			}(),
-			valid: false,
+			expectedError: `^machines\[2]\.name: Unsupported value: "other": supported values: "master", "worker"$`,
 		},
 		{
 			name: "missing platform",
@@ -198,7 +197,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Platform = types.Platform{}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^platform: Invalid value: types\.Platform{AWS:\(\*aws\.Platform\)\(nil\), Libvirt:\(\*libvirt\.Platform\)\(nil\), OpenStack:\(\*openstack\.Platform\)\(nil\)}: must specify one of the platforms \(aws, openstack\)$`,
 		},
 		{
 			name: "multiple platforms",
@@ -207,7 +206,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				c.Platform.Libvirt = &libvirt.Platform{}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^\[platform: Invalid value: types\.Platform{AWS:\(\*aws\.Platform\)\(0x[0-9a-f]*\), Libvirt:\(\*libvirt\.Platform\)\(0x[0-9a-f]*\), OpenStack:\(\*openstack\.Platform\)\(nil\)}: must only specify a single type of platform; cannot use both "aws" and "libvirt", platform\.libvirt\.uri: Invalid value: "": invalid URI "" \(no scheme\), platform\.libvirt\.network\.if: Required value, platform\.libvirt\.network\.ipRange: Invalid value: ipnet\.IPNet{IPNet:net\.IPNet{IP:net\.IP\(nil\), Mask:net\.IPMask\(nil\)}}: must use IPv4]$`,
 		},
 		{
 			name: "invalid aws platform",
@@ -218,7 +217,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^platform\.aws\.region: Unsupported value: "": supported values: "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2", "ca-central-1", "cn-north-1", "cn-northwest-1", "eu-central-1", "eu-west-1", "eu-west-2", "eu-west-3", "sa-east-1", "us-east-1", "us-east-2", "us-west-1", "us-west-2"$`,
 		},
 		{
 			name: "valid libvirt platform",
@@ -235,7 +234,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: true,
+			expectedError: `^platform: Invalid value: types\.Platform{AWS:\(\*aws\.Platform\)\(nil\), Libvirt:\(\*libvirt\.Platform\)\(0x[0-9a-f]*\), OpenStack:\(\*openstack\.Platform\)\(nil\)}: must specify one of the platforms \(aws, openstack\)$`,
 		},
 		{
 			name: "invalid libvirt platform",
@@ -246,7 +245,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^\[platform: Invalid value: types\.Platform{AWS:\(\*aws\.Platform\)\(nil\), Libvirt:\(\*libvirt\.Platform\)\(0x[0-9a-f]*\), OpenStack:\(\*openstack\.Platform\)\(nil\)}: must specify one of the platforms \(aws, openstack\), platform\.libvirt\.uri: Invalid value: "": invalid URI "" \(no scheme\), platform\.libvirt\.network\.if: Required value, platform\.libvirt\.network\.ipRange: Invalid value: ipnet\.IPNet{IPNet:net\.IPNet{IP:net\.IP\(nil\), Mask:net\.IPMask\(nil\)}}: must use IPv4]$`,
 		},
 		{
 			name: "valid openstack platform",
@@ -263,7 +262,6 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: true,
 		},
 		{
 			name: "invalid openstack platform",
@@ -274,7 +272,7 @@ func TestValidateInstallConfig(t *testing.T) {
 				}
 				return c
 			}(),
-			valid: false,
+			expectedError: `^\[platform\.openstack\.cloud: Unsupported value: "": supported values: "test-cloud", platform\.openstack\.NetworkCIDRBlock: Invalid value: ipnet\.IPNet{IPNet:net\.IPNet{IP:net\.IP\(nil\), Mask:net\.IPMask\(nil\)}}: must use IPv4]$`,
 		},
 	}
 	for _, tc := range cases {
@@ -289,10 +287,10 @@ func TestValidateInstallConfig(t *testing.T) {
 			fetcher.EXPECT().GetNetworkNames(gomock.Any()).Return([]string{"test-network"}, nil).AnyTimes()
 
 			err := ValidateInstallConfig(tc.installConfig, fetcher).ToAggregate()
-			if tc.valid {
+			if tc.expectedError == "" {
 				assert.NoError(t, err)
 			} else {
-				assert.Error(t, err)
+				assert.Regexp(t, tc.expectedError, err)
 			}
 		})
 	}
diff --git a/pkg/types/validation/machinepools.go b/pkg/types/validation/machinepools.go
@@ -2,6 +2,7 @@ package validation
 
 import (
 	"fmt"
+	"sort"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
@@ -27,6 +28,7 @@ var (
 			validValues[i] = n
 			i++
 		}
+		sort.Strings(validValues)
 		return validValues
 	}()
 )
diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net"
 	"net/url"
+	"sort"
 	"strings"
 
 	netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1"
@@ -38,6 +39,7 @@ var (
 			validValues[i] = string(t)
 			i++
 		}
+		sort.Strings(validValues)
 		return validValues
 	}()
 )

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// +build libvirt_destroy`
	`1`	`+// +build libvirt`
`2`	`2`
`3`	`3`	`package libvirt`
`4`	`4`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,8 @@`
`1`	`1`	`package validation`
`2`	`2`
`3`	`3`	`import (`
	`4`	`+ "sort"`
	`5`	`+`
`4`	`6`	`"k8s.io/apimachinery/pkg/util/validation/field"`
`5`	`7`
`6`	`8`	`"github.com/openshift/installer/pkg/types/aws"`
`@@ -39,6 +41,7 @@ var (`
`39`	`41`	`validValues[i] = r`
`40`	`42`	`i++`
`41`	`43`	`}`
	`44`	`+ sort.Strings(validValues)`
`42`	`45`	`return validValues`
`43`	`46`	`}()`
`44`	`47`	`)`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,6 @@ var (`
`14`	`14`	`// alphabetical order.`
`15`	`15`	`PlatformNames = []string{`
`16`	`16`	`aws.Name,`
`17`		`- libvirt.Name,`
`18`	`17`	`openstack.Name,`
`19`	`18`	`}`
`20`	`19`	`)`