Fix userdata compression

When the UncompressedUserData field is nil on CloudStackMachine
instances CAPC is interpreting it as true.

This reworks the compression decision logic to give a nil
UncompressedUserData the same semantics as false.

Co-authored-by: Guillermo Gaston <[email protected]>

Author: chrisdoherty4

Makefile

@@ -263,7 +263,8 @@ config/.flag-test.mk: $(CONTROLLER_GEN) $(MANIFEST_GEN_INPUTS_TEST)
 	@touch config/.flag-test.mk
 
 .PHONY: test
-test: generate-deepcopy-test generate-manifest-test generate-mocks lint $(GINKGO_V2) $(KUBECTL) $(API_SERVER) $(ETCD) ## Run tests. At the moment this is only unit tests.
+test: ## Run tests.
+test: generate-deepcopy-test generate-manifest-test generate-mocks lint $(GINKGO_V2) $(KUBECTL) $(API_SERVER) $(ETCD)
 	@./hack/testing_ginkgo_recover_statements.sh --add # Add ginkgo.GinkgoRecover() statements to controllers.
 	@# The following is a slightly funky way to make sure the ginkgo statements are removed regardless the test results.
 	@$(GINKGO_V2) --label-filter="!integ" --cover -coverprofile cover.out --covermode=atomic -v ./api/... ./controllers/... ./pkg/...; EXIT_STATUS=$$?;\

api/v1beta2/cloudstackmachine_types.go

@@ -92,6 +92,10 @@ type CloudStackMachineSpec struct {
 	UncompressedUserData *bool `json:"uncompressedUserData,omitempty"`
 }
 
+func (c *CloudStackMachine) CompressUserdata() bool {
+	return c.Spec.UncompressedUserData == nil || !*c.Spec.UncompressedUserData
+}
+
 type CloudStackResourceIdentifier struct {
 	// Cloudstack resource ID.
 	// +optional

api/v1beta2/cloudstackmachine_types_test.go

@@ -0,0 +1,67 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta2_test
+
+import (
+	"k8s.io/utils/pointer"
+	capcv1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta2"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("CloudStackMachineConfig_CompressUserdata", func() {
+	for _, tc := range []struct {
+		Name    string
+		Machine capcv1.CloudStackMachine
+		Expect  bool
+	}{
+		{
+			Name: "is true when uncompressed user data is nil",
+			Machine: capcv1.CloudStackMachine{
+				Spec: capcv1.CloudStackMachineSpec{
+					UncompressedUserData: nil,
+				},
+			},
+			Expect: true,
+		},
+		{
+			Name: "is false when uncompressed user data is true",
+			Machine: capcv1.CloudStackMachine{
+				Spec: capcv1.CloudStackMachineSpec{
+					UncompressedUserData: pointer.Bool(true),
+				},
+			},
+			Expect: false,
+		},
+		{
+			Name: "Is false when uncompressed user data is false",
+			Machine: capcv1.CloudStackMachine{
+				Spec: capcv1.CloudStackMachineSpec{
+					UncompressedUserData: pointer.Bool(false),
+				},
+			},
+			Expect: true,
+		},
+	} {
+		tc := tc
+		It(tc.Name, func() {
+			result := tc.Machine.CompressUserdata()
+			Expect(result).To(Equal(tc.Expect))
+		})
+	}
+})

pkg/cloud/helpers.go

@@ -18,7 +18,7 @@ package cloud
 
 import (
 	"bytes"
-	"compress/gzip"
+	cgzip "compress/gzip"
 )
 
 type set func(string)
@@ -43,13 +43,13 @@ func setIntIfPositive(num int64, setFn setInt) {
 	}
 }
 
-func CompressString(str string) (string, error) {
-	buf := &bytes.Buffer{}
-	gzipWriter := gzip.NewWriter(buf)
-	if _, err := gzipWriter.Write([]byte(str)); err != nil {
+func compress(str string) (string, error) {
+	var buf bytes.Buffer
+	w := cgzip.NewWriter(&buf)
+	if _, err := w.Write([]byte(str)); err != nil {
 		return "", err
 	}
-	if err := gzipWriter.Close(); err != nil {
+	if err := w.Close(); err != nil {
 		return "", err
 	}
 	return buf.String(), nil

pkg/cloud/helpers_test.go

@@ -24,27 +24,10 @@ import (
 	"reflect"
 
 	"github.com/golang/mock/gomock"
-	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/types"
-	"sigs.k8s.io/cluster-api-provider-cloudstack/pkg/cloud"
 )
 
-var _ = Describe("Helpers", func() {
-
-	It("should compress and encode string", func() {
-		str := "Hello World"
-
-		compressedData, err := cloud.CompressString(str)
-
-		reader, _ := gzip.NewReader(bytes.NewReader([]byte(compressedData)))
-		result, _ := io.ReadAll(reader)
-
-		Ω(err).Should(BeNil())
-		Ω(string(result)).Should(Equal(str))
-	})
-})
-
 // This matcher is used to make gomega matching compatible with gomock parameter matching.
 // It's pretty awesome!
 //
@@ -91,9 +74,16 @@ func FieldMatcherGenerator(fetchFunc string) func(string) types.GomegaMatcher {
 	}
 }
 
-var (
-	DomainIDEquals = FieldMatcherGenerator("GetDomainid")
-	AccountEquals  = FieldMatcherGenerator("GetAccount")
-	IDEquals       = FieldMatcherGenerator("GetId")
-	NameEquals     = FieldMatcherGenerator("GetName")
-)
+var NameEquals = FieldMatcherGenerator("GetName")
+
+func decompress(data []byte) ([]byte, error) {
+	reader, err := gzip.NewReader(bytes.NewBuffer(data))
+	if err != nil {
+		return nil, err
+	}
+	data, err = io.ReadAll(reader)
+	if err != nil {
+		return nil, err
+	}
+	return data, nil
+}

pkg/cloud/instance.go

@@ -214,7 +214,8 @@ func (c *client) GetOrCreateVMInstance(
 	csCluster *infrav1.CloudStackCluster,
 	fd *infrav1.CloudStackFailureDomain,
 	affinity *infrav1.CloudStackAffinityGroup,
-	userData string) error {
+	userData string,
+) error {
 
 	// Check if VM instance already exists.
 	if err := c.ResolveVMInstanceDetails(csMachine); err == nil ||
@@ -245,19 +246,19 @@ func (c *client) GetOrCreateVMInstance(
 
 	setIfNotEmpty(csMachine.Spec.SSHKey, p.SetKeypair)
 
-	userData, err = handleUserData(userData, csMachine.Spec.UncompressedUserData)
-	if err != nil {
-		return err
+	if csMachine.CompressUserdata() {
+		userData, err = compress(userData)
+		if err != nil {
+			return err
+		}
 	}
+	userData = base64.StdEncoding.EncodeToString([]byte(userData))
 	setIfNotEmpty(userData, p.SetUserdata)
 
 	if len(csMachine.Spec.AffinityGroupIDs) > 0 {
 		p.SetAffinitygroupids(csMachine.Spec.AffinityGroupIDs)
 	} else if strings.ToLower(csMachine.Spec.Affinity) != "no" && csMachine.Spec.Affinity != "" {
 		p.SetAffinitygroupids([]string{affinity.Spec.ID})
-		if err != nil {
-			return err
-		}
 	}
 
 	if csMachine.Spec.Details != nil {
@@ -268,21 +269,22 @@ func (c *client) GetOrCreateVMInstance(
 	if err != nil {
 		c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
 
-		// Just because an error was returned doesn't mean a (failed) VM wasn't created and will need to be dealt with.
-		// Regretfully the deployVMResp may be nil, so we need to get the VM ID with a separate query, so we
-		// can return it to the caller, so they can clean it up.
-		listVirtualMachineParams := c.cs.VirtualMachine.NewListVirtualMachinesParams()
-		listVirtualMachineParams.SetTemplateid(templateID)
-		listVirtualMachineParams.SetZoneid(fd.Spec.Zone.ID)
-		listVirtualMachineParams.SetNetworkid(fd.Spec.Zone.Network.ID)
-		listVirtualMachineParams.SetName(csMachine.Name)
-		listVirtualMachinesResponse, err2 := c.cs.VirtualMachine.ListVirtualMachines(listVirtualMachineParams)
-		if err2 != nil || listVirtualMachinesResponse.Count <= 0 {
-			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err2)
+		// CloudStack may have created the VM even though it reported an error. We attempt to
+		// retrieve the VM so we can populate the CloudStackMachine for the user to manually
+		// clean up.
+		vm, findErr := findVirtualMachine(c.cs.VirtualMachine, templateID, fd, csMachine)
+		if findErr != nil {
+			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(findErr)
+			return fmt.Errorf("%v; find virtual machine: %v", err, findErr)
+		}
+
+		// We didn't find a VM so return the original error.
+		if vm == nil {
 			return err
 		}
-		csMachine.Spec.InstanceID = pointer.String(listVirtualMachinesResponse.VirtualMachines[0].Id)
-		csMachine.Status.InstanceState = listVirtualMachinesResponse.VirtualMachines[0].State
+
+		csMachine.Spec.InstanceID = pointer.String(vm.Id)
+		csMachine.Status.InstanceState = vm.State
 	} else {
 		csMachine.Spec.InstanceID = pointer.String(deployVMResp.Id)
 		csMachine.Status.Status = pointer.String(metav1.StatusSuccess)
@@ -292,6 +294,32 @@ func (c *client) GetOrCreateVMInstance(
 	return c.ResolveVMInstanceDetails(csMachine)
 }
 
+// findVirtualMachine retrieves a virtual machine by matching its expected name, template, failure
+// domain zone and failure domain network. If no virtual machine is found it returns nil, nil.
+func findVirtualMachine(
+	client cloudstack.VirtualMachineServiceIface,
+	templateID string,
+	failureDomain *infrav1.CloudStackFailureDomain,
+	machine *infrav1.CloudStackMachine,
+) (*cloudstack.VirtualMachine, error) {
+	params := client.NewListVirtualMachinesParams()
+	params.SetTemplateid(templateID)
+	params.SetZoneid(failureDomain.Spec.Zone.ID)
+	params.SetNetworkid(failureDomain.Spec.Zone.Network.ID)
+	params.SetName(machine.Name)
+
+	response, err := client.ListVirtualMachines(params)
+	if err != nil {
+		return nil, err
+	}
+
+	if response.Count == 0 {
+		return nil, nil
+	}
+
+	return response.VirtualMachines[0], nil
+}
+
 // DestroyVMInstance Destroys a VM instance. Assumes machine has been fetched prior and has an instance ID.
 func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
 	// Attempt deletion regardless of machine state.
@@ -346,15 +374,3 @@ func (c *client) listVMInstanceDatadiskVolumeIDs(instanceID string) ([]string, e
 	return ret, nil
 
 }
-
-// handleUserData optionally compresses and then base64 encodes userdata
-func handleUserData(userData string, uncompressed *bool) (string, error) {
-	var err error
-	if uncompressed != nil && !*uncompressed {
-		userData, err = CompressString(userData)
-		if err != nil {
-			return "", err
-		}
-	}
-	return base64.StdEncoding.EncodeToString([]byte(userData)), nil
-}

pkg/cloud/instance_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cloud_test
 
 import (
+	"encoding/base64"
 	"fmt"
 
 	"github.com/apache/cloudstack-go/v2/cloudstack"
@@ -264,14 +265,27 @@ var _ = Describe("Instance", func() {
 
 				deploymentResp := &cloudstack.DeployVirtualMachineResponse{Id: *dummies.CSMachine1.Spec.InstanceID}
 
+				expectUserData := "my special userdata"
+
 				vms.EXPECT().DeployVirtualMachine(gomock.Any()).Do(
 					func(p interface{}) {
-						displayName, _ := p.(*cloudstack.DeployVirtualMachineParams).GetDisplayname()
+						params := p.(*cloudstack.DeployVirtualMachineParams)
+						displayName, _ := params.GetDisplayname()
 						Ω(displayName == dummies.CAPIMachine.Name).Should(BeTrue())
+
+						b64UserData, _ := params.GetUserdata()
+
+						userData, err := base64.StdEncoding.DecodeString(b64UserData)
+						Ω(err).ToNot(HaveOccurred())
+
+						decompressedUserData, err := decompress(userData)
+						Ω(err).ToNot(HaveOccurred())
+
+						Ω(string(decompressedUserData)).To(Equal(expectUserData))
 					}).Return(deploymentResp, nil)
 
 				Ω(client.GetOrCreateVMInstance(
-					dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, dummies.CSFailureDomain1, dummies.CSAffinityGroup, "")).
+					dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, dummies.CSFailureDomain1, dummies.CSAffinityGroup, expectUserData)).
 					Should(Succeed())
 			}
 
@@ -424,6 +438,69 @@ var _ = Describe("Instance", func() {
 
 			})
 		})
+
+		It("doesn't compress user data", func() {
+			dummies.CSMachine1.Spec.DiskOffering.ID = diskOfferingFakeID
+			dummies.CSMachine1.Spec.Offering.ID = ""
+			dummies.CSMachine1.Spec.Template.ID = ""
+			dummies.CSMachine1.Spec.Offering.Name = "offering"
+			dummies.CSMachine1.Spec.Template.Name = "template"
+			dummies.CSMachine1.Spec.UncompressedUserData = pointer.Bool(true)
+
+			vms.EXPECT().
+				GetVirtualMachinesMetricByID(*dummies.CSMachine1.Spec.InstanceID).
+				Return(nil, -1, notFoundError)
+			vms.EXPECT().
+				GetVirtualMachinesMetricByID(*dummies.CSMachine1.Spec.InstanceID).
+				Return(&cloudstack.VirtualMachinesMetric{}, 1, nil)
+			vms.EXPECT().
+				GetVirtualMachinesMetricByName(dummies.CSMachine1.Name).
+				Return(nil, -1, notFoundError)
+			sos.EXPECT().
+				GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name, gomock.Any()).
+				Return(offeringFakeID, 1, nil)
+			dos.EXPECT().
+				GetDiskOfferingID(dummies.CSMachine1.Spec.DiskOffering.Name, gomock.Any()).
+				Return(diskOfferingFakeID, 1, nil)
+			dos.EXPECT().
+				GetDiskOfferingByID(dummies.CSMachine1.Spec.DiskOffering.ID).
+				Return(&cloudstack.DiskOffering{Iscustomized: false}, 1, nil)
+			ts.EXPECT().
+				GetTemplateID(dummies.CSMachine1.Spec.Template.Name, executableFilter, dummies.Zone1.ID).
+				Return(templateFakeID, 1, nil)
+			vms.EXPECT().
+				NewDeployVirtualMachineParams(offeringFakeID, templateFakeID, dummies.Zone1.ID).
+				Return(&cloudstack.DeployVirtualMachineParams{})
+
+			deploymentResp := &cloudstack.DeployVirtualMachineResponse{
+				Id: *dummies.CSMachine1.Spec.InstanceID,
+			}
+
+			expectUserData := "my special userdata"
+
+			vms.EXPECT().DeployVirtualMachine(gomock.Any()).Do(
+				func(p interface{}) {
+					params := p.(*cloudstack.DeployVirtualMachineParams)
+					displayName, _ := params.GetDisplayname()
+					Ω(displayName == dummies.CAPIMachine.Name).Should(BeTrue())
+
+					// Ensure the user data is only base64 encoded.
+					b64UserData, _ := params.GetUserdata()
+					userData, err := base64.StdEncoding.DecodeString(b64UserData)
+					Ω(err).ToNot(HaveOccurred())
+					Ω(string(userData)).To(Equal(expectUserData))
+				}).Return(deploymentResp, nil)
+
+			err := client.GetOrCreateVMInstance(
+				dummies.CSMachine1,
+				dummies.CAPIMachine,
+				dummies.CSCluster,
+				dummies.CSFailureDomain1,
+				dummies.CSAffinityGroup,
+				expectUserData,
+			)
+			Ω(err).Should(Succeed())
+		})
 	})
 
 	Context("when destroying a VM instance", func() {