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

feat(oidc): allow for secret csi driver mounts #250

Merged
merged 6 commits into from
Mar 13, 2024

Conversation

rumstead
Copy link
Contributor

@rumstead rumstead commented Mar 4, 2024

kiali/kiali#6942

To deploy the token in a scalable and secure manner, we want to leverage the secret CSI drivers backed by secret management tools. The secret CSI driver uses a pod's service account for authorization and will only create the secret when there is a mount requested. However, the kiali helm chat doesn't allow us to add any volume mounts which in turn doesn't trigger the secret creation.

The goal of this PR is to allow a user to add additional mounts and leverage the existing custom_secrets configuration option.

@rumstead rumstead marked this pull request as draft March 4, 2024 20:11
kiali-server/values.yaml Outdated Show resolved Hide resolved
@rumstead
Copy link
Contributor Author

rumstead commented Mar 4, 2024

I still need to test this against a secret csi driver, I am just using existing deployments that leverage the secret csi driver to guide the yaml here. Once I test it, I will publish the PR and share some of the evidence.

Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com>
@rumstead
Copy link
Contributor Author

rumstead commented Mar 8, 2024

I still need to test this against a secret csi driver, I am just using existing deployments that leverage the secret csi driver to guide the yaml here. Once I test it, I will publish the PR and share some of the evidence.

❯ vault kv get secret/app/kiali/oidc_secret
========== Secret Path ==========
secret/data/app/kiali/oidc_secret

======= Metadata =======
Key                Value
---                -----
created_time       2024-03-08T18:50:46.321771966Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            1

==== Data ====
Key      Value
---      -----
kiali    hello from vault kiali

❯ kubectl get secret kiali -o yaml
apiVersion: v1
data:
  oidc-secret: aGVsbG8gZnJvbSB2YXVsdCBraWFsaQ==
kind: Secret
metadata:
  creationTimestamp: "2024-03-08T18:50:50Z"
  labels:
    secrets-store.csi.k8s.io/managed: "true"
  name: kiali
  namespace: cnp-istio
  ownerReferences:
  - apiVersion: apps/v1
    kind: ReplicaSet
    name: kiali-59d9667b5b
    uid: 00ad1a4c-0a49-44ae-bc57-818d1435e459
  resourceVersion: "13121"
  uid: 49012f2c-cefd-4b22-bb29-de59d5befd95
type: Opaque

❯ kubectl exec kiali-59d9667b5b-tmvw8 -- cat /mnt/secrets-store/oidc_secret
hello from vault kiali

❯ k get deploy kiali -o yaml | yq '[.spec.template.spec.containers[0].volumeMounts, .spec.template.spec.volumes]'
## volumeMounts
- - mountPath: /kiali-configuration
    name: kiali-configuration
  - mountPath: /kiali-cert
    name: kiali-cert
  - mountPath: /kiali-secret
    name: kiali-secret
  - mountPath: /kiali-cabundle
    name: kiali-cabundle
  - mountPath: /mnt/secrets-store
    name: kiali-secret-csi
## volumes
- - configMap:
      defaultMode: 420
      name: kiali
    name: kiali-configuration
  - name: kiali-cert
    secret:
      defaultMode: 420
      optional: true
      secretName: istio.kiali-service-account
  - name: kiali-secret
    secret:
      defaultMode: 420
      optional: true
      secretName: kiali
  - configMap:
      defaultMode: 420
      name: kiali-cabundle
      optional: true
    name: kiali-cabundle
  - csi:
      driver: secrets-store.csi.k8s.io
      readOnly: true
      volumeAttributes:
        secretProviderClass: kiali-secretprovider
    name: kiali-secret-csi

@jmazzitelli let me know your thoughts

@rumstead rumstead marked this pull request as ready for review March 8, 2024 19:00
@jmazzitelli jmazzitelli self-requested a review March 9, 2024 01:24
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

wow, is this all we need :) I have a trivial change request. But if this is all you need, then I can do the operator changes.

kiali-server/values.yaml Outdated Show resolved Hide resolved
@jmazzitelli
Copy link
Contributor

@rumstead Can you provide simple test procedures so I can manually test this? Doesn't have to mean installing CSI -- I just want to see what you used for the helm values that got the mounts to be defined in the deployment (even if it means without CSI that my deployment doesn't start). A dry-run of helm is fine, too.

I think you are using this for values, but I'm not sure:

deployment:
  custom_secrets:
    csi:
      driver: secrets-store.csi.k8s.io
      readOnly: true
      volumeAttributes:
        secretProviderClass: "kiali-secretprovider"

Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com>
@rumstead
Copy link
Contributor Author

@rumstead Can you provide simple test procedures so I can manually test this? Doesn't have to mean installing CSI -- I just want to see what you used for the helm values that got the mounts to be defined in the deployment (even if it means without CSI that my deployment doesn't start). A dry-run of helm is fine, too.

I think you are using this for values, but I'm not sure:

deployment:
  custom_secrets:
    csi:
      driver: secrets-store.csi.k8s.io
      readOnly: true
      volumeAttributes:
        secretProviderClass: "kiali-secretprovider"

Sure thing, here is the values.yaml I used called local-values.yaml.

helm template kiali-server -f local-values.yaml

auth:
  strategy: "openid"
  openid:
    client_id: "abc-123"
    issuer_uri:  "https://foo"
    http_proxy: "http://proxy-foo"
    https_proxy: "http://proxy-foo"
    insecure_skip_verify_tls: true
    disable_rbac: true
deployment:
  image_version: v1.76.0
  version_label: v1.76.0
  custom_secrets:
    - name: "kiali-secret-csi"
      mount: "/mnt/secrets-store"
      csi:
        driver: secrets-store.csi.k8s.io
        readOnly: true
        volumeAttributes:
          secretProviderClass: kiali-secretprovider
server:
  web_port: 443
  web_schema: https

@jmazzitelli
Copy link
Contributor

Here's a quick test:

  1. Make sure you pull this PR and build the helm charts via make build-helm-charts
  2. Run this command:
helm template \
  --show-only templates/deployment.yaml \
  --set deployment.custom_secrets[0].name=csi-secret-name \
  --set deployment.custom_secrets[0].mount=/csi-secret-mount \
  --set deployment.custom_secrets[0].csi.driver=csi-secret-driver \
  --set deployment.custom_secrets[0].csi.readOnly=true \
  --set deployment.custom_secrets[0].csi.volumeAttributes.secretProviderClass=csi-secret-providerclass \
  kiali-server \
  _output/charts/kiali-server-1.82.0-SNAPSHOT.tgz

Essentially, the values that command passes in are these:

deployment:
  custom_secrets:
  - name: "csi-secret-name"
    mount: "/csi-secret-mount"
    csi:
      driver: csi-secret-driver
      readOnly: true
      volumeAttributes:
        secretProviderClass: csi-secret-providerclass
  1. Examine the Deployment yaml that results and see that it contains the CSI information.

volume mount:

          volumeMounts:
...
            - name: csi-secret-name
              mountPath: "/csi-secret-mount"

volume:

      volumes:
...
        - name: csi-secret-name
          csi:
            driver: csi-secret-driver
            readOnly: true
            volumeAttributes:
              secretProviderClass: csi-secret-providerclass

@jmazzitelli
Copy link
Contributor

@rumstead please confirm that test procedure I documented in the previous comment is correct and that it produces what you expect.

@rumstead
Copy link
Contributor Author

@rumstead please confirm that test procedure I documented in the previous comment is correct and that it produces what you expect.

I get the expected output:

        volumeMounts:
...
        - name: csi-secret-name
          mountPath: "/csi-secret-mount"


volumes:
...
      - name: csi-secret-name
        csi: 
          driver: csi-secret-driver
          readOnly: true
          volumeAttributes:
            secretProviderClass: csi-secret-providerclass
...

@jmazzitelli
Copy link
Contributor

jmazzitelli commented Mar 12, 2024

@rumstead I'm doing more testing and I noticed something that doesn't look broken, but I think I will need to document somewhere.

In my test procedure - in the helm template command - add --set deployment.custom_secrets[0].optional=true and notice the optional field isn't in the generated yaml (which is expected, because looking at this PR, it doesn't use that field in the if-stmt when csi is defined).

So run this:

helm template --show-only templates/deployment.yaml --set deployment.custom_secrets[0].name=csi-secret-name --set deployment.custom_secrets[0].mount=/csi-secret-mount --set deployment.custom_secrets[0].csi.driver=csi-secret-driver --set deployment.custom_secrets[0].csi.readOnly=true --set deployment.custom_secrets[0].csi.volumeAttributes.secretProviderClass=csi-secret-providerclass --set deployment.custom_secrets[0].optional=true kiali-server _output/charts/kiali-server-1.82.0-SNAPSHOT.tgz

and you don't see the optional field. You can see what happens if you set the optional field on a non-CSI secret by adding these params to that above command:

--set deployment.custom_secrets[1].name=second-name --set deployment.custom_secrets[1].optional=true --set deployment.custom_secrets[1].mount=/second-mount

That results in:

        - name: second-name
          secret:
            secretName: second-name
            optional: true

Does the CSI secret configuration support the "optional" field like the normal secret config? I don't think it does. I just tried it and I get validation errors when I try - edit the deployment and add optional: true to the csi field and I see this: # deployments.apps "kiali" was not valid: * patch: Invalid value: "map[spec:map[template:map[spec:map[]]]]": strict decoding error: unknown field "spec.template.spec.volumes[4].csi.optional".

It does not look like the csi section supports an optional field (like the secret section does).

If that is true, then I should probably change the docs here to say this optional field is ignored/not used when using csi.

(update: I added a small blurb in those docs in this commit in the operator PR: kiali/kiali-operator@b8848eb)

@jmazzitelli
Copy link
Contributor

After some quick manual testing of my own, this PR is ready to merge as is the operator PR

@rumstead
Copy link
Contributor Author

@rumstead I'm doing more testing and I noticed something that doesn't look broken, but I think I will need to document somewhere.

In my test procedure - in the helm template command - add --set deployment.custom_secrets[0].optional=true and notice the optional field isn't in the generated yaml (which is expected, because looking at this PR, it doesn't use that field in the if-stmt when csi is defined).

So run this:

helm template --show-only templates/deployment.yaml --set deployment.custom_secrets[0].name=csi-secret-name --set deployment.custom_secrets[0].mount=/csi-secret-mount --set deployment.custom_secrets[0].csi.driver=csi-secret-driver --set deployment.custom_secrets[0].csi.readOnly=true --set deployment.custom_secrets[0].csi.volumeAttributes.secretProviderClass=csi-secret-providerclass --set deployment.custom_secrets[0].optional=true kiali-server _output/charts/kiali-server-1.82.0-SNAPSHOT.tgz

and you don't see the optional field. You can see what happens if you set the optional field on a non-CSI secret by adding these params to that above command:

--set deployment.custom_secrets[1].name=second-name --set deployment.custom_secrets[1].optional=true --set deployment.custom_secrets[1].mount=/second-mount

That results in:

        - name: second-name
          secret:
            secretName: second-name
            optional: true

Does the CSI secret configuration support the "optional" field like the normal secret config? I don't think it does. I just tried it and I get validation errors when I try - edit the deployment and add optional: true to the csi field and I see this: # deployments.apps "kiali" was not valid: * patch: Invalid value: "map[spec:map[template:map[spec:map[]]]]": strict decoding error: unknown field "spec.template.spec.volumes[4].csi.optional".

It does not look like the csi section supports an optional field (like the secret section does).

If that is true, then I should probably change the docs here to say this optional field is ignored/not used when using csi.

(update: I added a small blurb in those docs in this commit in the operator PR: kiali/kiali-operator@b8848eb)

yea great catch and shout. I agree, I don't think the csi section supports it since it would be strange to say I want the driver to mount this but optionally. Thanks for adding the optional snippet.

Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

this is ready to go - approve.

@jmazzitelli jmazzitelli merged commit b09c3af into kiali:master Mar 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires operator PR Requires changes to the operator
Projects
Development

Successfully merging this pull request may close these issues.

2 participants