From e753a08f856e0e0483f357c609da384a261a23f3 Mon Sep 17 00:00:00 2001 From: Anshul Ahuja Date: Wed, 16 Aug 2023 08:39:59 +0530 Subject: [PATCH] E2E test for Resource Modifier & Bug fixes (#6483) * Add resource modifier e2e test and bug fixes Signed-off-by: Anshul Ahuja --- .../resourcemodifiers/resource_modifiers.go | 30 +++- .../resource_modifiers_test.go | 32 +++- .../velero/v2alpha1/zz_generated.deepcopy.go | 31 +++- pkg/cmd/cli/restore/create.go | 6 +- test/e2e/README.md | 2 +- test/e2e/e2e_suite_test.go | 3 + .../resourcemodifiers/resource_modifiers.go | 164 ++++++++++++++++++ 7 files changed, 253 insertions(+), 15 deletions(-) create mode 100644 test/e2e/resourcemodifiers/resource_modifiers.go diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index 17959a99a5..a4d24012fb 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/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 +} diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index f795d18e50..911425f381 100644 --- a/internal/resourcemodifiers/resource_modifiers_test.go +++ b/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", diff --git a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go index 9a9afaa6d9..6f2f0441f5 100644 --- a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go +++ b/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. diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index d2acbe9188..410da5a346 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/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, } } diff --git a/test/e2e/README.md b/test/e2e/README.md index f845752fe8..30d8590669 100644 --- a/test/e2e/README.md +++ b/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 diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index d050b37dd2..82791f63eb 100644 --- a/test/e2e/e2e_suite_test.go +++ b/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) diff --git a/test/e2e/resourcemodifiers/resource_modifiers.go b/test/e2e/resourcemodifiers/resource_modifiers.go new file mode 100644 index 0000000000..e163038ea4 --- /dev/null +++ b/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 +}