From e4da85a8282e92a3d2d24a71838e5be396f70265 Mon Sep 17 00:00:00 2001 From: Rishabh Singhvi Date: Wed, 30 Mar 2022 14:27:20 +0000 Subject: [PATCH] Adding tagging support through StorageClass.parameters This commit adds tagging support through StorageClass.parameters. For more information about the design, refer to https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/1190 Signed-off-by: Rishabh Singhvi --- cmd/main.go | 1 + cmd/options/controller_options.go | 3 + docs/README.md | 3 + docs/TAGGING.md | 103 ++++++++++++++ go.mod | 2 +- pkg/cloud/cloud.go | 2 + pkg/driver/constants.go | 4 + pkg/driver/controller.go | 26 +++- pkg/driver/driver.go | 7 + pkg/driver/validation.go | 32 ++++- pkg/driver/validation_test.go | 2 +- pkg/util/template/funcs.go | 72 ++++++++++ pkg/util/template/template.go | 55 ++++++++ pkg/util/template/template_test.go | 214 +++++++++++++++++++++++++++++ 14 files changed, 520 insertions(+), 6 deletions(-) create mode 100644 docs/TAGGING.md create mode 100644 pkg/util/template/funcs.go create mode 100644 pkg/util/template/template.go create mode 100644 pkg/util/template/template_test.go diff --git a/cmd/main.go b/cmd/main.go index 493f0a8556..a58a7d0c07 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -51,6 +51,7 @@ func main() { driver.WithVolumeAttachLimit(options.NodeOptions.VolumeAttachLimit), driver.WithKubernetesClusterID(options.ControllerOptions.KubernetesClusterID), driver.WithAwsSdkDebugLog(options.ControllerOptions.AwsSdkDebugLog), + driver.WithWarnOnInvalidTag(options.ControllerOptions.WarnOnInvalidTag), ) if err != nil { klog.Fatalln(err) diff --git a/cmd/options/controller_options.go b/cmd/options/controller_options.go index 91f413ef91..9f6fb93724 100644 --- a/cmd/options/controller_options.go +++ b/cmd/options/controller_options.go @@ -35,6 +35,8 @@ type ControllerOptions struct { KubernetesClusterID string // flag to enable sdk debug log AwsSdkDebugLog bool + // flag to warn on invalid tag, instead of returning an error + WarnOnInvalidTag bool } func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) { @@ -42,4 +44,5 @@ func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) { fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '=,='") fs.StringVar(&s.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).") fs.BoolVar(&s.AwsSdkDebugLog, "aws-sdk-debug-log", false, "To enable the aws sdk debug log level (default to false).") + fs.BoolVar(&s.WarnOnInvalidTag, "warn-on-invalid-tag", false, "To warn on invalid tags, instead of returning an error") } diff --git a/docs/README.md b/docs/README.md index 661cc137f6..fe053d0ff2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -51,6 +51,9 @@ To help manage volumes in the aws account, CSI driver will automatically add tag | kubernetes.io/cluster/X| owned | kubernetes.io/cluster/aws-cluster-id-1 = owned | add to all volumes and snapshots if k8s-tag-cluster-id argument is set to X.| | extra-key | extra-value | extra-key = extra-value | add to all volumes and snapshots if extraTags argument is set| + +The CSI driver also supports passing tags through `StorageClass.parameters`. For more information, please refer to the [tagging doc](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/docs/TAGGING.md). + ## Driver Options There are couple driver options that can be passed as arguments when starting driver container. diff --git a/docs/TAGGING.md b/docs/TAGGING.md new file mode 100644 index 0000000000..308cfb36ab --- /dev/null +++ b/docs/TAGGING.md @@ -0,0 +1,103 @@ +# Tagging + +The AWS EBS CSI Driver supports tagging through `StorageClass.parameters`. + +If a key has the prefix `tagSpecification`, the CSI driver will treat the value as a key-value pair to be applied to the dynamically provisioned volume as tags. + + +**Example 1** +``` +kind: StorageClass +apiVersion: storage.k8s.io/v1 +metadata: + name: ebs-sc +provisioner: ebs.csi.aws.com +parameters: + tagSpecification_1: "key1=value1" + tagSpecification_2: "key2=hello world" + tagSpecification_3: "key3=" +``` + +Provisioning a volume using this StorageClass will apply two tags: + +``` +key1=value1 +key2=hello world +key3= +``` + +________ + +To allow for PV-level granularity, the CSI driver support runtime string interpolation on the tag values. You can specify placeholders for PVC namespace, PVC name and PV name, which will then be dynamically computed at runtime. + +**NOTE: This requires the `--extra-create-metadata` flag to be enabled on the `external-provisioner` sidecar.** + +**Example 2** +``` +kind: StorageClass +apiVersion: storage.k8s.io/v1 +metadata: + name: ebs-sc +provisioner: ebs.csi.aws.com +parameters: + tagSpecification_1: "pvcnamespace={{ .PVCNamespace }}" + tagSpecification_2: "pvcname={{ .PVCName }}" + tagSpecification_3: "pvname={{ .PVName }}" +``` +Provisioning a volume using this StorageClass, with a PVC named 'ebs-claim' in namespace 'default', will apply the following tags: + +``` +pvcnamespace=default +pvcname=ebs-claim +pvname= +``` + + +_________ + +The driver uses Go's `text/template` package for string interpolation. As such, cluster admins are free to use the constructs provided by the package (except for certain function, see `Failure Modes` below). To aid cluster admins to be more expressive, certain functions have been provided. + +They include: + +- **field** delim index str: Split `str` by `delim` and extract the word at position `index`. +- **substring** start end str: Get a substring of `str` given the `start` and `end` indices +- **toUpper** str: Convert `str` to uppercase +- **toLower** str: Convert `str` to lowercase +- **contains** str1 str2: Returns a boolean if `str2` contains `str1` + + +**Example 3** +``` +kind: StorageClass +apiVersion: storage.k8s.io/v1 +metadata: + name: ebs-sc +provisioner: ebs.csi.aws.com +parameters: + tagSpecification_1: 'backup={{ .PVCNamespace | contains "prod" }}' + tagSpecification_2: 'billingID={{ .PVCNamespace | field "-" 2 | toUpper }}' +``` + +Assuming the PVC namespace is `ns-prod-abcdef`, the attached tags will be + +``` +backup=true +billingID=ABCDEF +``` + +____ + +## Failure Modes + +There can be multipe failure modes: + +* The template cannot be parsed. +* The key/interpolated value do not meet the [AWS Tag Requirements](https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html) +* The key is not allowed (such as keys used internally by the CSI driver e.g., 'CSIVolumeName'). +* The template uses one of the disabled function calls. The driver disables the following `text/template` functions: `js`, `call`, `html`, `urlquery`. + +In this case, the CSI driver will not provision a volume, but instead return an error. + +The driver also defines another flag, `--warn-on-invalid-tag` that will (if set), instead of returning an error, log a warning and skip the offending tag. + + diff --git a/go.mod b/go.mod index 4c37faf80b..e2a3b97819 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ require ( github.com/container-storage-interface/spec v1.3.0 github.com/golang/mock v1.5.0 github.com/golang/protobuf v1.4.3 + github.com/google/go-cmp v0.5.5 github.com/kubernetes-csi/csi-proxy/client v1.0.1 github.com/kubernetes-csi/csi-test v2.0.0+incompatible github.com/kubernetes-csi/external-snapshotter/client/v4 v4.0.0 @@ -36,7 +37,6 @@ require ( github.com/go-logr/logr v0.4.0 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect - github.com/google/go-cmp v0.5.5 // indirect github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.1.2 // indirect github.com/googleapis/gnostic v0.4.1 // indirect diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 3accc1d411..2f12b1285b 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -98,6 +98,8 @@ const ( const ( // MaxNumTagsPerResource represents the maximum number of tags per AWS resource. MaxNumTagsPerResource = 50 + // MinTagKeyLength represents the minimum key length for a tag. + MinTagKeyLength = 1 // MaxTagKeyLength represents the maximum key length for a tag. MaxTagKeyLength = 128 // MaxTagValueLength represents the maximum value length for a tag. diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go index 43028b57eb..3b044f62ec 100644 --- a/pkg/driver/constants.go +++ b/pkg/driver/constants.go @@ -68,6 +68,10 @@ const ( // PVNameKey contains name of the final PV that will be used for the dynamically // provisioned volume PVNameKey = "csi.storage.k8s.io/pv/name" + + // TagKeyPrefix contains the prefix of a volume parameter that designates it as + // a tag to be attached to the resource + TagKeyPrefix = "tagSpecification" ) // constants for volume tags and their values diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index fa718c8570..c9560f2efe 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -29,6 +29,7 @@ import ( "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud" "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/internal" "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/util" + "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/util/template" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "k8s.io/klog" @@ -123,12 +124,15 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol throughput int isEncrypted bool kmsKeyID string + scTags []string volumeTags = map[string]string{ cloud.VolumeNameTagKey: volName, cloud.AwsEbsDriverTagKey: isManagedByDriver, } ) + tProps := new(template.Props) + for key, value := range req.GetParameters() { switch strings.ToLower(key) { case "fstype": @@ -162,12 +166,19 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol kmsKeyID = value case PVCNameKey: volumeTags[PVCNameTag] = value + tProps.PVCName = value case PVCNamespaceKey: volumeTags[PVCNamespaceTag] = value + tProps.PVCNamespace = value case PVNameKey: volumeTags[PVNameTag] = value + tProps.PVName = value default: - return nil, status.Errorf(codes.InvalidArgument, "Invalid parameter key %s for CreateVolume", key) + if strings.HasPrefix(key, TagKeyPrefix) { + scTags = append(scTags, value) + } else { + return nil, status.Errorf(codes.InvalidArgument, "Invalid parameter key %s for CreateVolume", key) + } } } @@ -205,6 +216,19 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol volumeTags[k] = v } + addTags, err := template.Evaluate(scTags, tProps, d.driverOptions.warnOnInvalidTag) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Error interpolating the tag value: %v", err) + } + + if err := validateExtraTags(addTags, d.driverOptions.warnOnInvalidTag); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid tag value: %v", err) + } + + for k, v := range addTags { + volumeTags[k] = v + } + opts := &cloud.DiskOptions{ CapacityBytes: volSizeBytes, Tags: volumeTags, diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index b4bc7fa134..61949bd030 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -66,6 +66,7 @@ type DriverOptions struct { volumeAttachLimit int64 kubernetesClusterID string awsSdkDebugLog bool + warnOnInvalidTag bool } func NewDriver(options ...func(*DriverOptions)) (*Driver, error) { @@ -191,3 +192,9 @@ func WithAwsSdkDebugLog(enableSdkDebugLog bool) func(*DriverOptions) { o.awsSdkDebugLog = enableSdkDebugLog } } + +func WithWarnOnInvalidTag(warnOnInvalidTag bool) func(*DriverOptions) { + return func(o *DriverOptions) { + o.warnOnInvalidTag = warnOnInvalidTag + } +} diff --git a/pkg/driver/validation.go b/pkg/driver/validation.go index 94920554c2..0fe0f54f8f 100644 --- a/pkg/driver/validation.go +++ b/pkg/driver/validation.go @@ -18,13 +18,15 @@ package driver import ( "fmt" + "regexp" "strings" "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud" + "k8s.io/klog" ) func ValidateDriverOptions(options *DriverOptions) error { - if err := validateExtraTags(options.extraTags); err != nil { + if err := validateExtraTags(options.extraTags, false); err != nil { return fmt.Errorf("Invalid extra tags: %v", err) } @@ -35,14 +37,21 @@ func ValidateDriverOptions(options *DriverOptions) error { return nil } -func validateExtraTags(tags map[string]string) error { +var ( + /// https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html + awsTagValidRegex = regexp.MustCompile(`[a-zA-Z0-9_.:=+\-@]*`) +) + +func validateExtraTags(tags map[string]string, warnOnly bool) error { if len(tags) > cloud.MaxNumTagsPerResource { return fmt.Errorf("Too many tags (actual: %d, limit: %d)", len(tags), cloud.MaxNumTagsPerResource) } - for k, v := range tags { + validate := func(k, v string) error { if len(k) > cloud.MaxTagKeyLength { return fmt.Errorf("Tag key too long (actual: %d, limit: %d)", len(k), cloud.MaxTagKeyLength) + } else if len(k) < cloud.MinTagKeyLength { + return fmt.Errorf("Tag key cannot be empty (min: 1)") } if len(v) > cloud.MaxTagValueLength { return fmt.Errorf("Tag value too long (actual: %d, limit: %d)", len(v), cloud.MaxTagValueLength) @@ -62,8 +71,25 @@ func validateExtraTags(tags map[string]string) error { if strings.HasPrefix(k, cloud.AWSTagKeyPrefix) { return fmt.Errorf("Tag key prefix '%s' is reserved", cloud.AWSTagKeyPrefix) } + if !awsTagValidRegex.MatchString(k) { + return fmt.Errorf("Tag key '%s' is not a valid AWS tag key", k) + } + if !awsTagValidRegex.MatchString(v) { + return fmt.Errorf("Tag value '%s' is not a valid AWS tag value", v) + } + return nil } + for k, v := range tags { + err := validate(k, v) + if err != nil { + if warnOnly { + klog.Warningf("Skipping tag: the following key-value pair is not valid: (%s, %s): (%v)", k, v, err) + } else { + return err + } + } + } return nil } diff --git a/pkg/driver/validation_test.go b/pkg/driver/validation_test.go index e89353c3c0..2cc8892369 100644 --- a/pkg/driver/validation_test.go +++ b/pkg/driver/validation_test.go @@ -108,7 +108,7 @@ func TestValidateExtraVolumeTags(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err := validateExtraTags(tc.tags) + err := validateExtraTags(tc.tags, false) if !reflect.DeepEqual(err, tc.expErr) { t.Fatalf("error not equal\ngot:\n%s\nexpected:\n%s", err, tc.expErr) } diff --git a/pkg/util/template/funcs.go b/pkg/util/template/funcs.go new file mode 100644 index 0000000000..4d444e40d5 --- /dev/null +++ b/pkg/util/template/funcs.go @@ -0,0 +1,72 @@ +package template + +import ( + "fmt" + "html/template" + "strings" +) + +// Disable functions +func html(args ...interface{}) (string, error) { + return "", fmt.Errorf("cannot call 'html' function") +} + +func js(args ...interface{}) (string, error) { + return "", fmt.Errorf("cannot call 'js' function") +} + +func call(args ...interface{}) (string, error) { + return "", fmt.Errorf("cannot call 'call' function") +} + +func urlquery(args ...interface{}) (string, error) { + return "", fmt.Errorf("cannot call 'urlquery' function") +} + +func contains(arg1, arg2 string) bool { + return strings.Contains(arg2, arg1) +} + +func substring(start, end int, arg string) string { + if start < 0 { + return arg[:end] + } + + if end < 0 || end > len(arg) { + return arg[start:] + } + + return arg[start:end] +} + +func field(delim string, idx int, arg string) (string, error) { + w := strings.Split(arg, delim) + if idx >= len(w) { + return "", fmt.Errorf("extractWord: cannot index into split string; index = %d, length = %d", idx, len(w)) + } + return w[idx], nil +} + +func index(arg1, arg2 string) int { + return strings.Index(arg2, arg1) +} + +func lastIndex(arg1, arg2 string) int { + return strings.LastIndex(arg2, arg1) +} + +func newFuncMap() template.FuncMap { + return template.FuncMap{ + "html": html, + "js": js, + "call": call, + "urlquery": urlquery, + "contains": contains, + "toUpper": strings.ToUpper, + "toLower": strings.ToLower, + "substring": substring, + "field": field, + "index": index, + "lastIndex": lastIndex, + } +} diff --git a/pkg/util/template/template.go b/pkg/util/template/template.go new file mode 100644 index 0000000000..ebf1c37646 --- /dev/null +++ b/pkg/util/template/template.go @@ -0,0 +1,55 @@ +package template + +import ( + "fmt" + "strings" + "text/template" + + "k8s.io/klog/v2" +) + +type Props struct { + PVCName string + PVCNamespace string + PVName string +} + +func Evaluate(tm []string, props *Props, warnOnly bool) (map[string]string, error) { + md := make(map[string]string) + for _, s := range tm { + st := strings.SplitN(s, "=", 2) + if len(st) != 2 { + return nil, fmt.Errorf("the key-value pair doesn't contain a value (string: %s)", s) + } + + key, value := st[0], st[1] + + t := template.New("tmpl").Funcs(template.FuncMap(newFuncMap())) + val, err := execTemplate(value, props, t) + if err != nil { + if warnOnly { + klog.Warningf("Unable to interpolate the value for tag (%s, %s): %v", key, value, err) + } else { + return nil, err + } + } else { + md[key] = val + } + } + return md, nil +} + +func execTemplate(value string, props *Props, t *template.Template) (string, error) { + tmpl, err := t.Parse(value) + if err != nil { + return "", err + } + + b := new(strings.Builder) + err = tmpl.Execute(b, props) + if err != nil { + return "", err + } + + return b.String(), nil +} diff --git a/pkg/util/template/template_test.go b/pkg/util/template/template_test.go new file mode 100644 index 0000000000..a11fef675a --- /dev/null +++ b/pkg/util/template/template_test.go @@ -0,0 +1,214 @@ +package template + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestEvaluate(t *testing.T) { + testCases := []struct { + name string + input []string + pvcName string + pvName string + pvcNamespace string + warnOnly bool + expectErr bool + expectedTags map[string]string + }{ + { + name: "empty input", + expectedTags: make(map[string]string), + }, + { + name: "no interpolation", + input: []string{ + "key1=value1", + "key2=hello world", + }, + expectedTags: map[string]string{ + "key1": "value1", + "key2": "hello world", + }, + }, + { + name: "no tag values gives empty string", + input: []string{ + "key1=", + }, + expectedTags: map[string]string{ + "key1": "", + }, + }, + { + name: "no = returns an error", + input: []string{ + "key1", + }, + expectErr: true, + }, + { + name: "simple substitution", + input: []string{ + "key1={{ .PVCName }}", + "key2={{ .PVCNamespace }}", + "key3={{ .PVName }}", + }, + pvcName: "ebs-claim", + pvcNamespace: "default", + pvName: "ebs-claim-012345", + expectedTags: map[string]string{ + "key1": "ebs-claim", + "key2": "default", + "key3": "ebs-claim-012345", + }, + }, + { + name: "template parsing error", + input: []string{ + "key1={{ .PVCName }", + }, + expectErr: true, + }, + { + name: "template parsing error warn only", + input: []string{ + "key1={{ .PVCName }", + "key2={{ .PVCNamespace }}", + }, + pvcName: "ebs-claim", + pvcNamespace: "default", + warnOnly: true, + expectedTags: map[string]string{ + "key2": "default", + }, + }, + { + name: "test function - html - returns error", + input: []string{ + `backup={{ .PVCNamespace | html }}`, + }, + pvcNamespace: "ns-prod", + expectErr: true, + }, + { + name: "test func - js - returns error", + input: []string{ + `backup={{ .PVCNamespace | js }}`, + }, + pvcNamespace: "ns-prod", + expectErr: true, + }, + { + name: "test func - call - returns error", + input: []string{ + `backup={{ .PVCNamespace | call }}`, + }, + pvcNamespace: "ns-prod", + expectErr: true, + }, + { + name: "test func - urlquery - returns error", + input: []string{ + `backup={{ .PVCNamespace | urlquery }}`, + }, + pvcNamespace: "ns-prod", + expectErr: true, + }, + { + name: "test function - contains", + input: []string{ + `backup={{ .PVCNamespace | contains "prod" }}`, + }, + pvcNamespace: "ns-prod", + expectedTags: map[string]string{ + "backup": "true", + }, + }, + { + name: "test function - toUpper", + input: []string{ + `backup={{ .PVCNamespace | toUpper }}`, + }, + pvcNamespace: "ns-prod", + expectedTags: map[string]string{ + "backup": "NS-PROD", + }, + }, + { + name: "test function - toLower", + input: []string{ + `backup={{ .PVCNamespace | toLower }}`, + }, + pvcNamespace: "ns-PROD", + expectedTags: map[string]string{ + "backup": "ns-prod", + }, + }, + { + name: "test function - field", + input: []string{ + `backup={{ .PVCNamespace | field "-" 1 }}`, + }, + pvcNamespace: "ns-prod-default", + expectedTags: map[string]string{ + "backup": "prod", + }, + }, + { + name: "test function - substring", + input: []string{ + `key1={{ .PVCNamespace | substring 0 4 }}`, + }, + pvcNamespace: "prod-12345", + expectedTags: map[string]string{ + "key1": "prod", + }, + }, + { + name: "no extra-create-metadata flag", + input: []string{ + `key1={{ .PVCNamespace }}`, + `key2={{ .PVCName }}`, + }, + expectedTags: map[string]string{ + "key1": "", + "key2": "", + }, + }, + { + name: "field returns error", + input: []string{ + `key1={{ .PVCNamespace | field "-" 1 }}`, + }, + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + props := &Props{ + PVCName: tc.pvcName, + PVCNamespace: tc.pvcNamespace, + PVName: tc.pvName, + } + + tags, err := Evaluate(tc.input, props, tc.warnOnly) + + if tc.expectErr { + if err == nil { + t.Fatalf("expected an error; got nil") + } + } else { + if err != nil { + t.Fatalf("err is not nil; err = %v", err) + } + if diff := cmp.Diff(tc.expectedTags, tags); diff != "" { + t.Fatalf("tags are different; diff = %v", diff) + } + } + }) + } +}