E2E test for Resource Modifier & Bug fixes (#6483)

* Add resource modifier e2e test and bug fixes

Signed-off-by: Anshul Ahuja <anshulahuja@microsoft.com>

internal/resourcemodifiers/resource_modifiers.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"regexp"
+	"strconv"
 	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch"
@@ -117,10 +118,10 @@ func (r *ResourceModifierRule) PatchArrayToByteArray() ([]byte, error) {
 }
 
 func (p *JSONPatch) ToString() string {
-	if strings.Contains(p.Value, "\"") {
-		return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value)
+	if addQuotes(p.Value) {
+		return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value)
 	}
-	return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value)
+	return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value)
 }
 
 func ApplyPatch(patch []byte, obj *unstructured.Unstructured, log logrus.FieldLogger) error {
@@ -162,3 +163,26 @@ func decodeStruct(r io.Reader, s interface{}) error {
 	dec.KnownFields(true)
 	return dec.Decode(s)
 }
+
+func addQuotes(value string) bool {
+	if value == "" {
+		return true
+	}
+	// if value is null, then don't add quotes
+	if value == "null" {
+		return false
+	}
+	// if value is a boolean, then don't add quotes
+	if _, err := strconv.ParseBool(value); err == nil {
+		return false
+	}
+	// if value is a json object or array, then don't add quotes.
+	if strings.HasPrefix(value, "{") || strings.HasPrefix(value, "[") {
+		return false
+	}
+	// if value is a number, then don't add quotes
+	if _, err := strconv.ParseFloat(value, 64); err == nil {
+		return false
+	}
+	return true
+}

internal/resourcemodifiers/resource_modifiers_test.go

@@ -185,7 +185,7 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) {
 				"namespace": "foo",
 			},
 			"spec": map[string]interface{}{
-				"replicas": "1",
+				"replicas": int64(1),
 				"template": map[string]interface{}{
 					"metadata": map[string]interface{}{
 						"labels": map[string]interface{}{
@@ -213,7 +213,7 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) {
 				"namespace": "foo",
 			},
 			"spec": map[string]interface{}{
-				"replicas": "2",
+				"replicas": int64(2),
 				"template": map[string]interface{}{
 					"metadata": map[string]interface{}{
 						"labels": map[string]interface{}{
@@ -241,7 +241,7 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) {
 				"namespace": "foo",
 			},
 			"spec": map[string]interface{}{
-				"replicas": "1",
+				"replicas": int64(1),
 				"template": map[string]interface{}{
 					"metadata": map[string]interface{}{
 						"labels": map[string]interface{}{
@@ -614,19 +614,37 @@ func TestJSONPatch_ToString(t *testing.T) {
 				Path:      "/spec/replicas",
 				Value:     "1",
 			},
-			want: `{"op": "test", "from": "", "path": "/spec/replicas", "value": "1"}`,
+			want: `{"op": "test", "from": "", "path": "/spec/replicas", "value": 1}`,
 		},
 		{
-			name: "replace",
+			name: "replace integer",
 			fields: fields{
 				Operation: "replace",
 				Path:      "/spec/replicas",
 				Value:     "2",
 			},
-			want: `{"op": "replace", "from": "", "path": "/spec/replicas", "value": "2"}`,
+			want: `{"op": "replace", "from": "", "path": "/spec/replicas", "value": 2}`,
 		},
 		{
-			name: "add complex interfaces",
+			name: "replace array",
+			fields: fields{
+				Operation: "replace",
+				Path:      "/spec/template/spec/containers/0/ports",
+				Value:     `[{"containerPort": 80}]`,
+			},
+			want: `{"op": "replace", "from": "", "path": "/spec/template/spec/containers/0/ports", "value": [{"containerPort": 80}]}`,
+		},
+		{
+			name: "replace with null",
+			fields: fields{
+				Operation: "replace",
+				Path:      "/spec/template/spec/containers/0/ports",
+				Value:     `null`,
+			},
+			want: `{"op": "replace", "from": "", "path": "/spec/template/spec/containers/0/ports", "value": null}`,
+		},
+		{
+			name: "add json object",
 			fields: fields{
 				Operation: "add",
 				Path:      "/spec/template/spec/containers/0",

pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go

@@ -1,17 +1,34 @@
 //go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
-// Code generated by controller-gen. DO NOT EDIT.
+/*
+Copyright the Velero contributors.
+
+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.
+*/
+
+// Code generated by deepcopy-gen. DO NOT EDIT.
 
 package v2alpha1
 
 import (
-	"k8s.io/apimachinery/pkg/runtime"
+	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *CSISnapshotSpec) DeepCopyInto(out *CSISnapshotSpec) {
 	*out = *in
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSISnapshotSpec.
@@ -31,6 +48,7 @@ func (in *DataDownload) DeepCopyInto(out *DataDownload) {
 	in.ObjectMeta.DeepCopyInto(&out.ObjectMeta)
 	in.Spec.DeepCopyInto(&out.Spec)
 	in.Status.DeepCopyInto(&out.Status)
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataDownload.
@@ -63,6 +81,7 @@ func (in *DataDownloadList) DeepCopyInto(out *DataDownloadList) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataDownloadList.
@@ -95,6 +114,7 @@ func (in *DataDownloadSpec) DeepCopyInto(out *DataDownloadSpec) {
 		}
 	}
 	out.OperationTimeout = in.OperationTimeout
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataDownloadSpec.
@@ -119,6 +139,7 @@ func (in *DataDownloadStatus) DeepCopyInto(out *DataDownloadStatus) {
 		*out = (*in).DeepCopy()
 	}
 	out.Progress = in.Progress
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataDownloadStatus.
@@ -138,6 +159,7 @@ func (in *DataUpload) DeepCopyInto(out *DataUpload) {
 	in.ObjectMeta.DeepCopyInto(&out.ObjectMeta)
 	in.Spec.DeepCopyInto(&out.Spec)
 	in.Status.DeepCopyInto(&out.Status)
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataUpload.
@@ -170,6 +192,7 @@ func (in *DataUploadList) DeepCopyInto(out *DataUploadList) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataUploadList.
@@ -204,6 +227,7 @@ func (in *DataUploadResult) DeepCopyInto(out *DataUploadResult) {
 			}
 		}
 	}
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataUploadResult.
@@ -236,6 +260,7 @@ func (in *DataUploadSpec) DeepCopyInto(out *DataUploadSpec) {
 		}
 	}
 	out.OperationTimeout = in.OperationTimeout
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataUploadSpec.
@@ -271,6 +296,7 @@ func (in *DataUploadStatus) DeepCopyInto(out *DataUploadStatus) {
 		*out = (*in).DeepCopy()
 	}
 	out.Progress = in.Progress
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataUploadStatus.
@@ -286,6 +312,7 @@ func (in *DataUploadStatus) DeepCopy() *DataUploadStatus {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *TargetVolumeSpec) DeepCopyInto(out *TargetVolumeSpec) {
 	*out = *in
+	return
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TargetVolumeSpec.

pkg/cmd/cli/restore/create.go

@@ -281,8 +281,10 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
 
 	if o.ResourceModifierConfigMap != "" {
 		resModifiers = &corev1.TypedLocalObjectReference{
-			Kind: resourcemodifiers.ConfigmapRefType,
-			Name: o.ResourceModifierConfigMap,
+			// Group for core API is ""
+			APIGroup: &corev1.SchemeGroupVersion.Group,
+			Kind:     resourcemodifiers.ConfigmapRefType,
+			Name:     o.ResourceModifierConfigMap,
 		}
 	}
 

test/e2e/README.md

@@ -126,7 +126,7 @@ For programming conventions, Take ResourcePoliciesCase case for example:
 ##### CreateResources
 - It's better to set a global timeout in CreateResources function which is the real beginning of one e2e test
 ##### Destroy
-- It's only clean up resources in currently test namespaces, if wantting to clean up all resources including resources created which is not in currently test namespaces, it's better to override base Destroy function
+- It only cleans up resources in currently test namespaces, if you wish to clean up all resources including resources created which are not in currently test namespaces, it's better to override base Destroy function
 ##### Clean
 - Clean function only clean resources in namespaces which has the prefix CaseBaseName. So the the names of test namespaces should start with prefix of CaseBaseName.
 - If wantting to clean up all resources including resources created which is not in currently test namespaces, it's better to override base Clean function

test/e2e/e2e_suite_test.go

@@ -39,6 +39,7 @@ import (
 	. "github.com/vmware-tanzu/velero/test/e2e/privilegesmgmt"
 	. "github.com/vmware-tanzu/velero/test/e2e/pv-backup"
 	. "github.com/vmware-tanzu/velero/test/e2e/resource-filtering"
+	. "github.com/vmware-tanzu/velero/test/e2e/resourcemodifiers"
 	. "github.com/vmware-tanzu/velero/test/e2e/resourcepolicies"
 	. "github.com/vmware-tanzu/velero/test/e2e/scale"
 	. "github.com/vmware-tanzu/velero/test/e2e/schedule"
@@ -119,6 +120,8 @@ var _ = Describe("[ResourceFiltering][IncludeResources][Restore] Velero test on
 var _ = Describe("[ResourceFiltering][LabelSelector] Velero test on backup include resources matching the label selector", BackupWithLabelSelector)
 var _ = Describe("[ResourceFiltering][ResourcePolicies] Velero test on skip backup of volume by resource policies", ResourcePoliciesTest)
 
+var _ = Describe("[ResourceModifier][Restore] Velero test on resource modifiers from the cluster restore", ResourceModifiersTest)
+
 var _ = Describe("[Backups][Deletion][Restic] Velero tests of Restic backup deletion", BackupDeletionWithRestic)
 var _ = Describe("[Backups][Deletion][Snapshot] Velero tests of snapshot backup deletion", BackupDeletionWithSnapshots)
 var _ = Describe("[Backups][TTL][LongTime] Local backups and restic repos will be deleted once the corresponding backup storage location is deleted", TTLTest)

test/e2e/resourcemodifiers/resource_modifiers.go

@@ -0,0 +1,164 @@
+/*
+Copyright 2021 the Velero contributors.
+
+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 resourcemodifiers
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/pkg/errors"
+
+	. "github.com/vmware-tanzu/velero/test/e2e"
+	. "github.com/vmware-tanzu/velero/test/e2e/test"
+	. "github.com/vmware-tanzu/velero/test/e2e/util/k8s"
+)
+
+var yamlData = `
+version: v1
+resourceModifierRules:
+- conditions:
+    groupKind: deployments.apps
+    resourceNameRegex: "resource-modifiers-.*"
+  patches:
+  - operation: add
+    path: "/spec/template/spec/containers/1"
+    value: "{\"name\": \"nginx\", \"image\": \"nginx:1.14.2\", \"ports\": [{\"containerPort\": 80}]}"
+  - operation: replace
+    path: "/spec/replicas"
+    value: "2"
+`
+
+type ResourceModifiersCase struct {
+	TestCase
+	cmName, yamlConfig string
+}
+
+var ResourceModifiersTest func() = TestFunc(&ResourceModifiersCase{})
+
+func (r *ResourceModifiersCase) Init() error {
+	// generate random number as UUIDgen and set one default timeout duration
+	r.TestCase.Init()
+
+	// generate variable names based on CaseBaseName + UUIDgen
+	r.CaseBaseName = "resource-modifiers-" + r.UUIDgen
+	r.BackupName = "backup-" + r.CaseBaseName
+	r.RestoreName = "restore-" + r.CaseBaseName
+	r.cmName = "cm-" + r.CaseBaseName
+
+	// generate namespaces by NamespacesTotal
+	r.NamespacesTotal = 1
+	r.NSIncluded = &[]string{}
+	for nsNum := 0; nsNum < r.NamespacesTotal; nsNum++ {
+		createNSName := fmt.Sprintf("%s-%00000d", r.CaseBaseName, nsNum)
+		*r.NSIncluded = append(*r.NSIncluded, createNSName)
+	}
+
+	// assign values to the inner variable for specific case
+	r.yamlConfig = yamlData
+	r.VeleroCfg = VeleroCfg
+	r.Client = *r.VeleroCfg.ClientToInstallVelero
+	r.VeleroCfg.UseVolumeSnapshots = false
+	r.VeleroCfg.UseNodeAgent = false
+
+	r.BackupArgs = []string{
+		"create", "--namespace", VeleroCfg.VeleroNamespace, "backup", r.BackupName,
+		"--include-namespaces", strings.Join(*r.NSIncluded, ","),
+		"--snapshot-volumes=false", "--wait",
+	}
+
+	r.RestoreArgs = []string{
+		"create", "--namespace", VeleroCfg.VeleroNamespace, "restore", r.RestoreName,
+		"--resource-modifier-configmap", r.cmName,
+		"--from-backup", r.BackupName, "--wait",
+	}
+
+	// Message output by ginkgo
+	r.TestMsg = &TestMSG{
+		Desc:      "Validate resource modifiers",
+		FailedMSG: "Failed to apply and / or validate resource modifiers in restored resources.",
+		Text:      "Should be able to apply and validate resource modifiers in restored resources.",
+	}
+	return nil
+}
+
+func (r *ResourceModifiersCase) CreateResources() error {
+	// It's better to set a global timeout in CreateResources function which is the real beginning of one e2e test
+	r.Ctx, r.CtxCancel = context.WithTimeout(context.Background(), 10*time.Minute)
+
+	By(fmt.Sprintf("Create configmap %s in namespaces %s for workload\n", r.cmName, r.VeleroCfg.VeleroNamespace), func() {
+		Expect(CreateConfigMapFromYAMLData(r.Client.ClientGo, r.yamlConfig, r.cmName, r.VeleroCfg.VeleroNamespace)).To(Succeed(), fmt.Sprintf("Failed to create configmap %s in namespaces %s for workload\n", r.cmName, r.VeleroCfg.VeleroNamespace))
+	})
+
+	By(fmt.Sprintf("Waiting for configmap %s in namespaces %s ready\n", r.cmName, r.VeleroCfg.VeleroNamespace), func() {
+		Expect(WaitForConfigMapComplete(r.Client.ClientGo, r.VeleroCfg.VeleroNamespace, r.cmName)).To(Succeed(), fmt.Sprintf("Failed to wait configmap %s in namespaces %s ready\n", r.cmName, r.VeleroCfg.VeleroNamespace))
+	})
+
+	for nsNum := 0; nsNum < r.NamespacesTotal; nsNum++ {
+		namespace := fmt.Sprintf("%s-%00000d", r.CaseBaseName, nsNum)
+		By(fmt.Sprintf("Create namespaces %s for workload\n", namespace), func() {
+			Expect(CreateNamespace(r.Ctx, r.Client, namespace)).To(Succeed(), fmt.Sprintf("Failed to create namespace %s", namespace))
+		})
+
+		By(fmt.Sprintf("Creating deployment in namespaces ...%s\n", namespace), func() {
+			Expect(r.createDeployment(namespace)).To(Succeed(), fmt.Sprintf("Failed to create deployment namespace %s", namespace))
+		})
+	}
+	return nil
+}
+
+func (r *ResourceModifiersCase) Verify() error {
+	for _, ns := range *r.NSIncluded {
+		By("Verify deployment has updated values", func() {
+			deploy, err := GetDeployment(r.Client.ClientGo, ns, r.CaseBaseName)
+			Expect(err).To(BeNil(), fmt.Sprintf("Failed to get deployment %s in namespace %s", r.CaseBaseName, ns))
+
+			Expect(*deploy.Spec.Replicas).To(Equal(int32(2)), fmt.Sprintf("Failed to verify deployment %s's replicas in namespace %s", r.CaseBaseName, ns))
+			Expect(deploy.Spec.Template.Spec.Containers[1].Image).To(Equal("nginx:1.14.2"), fmt.Sprintf("Failed to verify deployment %s's image in namespace %s", r.CaseBaseName, ns))
+		})
+	}
+	return nil
+}
+
+func (r *ResourceModifiersCase) Clean() error {
+	// If created some resources which is not in current test namespace, we NEED to override the base Clean function
+	if !r.VeleroCfg.Debug {
+		if err := DeleteConfigmap(r.Client.ClientGo, r.VeleroCfg.VeleroNamespace, r.cmName); err != nil {
+			return err
+		}
+
+		return r.GetTestCase().Clean() // only clean up resources in test namespace
+	}
+	return nil
+}
+
+func (r *ResourceModifiersCase) createDeployment(namespace string) error {
+	deployment := NewDeployment(r.CaseBaseName, namespace, 1, map[string]string{"app": "test"}, nil).Result()
+	deployment, err := CreateDeployment(r.Client.ClientGo, namespace, deployment)
+	if err != nil {
+		return errors.Wrap(err, fmt.Sprintf("failed to create deloyment %s the namespace %q", deployment.Name, namespace))
+	}
+	err = WaitForReadyDeployment(r.Client.ClientGo, namespace, deployment.Name)
+	if err != nil {
+		return errors.Wrap(err, fmt.Sprintf("failed to wait for deployment %s to be ready in namespace: %q", deployment.Name, namespace))
+	}
+	return nil
+}