Skip to content

Commit 02200fe

Browse files
andyzhangxandyzhangx
authored
Merge pull request #2388 from umagnus/tag_delimiter
feat: add tagValueDelimiter parameter
2 parents 7fcda0d + d58e00c commit 02200fe

File tree

8 files changed

+70
-29
lines changed

8 files changed

+70
-29
lines changed

‎pkg/azureconstants/azure_constants.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ const (
9999
SnapshotOpThrottlingSleepSec = 50
100100
MaxThrottlingSleepSec = 1200
101101
AgentNotReadyNodeTaintKeySuffix = "/agent-not-ready"
102+
// define tag value delimiter and default is comma
103+
TagValueDelimiterField = "tagValueDelimiter"
102104
)
103105

104106
var (

‎pkg/azuredisk/controllerserver.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -918,7 +918,7 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
918918
var customTags string
919919
// set incremental snapshot as true by default
920920
incremental := true
921-
var subsID, resourceGroup, dataAccessAuthMode string
921+
var subsID, resourceGroup, dataAccessAuthMode, tagValueDelimiter string
922922
var err error
923923
localCloud := d.cloud
924924
location := d.cloud.Location
@@ -947,6 +947,8 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
947947
subsID = v
948948
case consts.DataAccessAuthModeField:
949949
dataAccessAuthMode = v
950+
case consts.TagValueDelimiterField:
951+
tagValueDelimiter = v
950952
default:
951953
return nil, status.Errorf(codes.Internal, "AzureDisk - invalid option %s in VolumeSnapshotClass", k)
952954
}
@@ -964,7 +966,7 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
964966
}
965967
}
966968

967-
customTagsMap, err := volumehelper.ConvertTagsToMap(customTags)
969+
customTagsMap, err := volumehelper.ConvertTagsToMap(customTags, tagValueDelimiter)
968970
if err != nil {
969971
return nil, status.Errorf(codes.InvalidArgument, err.Error())
970972
}

‎pkg/azuredisk/controllerserver_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ func TestCreateVolume(t *testing.T) {
304304
d.getClientFactory().(*mock_azclient.MockClientFactory).EXPECT().GetDiskClientForSub(gomock.Any()).Return(diskClient, nil).AnyTimes()
305305
diskClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(disk, nil).AnyTimes()
306306
_, err := d.CreateVolume(context.Background(), req)
307-
expectedErr := status.Error(codes.InvalidArgument, "Failed parsing disk parameters: Tags 'unit-test' are invalid, the format should like: 'key1=value1,key2=value2'")
307+
expectedErr := status.Error(codes.InvalidArgument, "Failed parsing disk parameters: tags 'unit-test' are invalid, the format should like: 'key1=value1,key2=value2'")
308308
if !reflect.DeepEqual(err, expectedErr) {
309309
t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr)
310310
}
@@ -1255,7 +1255,7 @@ func TestCreateSnapshot(t *testing.T) {
12551255
}
12561256

12571257
_, err := d.CreateSnapshot(context.Background(), req)
1258-
expectedErr := status.Errorf(codes.InvalidArgument, "Tags 'unit-test' are invalid, the format should like: 'key1=value1,key2=value2'")
1258+
expectedErr := status.Errorf(codes.InvalidArgument, "tags 'unit-test' are invalid, the format should like: 'key1=value1,key2=value2'")
12591259
if !reflect.DeepEqual(err, expectedErr) {
12601260
t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr)
12611261
}

‎pkg/azuredisk/controllerserver_v2.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -772,7 +772,7 @@ func (d *DriverV2) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRe
772772
var customTags string
773773
// set incremental snapshot as true by default
774774
incremental := true
775-
var resourceGroup, subsID, dataAccessAuthMode string
775+
var resourceGroup, subsID, dataAccessAuthMode, tagValueDelimiter string
776776
var err error
777777

778778
parameters := req.GetParameters()
@@ -790,6 +790,8 @@ func (d *DriverV2) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRe
790790
subsID = v
791791
case consts.DataAccessAuthModeField:
792792
dataAccessAuthMode = v
793+
case consts.TagValueDelimiterField:
794+
tagValueDelimiter = v
793795
default:
794796
return nil, status.Errorf(codes.Internal, "AzureDisk - invalid option %s in VolumeSnapshotClass", k)
795797
}
@@ -807,7 +809,7 @@ func (d *DriverV2) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRe
807809
}
808810
}
809811

810-
customTagsMap, err := volumehelper.ConvertTagsToMap(customTags)
812+
customTagsMap, err := volumehelper.ConvertTagsToMap(customTags, tagValueDelimiter)
811813
if err != nil {
812814
return nil, status.Errorf(codes.InvalidArgument, err.Error())
813815
}

‎pkg/azureutils/azure_disk_utils.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,7 @@ func ParseDiskParameters(parameters map[string]string) (ManagedDiskParameters, e
581581
Tags: make(map[string]string),
582582
VolumeContext: parameters,
583583
}
584+
var originTags, tagValueDelimiter string
584585
for k, v := range parameters {
585586
switch strings.ToLower(k) {
586587
case consts.SkuNameField:
@@ -611,13 +612,7 @@ func ParseDiskParameters(parameters map[string]string) (ManagedDiskParameters, e
611612
case consts.DiskEncryptionTypeField:
612613
diskParams.DiskEncryptionType = v
613614
case consts.TagsField:
614-
customTagsMap, err := util.ConvertTagsToMap(v)
615-
if err != nil {
616-
return diskParams, err
617-
}
618-
for k, v := range customTagsMap {
619-
diskParams.Tags[k] = v
620-
}
615+
originTags = v
621616
case azure.WriteAcceleratorEnabled:
622617
diskParams.WriteAcceleratorEnabled = v
623618
case consts.MaxSharesField:
@@ -670,6 +665,8 @@ func ParseDiskParameters(parameters map[string]string) (ManagedDiskParameters, e
670665
if _, err = strconv.Atoi(v); err != nil {
671666
return diskParams, fmt.Errorf("parse %s failed with error: %v", v, err)
672667
}
668+
case consts.TagValueDelimiterField:
669+
tagValueDelimiter = v
673670
default:
674671
// accept all device settings params
675672
// device settings need to start with azureconstants.DeviceSettingsKeyPrefix
@@ -680,6 +677,13 @@ func ParseDiskParameters(parameters map[string]string) (ManagedDiskParameters, e
680677
}
681678
}
682679
}
680+
customTagsMap, err := util.ConvertTagsToMap(originTags, tagValueDelimiter)
681+
if err != nil {
682+
return diskParams, err
683+
}
684+
for k, v := range customTagsMap {
685+
diskParams.Tags[k] = v
686+
}
683687

684688
if strings.EqualFold(diskParams.AccountType, string(armcompute.DiskStorageAccountTypesPremiumV2LRS)) {
685689
if diskParams.CachingMode != "" && !strings.EqualFold(string(diskParams.CachingMode), string(v1.AzureDataDiskCachingNone)) {

‎pkg/util/util.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929

3030
const (
3131
GiB = 1024 * 1024 * 1024
32-
TagsDelimiter = ","
3332
TagKeyValueDelimiter = "="
3433
)
3534

@@ -78,27 +77,30 @@ func RoundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
7877
return roundedUp
7978
}
8079

81-
// ConvertTagsToMap convert the tags from string to map
80+
// ConvertTagsToMap convert the tags from string to map, default tagDelimiter is ","
8281
// the valid tags format is "key1=value1,key2=value2", which could be converted to
8382
// {"key1": "value1", "key2": "value2"}
84-
func ConvertTagsToMap(tags string) (map[string]string, error) {
83+
func ConvertTagsToMap(tags string, tagsDelimiter string) (map[string]string, error) {
8584
m := make(map[string]string)
8685
if tags == "" {
8786
return m, nil
8887
}
89-
s := strings.Split(tags, TagsDelimiter)
88+
if tagsDelimiter == "" {
89+
tagsDelimiter = ","
90+
}
91+
s := strings.Split(tags, tagsDelimiter)
9092
for _, tag := range s {
91-
kv := strings.Split(tag, TagKeyValueDelimiter)
93+
kv := strings.SplitN(tag, TagKeyValueDelimiter, 2)
9294
if len(kv) != 2 {
93-
return nil, fmt.Errorf("Tags '%s' are invalid, the format should like: 'key1=value1,key2=value2'", tags)
95+
return nil, fmt.Errorf("tags '%s' are invalid, the format should like: 'key1=value1%skey2=value2'", tags, tagsDelimiter)
9496
}
9597
key := strings.TrimSpace(kv[0])
9698
if key == "" {
97-
return nil, fmt.Errorf("Tags '%s' are invalid, the format should like: 'key1=value1,key2=value2'", tags)
99+
return nil, fmt.Errorf("tags '%s' are invalid, the format should like: 'key1=value1%skey2=value2'", tags, tagsDelimiter)
98100
}
99101
// <>%&?/. are not allowed in tag key
100102
if strings.ContainsAny(key, "<>%&?/.") {
101-
return nil, fmt.Errorf("Tag key '%s' contains invalid characters", key)
103+
return nil, fmt.Errorf("tag key '%s' contains invalid characters", key)
102104
}
103105
value := strings.TrimSpace(kv[1])
104106
m[key] = value

‎pkg/util/util_test.go‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,35 +62,40 @@ func TestConvertTagsToMap(t *testing.T) {
6262
testCases := []struct {
6363
desc string
6464
tags string
65+
tagsDelimiter string
6566
expectedOutput map[string]string
6667
expectedError bool
6768
}{
6869
{
6970
desc: "should return empty map when tag is empty",
7071
tags: "",
72+
tagsDelimiter: ",",
7173
expectedOutput: map[string]string{},
7274
expectedError: false,
7375
},
7476
{
75-
desc: "sing valid tag should be converted",
76-
tags: "key=value",
77+
desc: "sing valid tag should be converted",
78+
tags: "key=value",
79+
tagsDelimiter: ",",
7780
expectedOutput: map[string]string{
7881
"key": "value",
7982
},
8083
expectedError: false,
8184
},
8285
{
83-
desc: "multiple valid tags should be converted",
84-
tags: "key1=value1,key2=value2",
86+
desc: "multiple valid tags should be converted",
87+
tags: "key1=value1,key2=value2",
88+
tagsDelimiter: ",",
8589
expectedOutput: map[string]string{
8690
"key1": "value1",
8791
"key2": "value2",
8892
},
8993
expectedError: false,
9094
},
9195
{
92-
desc: "whitespaces should be trimmed",
93-
tags: "key1=value1, key2=value2",
96+
desc: "whitespaces should be trimmed",
97+
tags: "key1=value1, key2=value2",
98+
tagsDelimiter: ",",
9499
expectedOutput: map[string]string{
95100
"key1": "value1",
96101
"key2": "value2",
@@ -100,31 +105,55 @@ func TestConvertTagsToMap(t *testing.T) {
100105
{
101106
desc: "should return error for invalid format",
102107
tags: "foo,bar",
108+
tagsDelimiter: ",",
103109
expectedOutput: nil,
104110
expectedError: true,
105111
},
106112
{
107113
desc: "should return error for when key is missed",
108114
tags: "key1=value1,=bar",
115+
tagsDelimiter: ",",
109116
expectedOutput: nil,
110117
expectedError: true,
111118
},
112119
{
113120
desc: "should return error for invalid characters in key",
114121
tags: "key/1=value1,<bar",
122+
tagsDelimiter: ",",
115123
expectedOutput: nil,
116124
expectedError: true,
117125
},
118126
{
119127
desc: "should return success for special characters in value",
120128
tags: "key1=value1,key2=<>%&?/.",
129+
tagsDelimiter: ",",
121130
expectedOutput: map[string]string{"key1": "value1", "key2": "<>%&?/."},
122131
expectedError: false,
123132
},
133+
{
134+
desc: "should return success for empty tagsDelimiter",
135+
tags: "key1=value1,key2=value2",
136+
tagsDelimiter: "",
137+
expectedOutput: map[string]string{
138+
"key1": "value1",
139+
"key2": "value2",
140+
},
141+
expectedError: false,
142+
},
143+
{
144+
desc: "should return success for special tagsDelimiter and tag values containing commas and equal sign",
145+
tags: "key1=aGVsbG8=;key2=value-2, value-3",
146+
tagsDelimiter: ";",
147+
expectedOutput: map[string]string{
148+
"key1": "aGVsbG8=",
149+
"key2": "value-2, value-3",
150+
},
151+
expectedError: false,
152+
},
124153
}
125154

126155
for i, c := range testCases {
127-
m, err := ConvertTagsToMap(c.tags)
156+
m, err := ConvertTagsToMap(c.tags, c.tagsDelimiter)
128157
if c.expectedError {
129158
assert.NotNil(t, err, "TestCase[%d]: %s", i, c.desc)
130159
} else {

‎test/e2e/testsuites/dynamically_provisioned_azuredisk_tag_tester.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func (t *DynamicallyProvisionedAzureDiskWithTag) Run(ctx context.Context, client
8484
framework.ExpectNoError(err, fmt.Sprintf("Error getting client for azuredisk %v", err))
8585
disktest, err := disksClient.Get(ctx, resourceGroup, diskName)
8686
framework.ExpectNoError(err, fmt.Sprintf("Error getting disk for azuredisk %v", err))
87-
test, err := util.ConvertTagsToMap(t.Tags)
87+
test, err := util.ConvertTagsToMap(t.Tags, ",")
8888
framework.ExpectNoError(err, fmt.Sprintf("Error getting tag %v", err))
8989
test["k8s-azure-created-by"] = "kubernetes-azure-dd"
9090

0 commit comments

Comments
 (0)