From 2090d66d2257e03210893f21efbc4d38bcafd1a2 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Thu, 7 Nov 2024 13:38:09 +0530 Subject: [PATCH 01/12] Adding VolumeAttributes validations in resource policy Signed-off-by: mayaggar --- internal/resourcepolicies/volume_resources.go | 24 ++++++++++++++++--- .../volume_resources_validator.go | 2 ++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/internal/resourcepolicies/volume_resources.go b/internal/resourcepolicies/volume_resources.go index fd1b8182ac..e9ac8ddfc3 100644 --- a/internal/resourcepolicies/volume_resources.go +++ b/internal/resourcepolicies/volume_resources.go @@ -60,7 +60,7 @@ func (s *structuredVolume) parsePV(pv *corev1api.PersistentVolume) { csi := pv.Spec.CSI if csi != nil { - s.csi = &csiVolumeSource{Driver: csi.Driver} + s.csi = &csiVolumeSource{Driver: csi.Driver, VolumeAttributes: csi.VolumeAttributes} } s.volumeType = getVolumeTypeFromPV(pv) @@ -74,7 +74,7 @@ func (s *structuredVolume) parsePodVolume(vol *corev1api.Volume) { csi := vol.CSI if csi != nil { - s.csi = &csiVolumeSource{Driver: csi.Driver} + s.csi = &csiVolumeSource{Driver: csi.Driver, VolumeAttributes: csi.VolumeAttributes} } s.volumeType = getVolumeTypeFromVolume(vol) @@ -160,7 +160,25 @@ func (c *csiCondition) match(v *structuredVolume) bool { return false } - return c.csi.Driver == v.csi.Driver + if c.csi.Driver != v.csi.Driver { + return false + } + + if c.csi.VolumeAttributes == nil || len(c.csi.VolumeAttributes) == 0 { + return true + } + + if v.csi.VolumeAttributes == nil || len(v.csi.VolumeAttributes) == 0 { + return false + } + + for key, value := range c.csi.VolumeAttributes { // match csi: {driver: "csi", volumeAttributes: {key: value}} + if value != v.csi.VolumeAttributes[key] { // if the value of the key does not match, return false + return false + } + } + + return true } // parseCapacity parse string into capacity format diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index cf9a40c0fc..ad6e676892 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -27,6 +27,8 @@ const currentSupportDataVersion = "v1" type csiVolumeSource struct { Driver string `yaml:"driver,omitempty"` + // CSI volume attributes + VolumeAttributes map[string]string `yaml:"volumeAttributes,omitempty"` } type nFSVolumeSource struct { From 65f1ddc07ab77213b7ab41b927849207e0d8e120 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Thu, 7 Nov 2024 17:05:04 +0530 Subject: [PATCH 02/12] adding tests Signed-off-by: mayaggar --- internal/resourcepolicies/volume_resources.go | 4 +- .../resourcepolicies/volume_resources_test.go | 54 +++++++++- .../volume_resources_validator_test.go | 100 ++++++++++++++++++ 3 files changed, 151 insertions(+), 7 deletions(-) diff --git a/internal/resourcepolicies/volume_resources.go b/internal/resourcepolicies/volume_resources.go index e9ac8ddfc3..4f3ef765aa 100644 --- a/internal/resourcepolicies/volume_resources.go +++ b/internal/resourcepolicies/volume_resources.go @@ -172,8 +172,8 @@ func (c *csiCondition) match(v *structuredVolume) bool { return false } - for key, value := range c.csi.VolumeAttributes { // match csi: {driver: "csi", volumeAttributes: {key: value}} - if value != v.csi.VolumeAttributes[key] { // if the value of the key does not match, return false + for key, value := range c.csi.VolumeAttributes { + if value != v.csi.VolumeAttributes[key] { return false } } diff --git a/internal/resourcepolicies/volume_resources_test.go b/internal/resourcepolicies/volume_resources_test.go index 4d5d7a743a..bba89ad350 100644 --- a/internal/resourcepolicies/volume_resources_test.go +++ b/internal/resourcepolicies/volume_resources_test.go @@ -201,23 +201,47 @@ func TestCSIConditionMatch(t *testing.T) { expectedMatch bool }{ { - name: "match csi condition", + name: "match csi driver condition", condition: &csiCondition{&csiVolumeSource{Driver: "test"}}, volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test"}), expectedMatch: true, }, { - name: "empty csi condition", + name: "empty csi driver condition", condition: &csiCondition{nil}, volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test"}), expectedMatch: true, }, { - name: "empty csi volume", + name: "empty csi driver volume", condition: &csiCondition{&csiVolumeSource{Driver: "test"}}, volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{}), expectedMatch: false, }, + { + name: "match csi volumeAttributes condition", + condition: &csiCondition{&csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}}, + volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}), + expectedMatch: true, + }, + { + name: "empty csi volumeAttributes condition", + condition: &csiCondition{&csiVolumeSource{Driver: "test"}}, + volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}), + expectedMatch: true, + }, + { + name: "empty csi volumeAttributes volume // SMB volumes", + condition: &csiCondition{&csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}}, + volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": ""}}), + expectedMatch: false, + }, + { + name: "empty csi volumeAttributes volume // SMB volumes", + condition: &csiCondition{&csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}}, + volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test"}), + expectedMatch: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -348,9 +372,19 @@ func TestParsePodVolume(t *testing.T) { if tc.expectedCSI != nil { if structuredVolume.csi == nil { t.Errorf("Expected a non-nil CSI volume source") - } else if *tc.expectedCSI != *structuredVolume.csi { + } else if tc.expectedCSI.Driver != structuredVolume.csi.Driver { t.Errorf("CSI volume source does not match expected value") } + // Check volumeAttributes + if len(tc.expectedCSI.VolumeAttributes) != len(structuredVolume.csi.VolumeAttributes) { + t.Errorf("CSI volume attributes does not match expected value") + } else { + for k, v := range tc.expectedCSI.VolumeAttributes { + if structuredVolume.csi.VolumeAttributes[k] != v { + t.Errorf("CSI volume attributes does not match expected value") + } + } + } } }) } @@ -415,9 +449,19 @@ func TestParsePV(t *testing.T) { if tc.expectedCSI != nil { if structuredVolume.csi == nil { t.Errorf("Expected a non-nil CSI volume source") - } else if *tc.expectedCSI != *structuredVolume.csi { + } else if tc.expectedCSI.Driver != structuredVolume.csi.Driver { t.Errorf("CSI volume source does not match expected value") } + // Check volumeAttributes + if len(tc.expectedCSI.VolumeAttributes) != len(structuredVolume.csi.VolumeAttributes) { + t.Errorf("CSI volume attributes does not match expected value") + } else { + for k, v := range tc.expectedCSI.VolumeAttributes { + if structuredVolume.csi.VolumeAttributes[k] != v { + t.Errorf("CSI volume attributes does not match expected value") + } + } + } } }) } diff --git a/internal/resourcepolicies/volume_resources_validator_test.go b/internal/resourcepolicies/volume_resources_validator_test.go index 1cbc6d7325..fb8ce145dd 100644 --- a/internal/resourcepolicies/volume_resources_validator_test.go +++ b/internal/resourcepolicies/volume_resources_validator_test.go @@ -165,6 +165,47 @@ func TestValidate(t *testing.T) { }, wantErr: true, }, + { + name: "error format of csi driver", + res: &ResourcePolicies{ + Version: "v1", + VolumePolicies: []VolumePolicy{ + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "capacity": "0,10Gi", + "storageClass": []string{"gp2", "ebs-sc"}, + "csi": interface{}( + map[string]interface{}{ + "driver": []string{"aws.efs.csi.driver"}, + }), + }, + }, + }, + }, + wantErr: true, + }, + { + name: "error format of csi driver volumeAttributes", + res: &ResourcePolicies{ + Version: "v1", + VolumePolicies: []VolumePolicy{ + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "capacity": "0,10Gi", + "storageClass": []string{"gp2", "ebs-sc"}, + "csi": interface{}( + map[string]interface{}{ + "driver": "aws.efs.csi.driver", + "volumeAttributes": "test", + }), + }, + }, + }, + }, + wantErr: true, + }, { name: "unsupported version", res: &ResourcePolicies{ @@ -220,6 +261,65 @@ func TestValidate(t *testing.T) { }, wantErr: true, }, + { + name: "supported format volume policies only csi driver", + res: &ResourcePolicies{ + Version: "v1", + VolumePolicies: []VolumePolicy{ + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "csi": interface{}( + map[string]interface{}{ + "driver": "aws.efs.csi.driver", + }), + }, + }, + }, + }, + wantErr: false, + }, + { + name: "supported format volume policies only csi volumeattributes", + res: &ResourcePolicies{ + Version: "v1", + VolumePolicies: []VolumePolicy{ + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "csi": interface{}( + map[string]interface{}{ + "volumeAttributes": map[string]string{ + "key1": "value1", + }, + }), + }, + }, + }, + }, + wantErr: false, + }, + { + name: "supported format volume policies with csi driver and volumeattributes", + res: &ResourcePolicies{ + Version: "v1", + VolumePolicies: []VolumePolicy{ + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "csi": interface{}( + map[string]interface{}{ + "driver": "aws.efs.csi.driver", + "volumeAttributes": map[string]string{ + "key1": "value1", + }, + }), + }, + }, + }, + }, + wantErr: false, + }, { name: "supported format volume policies", res: &ResourcePolicies{ From 1adaf0a042ca339e26f2187cdb57b0f9a84a2660 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Fri, 8 Nov 2024 12:31:30 +0530 Subject: [PATCH 03/12] adding tests Signed-off-by: mayaggar --- .../resource_policies_test.go | 204 ++++++++++++++++-- internal/resourcepolicies/volume_resources.go | 6 +- .../resourcepolicies/volume_resources_test.go | 13 +- .../volume_resources_validator.go | 5 +- .../volume_resources_validator_test.go | 2 +- 5 files changed, 208 insertions(+), 22 deletions(-) diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 4fd705731e..6586d7f284 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -95,18 +95,30 @@ func TestLoadResourcePolicies(t *testing.T) { { name: "supported formart volume policies", yamlData: `version: v1 - volumePolicies: - - conditions: - capacity: "0,100Gi" - csi: - driver: aws.efs.csi.driver - nfs: {} - storageClass: - - gp2 - - ebs-sc - action: - type: skip`, - wantErr: true, +volumePolicies: + - conditions: + capacity: '0,100Gi' + csi: + driver: aws.efs.csi.driver + action: + type: skip +`, + wantErr: false, + }, + { + name: "supported formart csi driver with volumeAttributes for volume policies", + yamlData: `version: v1 +volumePolicies: + - conditions: + capacity: '0,100Gi' + csi: + driver: aws.efs.csi.driver + volumeAttributes: + key1: value1 + action: + type: skip +`, + wantErr: false, }, } for _, tc := range testCases { @@ -298,7 +310,173 @@ volumePolicies: skip: false, }, { - name: "csi not configured", + name: "Skip AFS CSI condition with Disk volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: files.csi.driver + action: + type: skip`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "disks.csi.driver"}, + }}, + }, + skip: false, + }, + { + name: "Skip AFS CSI condition with AFS volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: files.csi.driver + action: + type: skip`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "files.csi.driver"}, + }}, + }, + skip: true, + }, + { + name: "Skip AFS NFS CSI condition with Disk volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip +`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "disks.csi.driver"}, + }}, + }, + skip: false, + }, + { + name: "Skip AFS NFS CSI condition with AFS SMB volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip +`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "files.csi.driver", VolumeAttributes: map[string]string{"key1": "val1"}}, + }}, + }, + skip: false, + }, + { + name: "Skip AFS NFS CSI condition with AFS NFS volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip +`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "files.csi.driver", VolumeAttributes: map[string]string{"protocol": "nfs"}}, + }}, + }, + skip: true, + }, + { + name: "Skip Disk and AFS NFS CSI condition with Disk volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: disks.csi.driver + action: + type: skip + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "disks.csi.driver", VolumeAttributes: map[string]string{"key1": "val1"}}, + }}, + }, + skip: true, + }, + { + name: "Skip Disk and AFS NFS CSI condition with AFS SMB volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: disks.csi.driver + action: + type: skip + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "files.csi.driver", VolumeAttributes: map[string]string{"key1": "val1"}}, + }}, + }, + skip: false, + }, + { + name: "Skip Disk and AFS NFS CSI condition with AFS NFS volumes", + yamlData: `version: v1 +volumePolicies: + - conditions: + csi: + driver: disks.csi.driver + action: + type: skip + - conditions: + csi: + driver: files.csi.driver + volumeAttributes: + protocol: nfs + action: + type: skip`, + vol: &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{Driver: "files.csi.driver", VolumeAttributes: map[string]string{"key1": "val1", "protocol": "nfs"}}, + }}, + }, + skip: true, + }, + { + name: "csi not configured and testing capacity condition", yamlData: `version: v1 volumePolicies: - conditions: diff --git a/internal/resourcepolicies/volume_resources.go b/internal/resourcepolicies/volume_resources.go index 4f3ef765aa..c1cd1e581a 100644 --- a/internal/resourcepolicies/volume_resources.go +++ b/internal/resourcepolicies/volume_resources.go @@ -153,7 +153,11 @@ func (c *csiCondition) match(v *structuredVolume) bool { } if c.csi.Driver == "" { // match csi: {} - return v.csi != nil + if v.csi != nil { + return true + } + + return false } if v.csi == nil { diff --git a/internal/resourcepolicies/volume_resources_test.go b/internal/resourcepolicies/volume_resources_test.go index bba89ad350..9eca07ef75 100644 --- a/internal/resourcepolicies/volume_resources_test.go +++ b/internal/resourcepolicies/volume_resources_test.go @@ -231,13 +231,13 @@ func TestCSIConditionMatch(t *testing.T) { expectedMatch: true, }, { - name: "empty csi volumeAttributes volume // SMB volumes", + name: "empty csi volumeAttributes volume", condition: &csiCondition{&csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}}, volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": ""}}), expectedMatch: false, }, { - name: "empty csi volumeAttributes volume // SMB volumes", + name: "empty csi volumeAttributes volume", condition: &csiCondition{&csiVolumeSource{Driver: "test", VolumeAttributes: map[string]string{"protocol": "nfs"}}}, volume: setStructuredVolume(*resource.NewQuantity(0, resource.BinarySI), "", nil, &csiVolumeSource{Driver: "test"}), expectedMatch: false, @@ -326,7 +326,8 @@ func TestParsePodVolume(t *testing.T) { } csiVolume := corev1api.Volume{} csiVolume.CSI = &corev1api.CSIVolumeSource{ - Driver: "csi.example.com", + Driver: "csi.example.com", + VolumeAttributes: map[string]string{"protocol": "nfs"}, } emptyVolume := corev1api.Volume{} @@ -345,7 +346,7 @@ func TestParsePodVolume(t *testing.T) { { name: "CSI volume", inputVolume: &csiVolume, - expectedCSI: &csiVolumeSource{Driver: "csi.example.com"}, + expectedCSI: &csiVolumeSource{Driver: "csi.example.com", VolumeAttributes: map[string]string{"protocol": "nfs"}}, }, { name: "Empty volume", @@ -397,7 +398,7 @@ func TestParsePV(t *testing.T) { nfsVolume.Spec.NFS = &corev1api.NFSVolumeSource{Server: "nfs.example.com", Path: "/exports/data"} csiVolume := corev1api.PersistentVolume{} csiVolume.Spec.Capacity = corev1api.ResourceList{corev1api.ResourceStorage: resource.MustParse("2Gi")} - csiVolume.Spec.CSI = &corev1api.CSIPersistentVolumeSource{Driver: "csi.example.com"} + csiVolume.Spec.CSI = &corev1api.CSIPersistentVolumeSource{Driver: "csi.example.com", VolumeAttributes: map[string]string{"protocol": "nfs"}} emptyVolume := corev1api.PersistentVolume{} // Test cases @@ -417,7 +418,7 @@ func TestParsePV(t *testing.T) { name: "CSI volume", inputVolume: &csiVolume, expectedNFS: nil, - expectedCSI: &csiVolumeSource{Driver: "csi.example.com"}, + expectedCSI: &csiVolumeSource{Driver: "csi.example.com", VolumeAttributes: map[string]string{"protocol": "nfs"}}, }, { name: "Empty volume", diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index ad6e676892..6064b89f4b 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -70,7 +70,10 @@ func (c *nfsCondition) validate() error { } func (c *csiCondition) validate() error { - // validate by yamlv3 + if c != nil && c.csi != nil && c.csi.Driver == "" && c.csi.VolumeAttributes != nil { + return errors.New("csi driver should not be empty") + } + return nil } diff --git a/internal/resourcepolicies/volume_resources_validator_test.go b/internal/resourcepolicies/volume_resources_validator_test.go index fb8ce145dd..5bd9e1b9c5 100644 --- a/internal/resourcepolicies/volume_resources_validator_test.go +++ b/internal/resourcepolicies/volume_resources_validator_test.go @@ -297,7 +297,7 @@ func TestValidate(t *testing.T) { }, }, }, - wantErr: false, + wantErr: true, }, { name: "supported format volume policies with csi driver and volumeattributes", From 93a3d6319c580b9535d475e304a5e1e9283cd832 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Fri, 8 Nov 2024 12:57:47 +0530 Subject: [PATCH 04/12] adding tests Signed-off-by: mayaggar --- .../resource_policies_test.go | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 6586d7f284..de57126355 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -147,6 +147,16 @@ func TestGetResourceMatchedAction(t *testing.T) { }), }, }, + { + Action: Action{Type: "skip"}, + Conditions: map[string]interface{}{ + "csi": interface{}( + map[string]interface{}{ + "driver": "files.csi.driver", + "volumeAttributes": map[string]string{"protocol": "nfs"}, + }), + }, + }, { Action: Action{Type: "snapshot"}, Conditions: map[string]interface{}{ @@ -184,6 +194,24 @@ func TestGetResourceMatchedAction(t *testing.T) { }, expectedAction: &Action{Type: "skip"}, }, + { + name: "match policy AFS NFS", + volume: &structuredVolume{ + capacity: *resource.NewQuantity(5<<30, resource.BinarySI), + storageClass: "afs-nfs", + csi: &csiVolumeSource{Driver: "files.csi.driver", VolumeAttributes: map[string]string{"protocol": "nfs"}}, + }, + expectedAction: &Action{Type: "skip"}, + }, + { + name: "match policy AFS SMB", + volume: &structuredVolume{ + capacity: *resource.NewQuantity(5<<30, resource.BinarySI), + storageClass: "afs-smb", + csi: &csiVolumeSource{Driver: "files.csi.driver"}, + }, + expectedAction: nil, + }, { name: "both matches return the first policy", volume: &structuredVolume{ @@ -238,7 +266,7 @@ func TestGetResourcePoliciesFromConfig(t *testing.T) { Namespace: "test-namespace", }, Data: map[string]string{ - "test-data": "version: v1\nvolumePolicies:\n- conditions:\n capacity: '0,10Gi'\n action:\n type: skip", + "test-data": "version: v1\nvolumePolicies:\n - conditions:\n capacity: '0,10Gi'\n csi:\n driver: disks.csi.driver\n action:\n type: skip\n - conditions:\n csi:\n driver: files.csi.driver\n volumeAttributes:\n protocol: nfs\n action:\n type: skip", }, } @@ -248,13 +276,27 @@ func TestGetResourcePoliciesFromConfig(t *testing.T) { // Check that the returned resourcePolicies object contains the expected data assert.Equal(t, "v1", resPolicies.version) - assert.Len(t, resPolicies.volumePolicies, 1) + assert.Len(t, resPolicies.volumePolicies, 2) policies := ResourcePolicies{ Version: "v1", VolumePolicies: []VolumePolicy{ { Conditions: map[string]interface{}{ "capacity": "0,10Gi", + "csi": map[string]interface{}{ + "driver": "disks.csi.driver", + }, + }, + Action: Action{ + Type: Skip, + }, + }, + { + Conditions: map[string]interface{}{ + "csi": map[string]interface{}{ + "driver": "files.csi.driver", + "volumeAttributes": map[string]string{"protocol": "nfs"}, + }, }, Action: Action{ Type: Skip, From db886962356753603159131d0c595f4406c8b3d6 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Fri, 8 Nov 2024 15:54:28 +0530 Subject: [PATCH 05/12] added changelog Signed-off-by: mayaggar --- changelogs/unreleased/8382-mayankagg9722 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/8382-mayankagg9722 diff --git a/changelogs/unreleased/8382-mayankagg9722 b/changelogs/unreleased/8382-mayankagg9722 new file mode 100644 index 0000000000..bd7581bee0 --- /dev/null +++ b/changelogs/unreleased/8382-mayankagg9722 @@ -0,0 +1 @@ +Adding support in velero Resource Policies for filtering PVs based on additional VolumeAttributes properties under CSI PVs \ No newline at end of file From d6850c584ed7f9816bf8f294dc313eb42324b0c8 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Fri, 8 Nov 2024 16:10:42 +0530 Subject: [PATCH 06/12] changelog Signed-off-by: mayaggar --- changelogs/unreleased/{8382-mayankagg9722 => 8383-mayankagg9722} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelogs/unreleased/{8382-mayankagg9722 => 8383-mayankagg9722} (100%) diff --git a/changelogs/unreleased/8382-mayankagg9722 b/changelogs/unreleased/8383-mayankagg9722 similarity index 100% rename from changelogs/unreleased/8382-mayankagg9722 rename to changelogs/unreleased/8383-mayankagg9722 From 306cc20a90a9abc5ed823df437b3fe1be122869d Mon Sep 17 00:00:00 2001 From: mayaggar Date: Tue, 12 Nov 2024 21:11:56 +0530 Subject: [PATCH 07/12] design spec Signed-off-by: mayaggar --- ...rting-volumeattributes-resource-policy.md | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 design/Implemented/supporting-volumeattributes-resource-policy.md diff --git a/design/Implemented/supporting-volumeattributes-resource-policy.md b/design/Implemented/supporting-volumeattributes-resource-policy.md new file mode 100644 index 0000000000..76f382be9f --- /dev/null +++ b/design/Implemented/supporting-volumeattributes-resource-policy.md @@ -0,0 +1,72 @@ +# Adding Support For VolumeAttributes in Resource Policy + +## Abstract +Currently [Velero Resource policies](https://velero.io/docs/main/resource-filtering/#creating-resource-policies) are only supporting "Driver" to be filtered for [CSI volume conditions](https://github.com/vmware-tanzu/velero/blob/8e23752a6ea83f101bd94a69dcf17f519a805388/internal/resourcepolicies/volume_resources_validator.go#L28) + +If user want to skip certain CSI volumes based on other volume attributes like protocol or SKU, etc, they can't do it with the current Velero resource policies. It would be convenient if Velero resource policies could be extended to filter on volume attributes along with existing driver filter in the resource policies `conditions` to handle the backup of volumes just by `some specific volumes attributes conditions`. + +## Background +As of Today, Velero resource policy already provides us the way to filter volumes based on the `driver` name. But it's not enough to handle the volumes based on other volume attributes like protocol, SKU, etc. + +## Example: + - Provision Azure NFS: Define the Storage class with `protocol: nfs` under storage class parameters to provision [CSI NFS Azure File Shares](https://learn.microsoft.com/en-us/azure/aks/azure-files-csi#nfs-file-shares). + - User wants to back up AFS (Azure file shares) but only want to backup `SMB` type of file share volumes and not `NFS` file share volumes. + +## Goals +- Introducing support for `VolumeAttributes` filter along with `driver` filter in CSI volume conditions to handle volumes. + +## Non-Goals +- We are only bringing additional support to only handle volumes for backup and do not support restore. +- Currently, only handles volumes, and does not support other resources. + +## Use-cases/Scenarios +### Skip backup volumes by some volume attributes: +Users want to skip PV with the requirements: +- option to skip specified PV on volume attributes type (like Protocol as NFS, SMB, etc) + +## High-Level Design +Modifying the existing Resource Policies code for [csiVolumeSource](https://github.com/vmware-tanzu/velero/blob/8e23752a6ea83f101bd94a69dcf17f519a805388/internal/resourcepolicies/volume_resources_validator.go#L28C6-L28C22) to add the new `VolumeAttributes` filter for CSI volumes and adding validations in existing [csiCondition](https://github.com/vmware-tanzu/velero/blob/8e23752a6ea83f101bd94a69dcf17f519a805388/internal/resourcepolicies/volume_resources.go#L150) to match with volume attributes in the conditions from Resource Policy config map and original persistent volume. + +## Detailed Design +The volume resources policies should contain a list of policies which is the combination of conditions and related `action`, when target volumes meet the conditions, the related `action` will take effection. + +Below is the API Design for the user configuration: + +### API Design +```go +type csiVolumeSource struct { + Driver string `yaml:"driver,omitempty"` + // [NEW] CSI volume attributes + VolumeAttributes map[string]string `yaml:"volumeAttributes,omitempty"` +} +``` + +The policies YAML config file would look like this: +```yaml +version: v1 +volumePolicies: + - conditions: + csi: + driver: disk.csi.azure.com + action: + type: skip + - conditions: + csi: + driver: file.csi.azure.com + volumeAttributes: + protocol: nfs + action: + type: skip` +``` + +### New Supported Conditions +#### VolumeAttributes +Existing CSI Volume Condition can now add `volumeAttributes` which will be key and value pairs. + + Specify details for the related volume source (currently only csi driver is supported filter) + ```yaml + csi: // match volume using `file.csi.azure.com` and with volumeAttributes protocol as nfs + driver: file.csi.azure.com + volumeAttributes: + protocol: nfs + ``` \ No newline at end of file From 6de88bd5ce1a8876bb5245f4b69dac43ad6a0c78 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Wed, 13 Nov 2024 15:38:15 +0530 Subject: [PATCH 08/12] lint fixes Signed-off-by: mayaggar --- internal/resourcepolicies/volume_resources.go | 10 +++------- .../resourcepolicies/volume_resources_validator.go | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/resourcepolicies/volume_resources.go b/internal/resourcepolicies/volume_resources.go index c1cd1e581a..6abdb0648e 100644 --- a/internal/resourcepolicies/volume_resources.go +++ b/internal/resourcepolicies/volume_resources.go @@ -153,11 +153,7 @@ func (c *csiCondition) match(v *structuredVolume) bool { } if c.csi.Driver == "" { // match csi: {} - if v.csi != nil { - return true - } - - return false + return v.csi != nil } if v.csi == nil { @@ -168,11 +164,11 @@ func (c *csiCondition) match(v *structuredVolume) bool { return false } - if c.csi.VolumeAttributes == nil || len(c.csi.VolumeAttributes) == 0 { + if len(c.csi.VolumeAttributes) == 0 { return true } - if v.csi.VolumeAttributes == nil || len(v.csi.VolumeAttributes) == 0 { + if len(v.csi.VolumeAttributes) == 0 { return false } diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index 6064b89f4b..140be812c5 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -73,7 +73,7 @@ func (c *csiCondition) validate() error { if c != nil && c.csi != nil && c.csi.Driver == "" && c.csi.VolumeAttributes != nil { return errors.New("csi driver should not be empty") } - + return nil } From 561ec6b4ca05a1435f64ddef4de90263b2160f9c Mon Sep 17 00:00:00 2001 From: mayaggar Date: Wed, 20 Nov 2024 14:14:23 +0530 Subject: [PATCH 09/12] doc update Signed-off-by: mayaggar --- ...orting-volumeattributes-resource-policy.md | 18 +++++++++++++++--- .../resourcepolicies/resource_policies_test.go | 4 ++-- .../volume_resources_validator_test.go | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/design/Implemented/supporting-volumeattributes-resource-policy.md b/design/Implemented/supporting-volumeattributes-resource-policy.md index 76f382be9f..fafde200cb 100644 --- a/design/Implemented/supporting-volumeattributes-resource-policy.md +++ b/design/Implemented/supporting-volumeattributes-resource-policy.md @@ -11,19 +11,31 @@ As of Today, Velero resource policy already provides us the way to filter volume ## Example: - Provision Azure NFS: Define the Storage class with `protocol: nfs` under storage class parameters to provision [CSI NFS Azure File Shares](https://learn.microsoft.com/en-us/azure/aks/azure-files-csi#nfs-file-shares). - User wants to back up AFS (Azure file shares) but only want to backup `SMB` type of file share volumes and not `NFS` file share volumes. - + ## Goals - Introducing support for `VolumeAttributes` filter along with `driver` filter in CSI volume conditions to handle volumes. ## Non-Goals -- We are only bringing additional support to only handle volumes for backup and do not support restore. +- We are only bringing additional support in the resource policy to only handle volumes during backup. - Currently, only handles volumes, and does not support other resources. ## Use-cases/Scenarios ### Skip backup volumes by some volume attributes: Users want to skip PV with the requirements: - option to skip specified PV on volume attributes type (like Protocol as NFS, SMB, etc) - + +### Sample Storage Class Used to create such Volumes +``` +apiVersion: storage.k8s.io/v1 +kind: StorageClass +metadata: + name: azurefile-csi-nfs +provisioner: file.csi.azure.com +allowVolumeExpansion: true +parameters: + protocol: nfs +``` + ## High-Level Design Modifying the existing Resource Policies code for [csiVolumeSource](https://github.com/vmware-tanzu/velero/blob/8e23752a6ea83f101bd94a69dcf17f519a805388/internal/resourcepolicies/volume_resources_validator.go#L28C6-L28C22) to add the new `VolumeAttributes` filter for CSI volumes and adding validations in existing [csiCondition](https://github.com/vmware-tanzu/velero/blob/8e23752a6ea83f101bd94a69dcf17f519a805388/internal/resourcepolicies/volume_resources.go#L150) to match with volume attributes in the conditions from Resource Policy config map and original persistent volume. diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index de57126355..f84fd94b91 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -93,7 +93,7 @@ func TestLoadResourcePolicies(t *testing.T) { wantErr: true, }, { - name: "supported formart volume policies", + name: "supported format volume policies", yamlData: `version: v1 volumePolicies: - conditions: @@ -106,7 +106,7 @@ volumePolicies: wantErr: false, }, { - name: "supported formart csi driver with volumeAttributes for volume policies", + name: "supported format csi driver with volumeAttributes for volume policies", yamlData: `version: v1 volumePolicies: - conditions: diff --git a/internal/resourcepolicies/volume_resources_validator_test.go b/internal/resourcepolicies/volume_resources_validator_test.go index 5bd9e1b9c5..a74bbc52fa 100644 --- a/internal/resourcepolicies/volume_resources_validator_test.go +++ b/internal/resourcepolicies/volume_resources_validator_test.go @@ -280,7 +280,7 @@ func TestValidate(t *testing.T) { wantErr: false, }, { - name: "supported format volume policies only csi volumeattributes", + name: "unsupported format volume policies only csi volumeattributes", res: &ResourcePolicies{ Version: "v1", VolumePolicies: []VolumePolicy{ From 0de43a362397ee6505e8f5edd0cd125dc3ac0ef6 Mon Sep 17 00:00:00 2001 From: mayaggar Date: Wed, 20 Nov 2024 14:22:39 +0530 Subject: [PATCH 10/12] doc update Signed-off-by: mayaggar --- .../Implemented/supporting-volumeattributes-resource-policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design/Implemented/supporting-volumeattributes-resource-policy.md b/design/Implemented/supporting-volumeattributes-resource-policy.md index fafde200cb..a4f4d1bfc4 100644 --- a/design/Implemented/supporting-volumeattributes-resource-policy.md +++ b/design/Implemented/supporting-volumeattributes-resource-policy.md @@ -13,10 +13,10 @@ As of Today, Velero resource policy already provides us the way to filter volume - User wants to back up AFS (Azure file shares) but only want to backup `SMB` type of file share volumes and not `NFS` file share volumes. ## Goals +- We are only bringing additional support in the resource policy to only handle volumes during backup. - Introducing support for `VolumeAttributes` filter along with `driver` filter in CSI volume conditions to handle volumes. ## Non-Goals -- We are only bringing additional support in the resource policy to only handle volumes during backup. - Currently, only handles volumes, and does not support other resources. ## Use-cases/Scenarios From 4b1e0b85a5f82e93f8a16ca5683d6604d02edfed Mon Sep 17 00:00:00 2001 From: Mayank Aggarwal Date: Thu, 21 Nov 2024 19:34:04 +0530 Subject: [PATCH 11/12] Update internal/resourcepolicies/volume_resources_validator.go Co-authored-by: Tiger Kaovilai Signed-off-by: Mayank Aggarwal --- internal/resourcepolicies/volume_resources_validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index 140be812c5..f2ca97a300 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -71,7 +71,7 @@ func (c *nfsCondition) validate() error { func (c *csiCondition) validate() error { if c != nil && c.csi != nil && c.csi.Driver == "" && c.csi.VolumeAttributes != nil { - return errors.New("csi driver should not be empty") + return errors.New("csi driver should not be empty when filtering by volume attributes") } return nil From 5cf79aa9e8a3167c226dfa8dcaf8f6ecc3bda4dc Mon Sep 17 00:00:00 2001 From: mayaggar Date: Wed, 27 Nov 2024 11:18:53 +0530 Subject: [PATCH 12/12] doc name update Signed-off-by: mayaggar --- ...-policy.md => supporting-volumeattributes-resource-policy.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename design/Implemented/{supporting-volumeattributes-resource-policy.md => supporting-volumeattributes-resource-policy.md} (100%) diff --git a/design/Implemented/supporting-volumeattributes-resource-policy.md b/design/Implemented/supporting-volumeattributes-resource-policy.md similarity index 100% rename from design/Implemented/supporting-volumeattributes-resource-policy.md rename to design/Implemented/supporting-volumeattributes-resource-policy.md