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

Allow driver node daemonSet annotations to be configurable #1923

Conversation

AndrewSirenko
Copy link
Contributor

@AndrewSirenko AndrewSirenko commented Feb 6, 2024

Is this a bug fix or adding new feature?
helm feature

What is this PR about? / Why do we need it?
Closes #1918

"Running static analysis tools like KubeLinter against the chart fails due to pods (specifically ebs-plugin node.containerSecurityContext) allowing a container to run in privilege mode.

These can be ignored via annotations to the resource (not the pod but the daemonset) but currently there are no means to provide them."

What testing is done?
CI

Manually testing the following values file:

controller:
  deploymentAnnotations:
    test.deployment.annotation/foo: bar
    some.deployment.annotation/foo2: bar2
node:
  daemonSetAnnotations:
    test.annotation/foo: bar
    test.annotation.2/foo2: bar2
  enableWindows: true

Result:

❯ helm upgrade \
  --install aws-ebs-csi-driver \
  --namespace kube-system \
  ./charts/aws-ebs-csi-driver \
  --values test-ebs-config-values.yaml  --debug

history.go:56: [debug] getting history for release aws-ebs-csi-driver
upgrade.go:150: [debug] preparing upgrade for aws-ebs-csi-driver
upgrade.go:158: [debug] performing update for aws-ebs-csi-driver
upgrade.go:321: [debug] dry run for aws-ebs-csi-driver
Release "aws-ebs-csi-driver" has been upgraded. Happy Helming!
NAME: aws-ebs-csi-driver
LAST DEPLOYED: Tue Feb  6 18:06:10 2024
NAMESPACE: kube-system
STATUS: pending-upgrade
REVISION: 2
USER-SUPPLIED VALUES:
node:
  daemonSetAnnotations:
    test.annotation.2/foo2: bar2
    test.annotation/foo: bar

COMPUTED VALUES:
...
# Source: aws-ebs-csi-driver/templates/node.yaml
kind: DaemonSet
apiVersion: apps/v1
metadata:
  name: ebs-csi-node
  namespace: kube-system
  labels:
    app.kubernetes.io/name: aws-ebs-csi-driver
    app.kubernetes.io/instance: aws-ebs-csi-driver
    helm.sh/chart: aws-ebs-csi-driver-2.27.0
    app.kubernetes.io/version: "1.27.0"
    app.kubernetes.io/component: csi-driver
    app.kubernetes.io/managed-by: Helm
  annotations:
    test.annotation.2/foo2: bar2
    test.annotation/foo: bar
...

Proof in live cluster:

❯ kubectl describe daemonset ebs-csi-node -n kube-system
Name:           ebs-csi-node
Selector:       app=ebs-csi-node,app.kubernetes.io/instance=aws-ebs-csi-driver,app.kubernetes.io/name=aws-ebs-csi-driver
Node-Selector:  kubernetes.io/os=linux
Labels:         app.kubernetes.io/component=csi-driver
                app.kubernetes.io/instance=aws-ebs-csi-driver
                app.kubernetes.io/managed-by=Helm
                app.kubernetes.io/name=aws-ebs-csi-driver
                app.kubernetes.io/version=1.27.0
                helm.sh/chart=aws-ebs-csi-driver-2.27.0
Annotations:    deprecated.daemonset.template.generation: 1
                meta.helm.sh/release-name: aws-ebs-csi-driver
                meta.helm.sh/release-namespace: kube-system
                test.annotation.2/foo2: bar2
                test.annotation/foo: bar

❯ kubectl describe daemonset ebs-csi-node-windows -n kube-system
Name:           ebs-csi-node-windows
Selector:       app=ebs-csi-node,app.kubernetes.io/instance=aws-ebs-csi-driver,app.kubernetes.io/name=aws-ebs-csi-driver
Node-Selector:  kubernetes.io/os=windows
Labels:         app.kubernetes.io/component=csi-driver
                app.kubernetes.io/instance=aws-ebs-csi-driver
                app.kubernetes.io/managed-by=Helm
                app.kubernetes.io/name=aws-ebs-csi-driver
                app.kubernetes.io/version=1.27.0
                helm.sh/chart=aws-ebs-csi-driver-2.27.0
Annotations:    deprecated.daemonset.template.generation: 1
                meta.helm.sh/release-name: aws-ebs-csi-driver
                meta.helm.sh/release-namespace: kube-system

❯ kubectl describe deployment ebs-csi-controller -n kube-system
...
Annotations:            deployment.kubernetes.io/revision: 2
                        meta.helm.sh/release-name: aws-ebs-csi-driver
                        meta.helm.sh/release-namespace: kube-system
                        some.deployment.annotation/foo2: bar2
                        test.deployment.annotation/foo: bar

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 6, 2024
Copy link

github-actions bot commented Feb 6, 2024

Code Coverage Diff

This PR does not change the code coverage

@AndrewSirenko
Copy link
Contributor Author

AndrewSirenko commented Feb 6, 2024

Shall I also do a similar deploymentAnnotations for controller within this PR? Done.

I thought about adding a customAnnotations that would be shared across ebs csi driver resources like the existing customLabels, but I assume cx might want more fine-grained control over annotations (in original issue, cx only wanted a linter annotation on daemonSet).

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 6, 2024
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR lgtm, tested with the following values.yaml:

controller:
  deploymentAnnotations:
    ignore-check.kube-linter.io/unset-cpu-requirements: ""
    
node:
  daemonsetAnnotations:
    ignore-check.kube-linter.io/privileged-container: ""
    ignore-check.kube-linter.io/unset-cpu-requirements: ""
    ignore-check.kube-linter.io/run-as-non-root: ""
    ignore-check.kube-linter.io/sensitive-host-mounts: ""
    ignore-check.kube-linter.io/privilege-escalation-container: ""

@torredil
Copy link
Member

torredil commented Feb 6, 2024

Do note that 6 lint errors remain, coming from the ebs-csi-driver-test pod specification.

$ kube-linter lint aws-ebs-csi-driver

Error: found 6 lint errors

We should strive to expose a minimal set of necessary parameters and avoid overloading values.yaml. There is no need to expose annotations for that pod, but to fully address #1918 we should resolve the remaining lint errors.

@torredil
Copy link
Member

torredil commented Feb 6, 2024

As previously mentioned, this PR lgtm. For clarity, the previous comment is specifically referring to addressing kube-linter errors for ebs-csi-driver-test without introducing a third parameter in values.yaml. That change is not in scope for this PR but still relevant to #1918.

@ConnorJC3
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
@k8s-ci-robot k8s-ci-robot merged commit be9426c into kubernetes-sigs:master Feb 7, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow resource annotations to be configurable
4 participants