Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to provide arbitrary EBS volume tags #180

Closed
dkoshkin opened this issue Jan 18, 2019 · 60 comments
Closed

Add the ability to provide arbitrary EBS volume tags #180

dkoshkin opened this issue Jan 18, 2019 · 60 comments
Assignees

Comments

@dkoshkin
Copy link
Contributor

Is your feature request related to a problem? Please describe.
With dynamically provisioned volumes there is always a danger of having orphaned EBS volumes. With current implementation, the only way to tie an EBS volume to a resource in Kubernetes is via the CSIVolumeName tag that gets automatically applied or with volumeHandle in the PV spec. However, both of these will not work if the Kubernetes resource is deleted and the EBS volume persists (either due the ReclaimPolicy or some bug where the volume is not deleted).

Providing a mechanism for operators to tag created volumes with custom tags, would help them in managing those EBS volumes, with this feature a user would be able to tag the volume with team, app, billing code, or whatever other tag they might already be using with other EBS volumes.

Describe the solution you'd like in detail
A new field can be added in the StorageClass parameters tag:

parameters:
  type: gp2 #gp2, io1, sc1, st1 - io1 requires 'iopsPerGB' to be set
  fsType: ext4 # ext4, ext3, ext2
  encrypted: "false"  # false/true
  additionalTags: "team=kubernetes,app=myapp,important"

The driver would perform a single validation:

  • additionalTags does not contain CSIVolumeName, as that is reserved to be used by the driver.

Otherwise tags would be passed along to the AWS API request and if any of those validations fail, that error will be propagated back to the user via Kubernetes events.

Describe alternatives you've considered
Another option would be for the user to pass these tags in the PVC spec via an annotation. AFAIK this is not supported by the CSI spec.

Additional context
I might be wrong here but I believe the AWS cloud-provider supports this feature.

@leakingtapan
Copy link
Contributor

/feature

@leakingtapan
Copy link
Contributor

/assign @dkoshkin

@frittentheke
Copy link
Contributor

This feature also has some nice security implications.... I personally would like my provisioners IAM policy to require certain tags to be applied and then only allow the attacher to attach such volumes in their policy.

Otherwise I could i.e. hack into a worker node and reattach i.e. an etcd volume from another, totally unrelated EC2 (yeah, I know about encryption at rest for etcd 😉)

Just best practice on AWS: https://aws.amazon.com/de/premiumsupport/knowledge-center/iam-policy-tags-restrict/

@mgoodness
Copy link

mgoodness commented Feb 20, 2019

A StorageClass parameter would not be great for my use-case, which requires every EBS volume to be tagged with a product code for cost show-back purposes. We have hundreds of these codes. I'd rather not have to create the same number of StorageClasses (potentially multiplied by some combination of other parameters) for every cluster.

Our ideal solution involves PersistentVolumeClaim annotations as mentioned in kubernetes/kubernetes#50898 (comment).

@dkoshkin
Copy link
Contributor Author

There has been an ongoing discussion here.
At this point it is not possible to get the PVC annotations as that would leak the Kubernetes implementation details in the CSI spec.

@leakingtapan
Copy link
Contributor

leakingtapan commented Feb 28, 2019

The discussion mentioned by @dkoshkin is currently the biggest issue blocking us from support this feature in PVC. Here is another related discussion.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2019
@frittentheke
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 4, 2019
@jieyu
Copy link
Contributor

jieyu commented Aug 7, 2019

OK, looks like upstream does not like the either of the proposals. But this is a real production issue. Can we do some workaround like: introduce a config options (i.e., a cmdline flag) for the controller service so that it'll always attach some tags to all the dynamically provisioned EBS volumes. The default can be empty so that it matches the current behavior. Thoughts?

@leakingtapan
Copy link
Contributor

leakingtapan commented Aug 8, 2019

@jieyu Given my understanding of the use cases, we need a way to dynamically tag the volumes given some user input. Passing the tag through controller service cmd flag won’t help in those cases

@leakingtapan
Copy link
Contributor

I remember chatted with @saad-ali a while back. One potential solution to this problem could be user creates volume tags inside PV annotation, and we create a separated small controller that watches PV and it reads PV's annotation and tags the corresponding volume with the tags.

@mgoodness @frittentheke @jieyu @dkoshkin WDYT?

@dkoshkin
Copy link
Contributor Author

dkoshkin commented Aug 8, 2019

The PVs are not created by a user PVCs are, I think it would be strange as a user to have add annotations because how would you do it, a controller :P?

@jieyu
Copy link
Contributor

jieyu commented Aug 8, 2019

@leakingtapan this is our issue: we need to make sure all volumes dynamic provisioned are tagged with a cluster uuid so that we can safely delete the volume during cluster teardown. For that particular use case, we don't necessarily need to dynamically tag the volumes given some user input.

That's the reason I was suggesting the above solution, which sounds less intrusive and backwards compatible.

@leakingtapan
Copy link
Contributor

@jieyu I see your point. Just created a separate issue to track your request since I feel we might be able to solve it under that specific situation with your suggestion as one option.

@leakingtapan
Copy link
Contributor

leakingtapan commented Aug 8, 2019

The PVs are not created by a user PVCs are, I think it would be strange as a user to have add annotations because how would you do it, a controller :P?

If we were to provide arbitrary EBS volume tagging functionality, I think we need to consider both dynamic provisioning and static provisioning case. If we want user to create tag annotations as part of PVC, this solves for dynamic provisioning. But I'm not sure how it will work for static provisioning. Another way is, maybe we could only provide this tagging feature to dynamic provisioned volumes and we require user to tag themselves if it is static provisioning.

But I agree that asking user to add annotation in PV when dynamic provisioning is used is weird.

@leakingtapan
Copy link
Contributor

leakingtapan commented Aug 21, 2019

I created a high level design #351 using operator for this problem. PTAL @dkoshkin @mgoodness

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 19, 2019
@frittentheke
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 20, 2019
@flickerfly
Copy link

Is there two different problems here? I can't create an EBS volume without an OWNER tag due to billing requirements forcing me to configure other storage. That can be the same tag every time when associated with the cluster. Perhaps that's an easier problem to solve first?

Then figuring out how to provide a method for changing up tags depending on certain scenarios can come as a follow-up problem?

@jieyu
Copy link
Contributor

jieyu commented Feb 14, 2020

@flickerfly you might want to checkout this feature #333

@danielwegener
Copy link

Works great, Thanks!

@hywook4
Copy link

hywook4 commented Aug 23, 2021

When I can add these to the StorageClass, I'd need to have a different StorageClass in order to have a different set of tags for each namespace for example? Ideally, I'd like to set the tags at the PVC and have them passed down.

The only metadata about a PVC that the CSI driver has access to is its name and namespace, so it won't be possible to read a PVCs' annotations and pass them down to set tags accordingly (as was originally suggested in this issue), but it will be possible to use the namespace. So what we could do is A) add another boolean StorageClass parameter to control whether to add PVC name and namespace tags to provisioned volumes. Or B) have the hypothetical extraTags parameter respect a template string like {{namespace}} so that if I set extraTags to namespace={{namespace}},key=value the driver will tag volumes accordingly. Either way it would save you from having to create a StorageClass per-namespace. Without either A) or B), then yes you would have to create a StorageClass per namespace and then get creative with https://kubernetes.io/docs/concepts/policy/resource-quotas/#storage-resource-quota to enforce that e.g. PVCs in namespace 1 cannot use StorageClass 2.

I don't think it's generally desirable to let the PVC creator dictate tags anyway, because using your billing example, you wouldn't want the PVC creator to tag the volume such that somebody else gets billed.

Hi @wongma7
I'm trying to add dynamic extraVolumeTags using the metadata that CSI driver can access.
I'm using helm to deploy the Daemonset. Could you help me how to use the metadata to add custom volume tag?
For example.
I'd like to add Name tag so

extraVolumeTags: Name: {{namespace}}-{{name}}
And namespace and name would be the metadata that CSI driver can access. Could you give me guide for this?

Thank you.

@buddhdev-harsh
Copy link

buddhdev-harsh commented Sep 15, 2021

@hywook4
i think i might have fix for you

every time you make new volume you'll redeploy ebs-csi driver with extra-key tags suggested in README.md. this extra-key tag will be Name and value will be provided by config-map that will also be made directly.

i was using it with helm charts and i also had issue with naming EBS volumes in aws so i came with this solution.

Script that will deploy EBS-csi driver along with config-map and helm chart
EBS_name.sh

#!/bin/bash

if [[ -z "$1" ]]
    then
        echo -e "error: \033[31;7m Please Give name for Deployment.. example : EBS_name.sh <chartname> <path_for_chart>\e[0m";
    exit
fi

if [[ -z "$2" ]]
    then
        echo -e "error: \033[31;7m Please Give path for chart.. example : EBS_name.sh $1 <path_for_chart>\e[0m";
    exit
fi

kubectl delete configmap name-config -n kube-system
kubectl create configmap name-config --from-literal=SPECIAL_NAME_KEY=$1 -n kube-system
kubectl delete pods -n kube-system -l=app=ebs-csi-controller
helm install $1 $2 -n $1 --create-namespace

for this i had controller.yaml file in the same location as this script
which i use to deploy ebs-csi driver with configmap to extract that value and use it to tag valume with Name Tag.

controller.yaml

---
kind: Deployment
apiVersion: apps/v1
metadata:
  name: ebs-csi-controller
  labels:
    app.kubernetes.io/name: aws-ebs-csi-driver
spec:
  replicas: 2
  selector:
    matchLabels:
      app: ebs-csi-controller
      app.kubernetes.io/name: aws-ebs-csi-driver
  template:
    metadata:
      labels:
        app: ebs-csi-controller
        app.kubernetes.io/name: aws-ebs-csi-driver
    spec:
      nodeSelector:
        kubernetes.io/os: linux
      serviceAccountName: ebs-csi-controller-sa
      priorityClassName: system-cluster-critical
      tolerations:
        - key: CriticalAddonsOnly
          operator: Exists
        - operator: Exists
          effect: NoExecute
          tolerationSeconds: 300
      containers:
        - name: ebs-plugin
          image: k8s.gcr.io/provider-aws/aws-ebs-csi-driver:v1.2.0
          imagePullPolicy: IfNotPresent
          args:
            # - {all,controller,node} # specify the driver mode
            - --endpoint=$(CSI_ENDPOINT)
            - --logtostderr
            - --v=2
#######################change i made for extract value from configmap #############
            - --extra-tags=Name=$(SPECIAL_NAME_KEY)
 ##################################
          env:
            - name: CSI_ENDPOINT
              value: unix:///var/lib/csi/sockets/pluginproxy/csi.sock
            - name: CSI_NODE_NAME
              valueFrom:
                fieldRef:
                  fieldPath: spec.nodeName
            - name: AWS_ACCESS_KEY_ID
              valueFrom:
                secretKeyRef:
                  name: aws-secret
                  key: key_id
                  optional: true
            - name: AWS_SECRET_ACCESS_KEY
              valueFrom:
                secretKeyRef:
                  name: aws-secret
                  key: access_key
                  optional: true
#########################################change i made to use configmap for Tag value####################
            - name: SPECIAL_NAME_KEY
              valueFrom: 
                configMapKeyRef:
                  name: name-config
                  key: SPECIAL_NAME_KEY
###################################################
          volumeMounts:
            - name: socket-dir
              mountPath: /var/lib/csi/sockets/pluginproxy/
          ports:
            - name: healthz
              containerPort: 9808
              protocol: TCP
          livenessProbe:
            httpGet:
              path: /healthz
              port: healthz
            initialDelaySeconds: 10
            timeoutSeconds: 3
            periodSeconds: 10
            failureThreshold: 5
          readinessProbe:
            httpGet:
              path: /healthz
              port: healthz
            initialDelaySeconds: 10
            timeoutSeconds: 3
            periodSeconds: 10
            failureThreshold: 5
        - name: csi-provisioner
          image: k8s.gcr.io/sig-storage/csi-provisioner:v2.1.1
          args:
            - --csi-address=$(ADDRESS)
            - --v=2
            - --feature-gates=Topology=true
            - --extra-create-metadata
            - --leader-election=true
            - --default-fstype=ext4
          env:
            - name: ADDRESS
              value: /var/lib/csi/sockets/pluginproxy/csi.sock
          volumeMounts:
            - name: socket-dir
              mountPath: /var/lib/csi/sockets/pluginproxy/
        - name: csi-attacher
          image: k8s.gcr.io/sig-storage/csi-attacher:v3.1.0
          args:
            - --csi-address=$(ADDRESS)
            - --v=2
            - --leader-election=true
          env:
            - name: ADDRESS
              value: /var/lib/csi/sockets/pluginproxy/csi.sock
          volumeMounts:
            - name: socket-dir
              mountPath: /var/lib/csi/sockets/pluginproxy/
        - name: csi-snapshotter
          image: k8s.gcr.io/sig-storage/csi-snapshotter:v3.0.3
          args:
            - --csi-address=$(ADDRESS)
            - --leader-election=true
          env:
            - name: ADDRESS
              value: /var/lib/csi/sockets/pluginproxy/csi.sock
          volumeMounts:
            - name: socket-dir
              mountPath: /var/lib/csi/sockets/pluginproxy/
        - name: csi-resizer
          image: k8s.gcr.io/sig-storage/csi-resizer:v1.0.0
          imagePullPolicy: Always
          args:
            - --csi-address=$(ADDRESS)
            - --v=2
          env:
            - name: ADDRESS
              value: /var/lib/csi/sockets/pluginproxy/csi.sock
          volumeMounts:
            - name: socket-dir
              mountPath: /var/lib/csi/sockets/pluginproxy/
        - name: liveness-probe
          image: k8s.gcr.io/sig-storage/livenessprobe:v2.2.0
          args:
            - --csi-address=/csi/csi.sock
          volumeMounts:
            - name: socket-dir
              mountPath: /csi
      volumes:
        - name: socket-dir
          emptyDir: {}

And it works best.

@jordiprats
Copy link

the extraTags feature is nice for setting a global "default". Could it be possible to set tags using some labels/annotations on the PV/PVC?

@ilyapcommit
Copy link

ilyapcommit commented Oct 20, 2021

the additional tagging of created EBS volumes is also important for tuning of backup plans/backup selections (assignments), especially in production environments

@olemarkus
Copy link

Have the same requirement here in order to make the most out of AWS Backup.

@sarvajit-sankar-comprinno

@harshdevl @wongma7 , is there a way to pass a tag with value containing a space like name=First Name, to the extraVolumeTags flag?

@buddhdev-harsh
Copy link

@sarvajit-sankar-comprinno

@harshdevl @wongma7 , is there a way to pass a tag with value containing a space like name=First Name, to the extraVolumeTags flag?

Well I haven't tried something like that before but you can also make configmap of that with space names and then use it in ebs controller file like below.

#180 (comment)

@tbondarchuk
Copy link

A bit offtop but for those who got to this issue like me, looking for just having a Name tag:

I've just found out that Name tag is created if driver's chart is deployed with controller.k8sTagClusterId=CLUSTERNAME.

tags without k8sTagClusterId:

kubernetes.io/created-for/pv/name | pvc-196694cc-3e6c-4bde-bedf-3709ee981da3
kubernetes.io/created-for/pvc/name | ebs-claim
kubernetes.io/created-for/pvc/namespace | default
ebs.csi.aws.com/cluster | true
CSIVolumeName | pvc-196694cc-3e6c-4bde-bedf-3709ee981da3

tags with k8sTagClusterId configured:

kubernetes.io/created-for/pv/name | pvc-fb58e538-da47-4524-a58c-6a94de4bd252
kubernetes.io/cluster/sandbox | owned
kubernetes.io/created-for/pvc/namespace | default
ebs.csi.aws.com/cluster | true
CSIVolumeName | pvc-fb58e538-da47-4524-a58c-6a94de4bd252
KubernetesCluster | sandbox
kubernetes.io/created-for/pvc/name | ebs-claim
Name | sandbox-dynamic-pvc-fb58e538-da47-4524-a58c-6a94de4bd252

Tested on chart version 2.4.1

P.S. chart's values.yaml comment says ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional). which is rather vague. I believe it would be much clearer to have it mentioned Name and KubernetesCluster tags explicitly

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 3, 2022
@OliverCole
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 3, 2022
@rdpsin
Copy link
Contributor

rdpsin commented Apr 5, 2022

Hey folks, we have a design for SC tags in #1190 and the PR for it in #1199.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 4, 2022
@pierluigilenoci
Copy link

@rdpsin any news about this?

@rdpsin
Copy link
Contributor

rdpsin commented Jul 5, 2022

@pierluigilenoci The driver supports StorageClass tags since v1.6.0.

@pierluigilenoci
Copy link

@rdpsin then this should be closed.

@rdpsin
Copy link
Contributor

rdpsin commented Jul 5, 2022

I agree.

@rdpsin
Copy link
Contributor

rdpsin commented Jul 5, 2022

Closing this since the CSI driver now supports tagging through StorageClass.parameters since v1.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.