Skip to content

Add mount and namespace for Vault Enterprise use in new Kubernetes pr… #93

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.

Sign up for GitHub

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

Open
briantopping wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add mount and namespace for Vault Enterprise use in new Kubernetes pr… #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 1 addition & 46 deletions apis/kubernetes/v1alpha1/zz_secretbackend_types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions apis/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ type ProviderConfigSpec struct {
// +optional
Role *string `json:"role,omitempty"`

// Mount of the auth method to log in with.
// +optional
Mount *string `json:"mount,omitempty"`

// Credentials required to authenticate to this provider.
// There are many options to authenticate. They include
// - token - (Optional) Vault token that will be used
Expand Down
5 changes: 5 additions & 0 deletions apis/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 0 additions & 41 deletions config/provider-metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4711,47 +4711,6 @@ resources:
default-service and default-batch which specify the type to return unless the client
requests a different type at generation time.
importStatements: []
vault_kubernetes_secret_backend:
subCategory: ""
description: Creates a Kubernetes Secrets Engine in Vault.
name: vault_kubernetes_secret_backend
title: vault_kubernetes_secret_backend resource
examples:
- name: config
manifest: |-
{
"default_lease_ttl_seconds": 43200,
"description": "kubernetes secrets engine description",
"disable_local_ca_jwt": false,
"kubernetes_ca_cert": "${file(\"/path/to/cert\")}",
"kubernetes_host": "https://127.0.0.1:61233",
"max_lease_ttl_seconds": 86400,
"path": "kubernetes",
"service_account_jwt": "${file(\"/path/to/token\")}"
}
argumentDocs:
disable_local_ca_jwt: |-
- (Optional) Disable defaulting to the local CA certificate and
service account JWT when Vault is running in a Kubernetes pod.
kubernetes_ca_cert: |-
- (Optional) A PEM-encoded CA certificate used by the
secrets engine to verify the Kubernetes API server certificate. Defaults to the local
pod’s CA if Vault is running in Kubernetes. Otherwise, defaults to the root CA set where
Vault is running.
kubernetes_host: |-
- (Optional) The Kubernetes API URL to connect to. Required if the
standard pod environment variables KUBERNETES_SERVICE_HOST or KUBERNETES_SERVICE_PORT
are not set on the host that Vault is running on.
namespace: |-
- (Optional) The namespace to provision the resource in.
The value should not contain leading or trailing forward slashes.
The namespace is always relative to the provider's configured namespace.
Available only for Vault Enterprise.
service_account_jwt: |-
- (Optional) The JSON web token of the service account used by the
secrets engine to manage Kubernetes credentials. Defaults to the local pod’s JWT if Vault
is running in Kubernetes.
importStatements: []
vault_kubernetes_secret_backend_role:
subCategory: ""
description: Creates a role for the Kubernetes Secrets Engine in Vault.
Expand Down
21 changes: 0 additions & 21 deletions examples-generated/kubernetes/v1alpha1/secretbackend.yaml
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions examples-generated/kubernetes/v1alpha1/secretbackendrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,3 @@ spec:
serviceAccountName: test-service-account-with-generated-token
tokenDefaultTtl: 21600
tokenMaxTtl: 43200

---

apiVersion: kubernetes.vault.upbound.io/v1alpha1
kind: SecretBackend
metadata:
annotations:
meta.upbound.io/example-id: kubernetes/v1alpha1/secretbackendrole
labels:
testing.upbound.io/example-name: config
name: config
spec:
forProvider:
description: kubernetes secrets engine description
disableLocalCaJwt: false
kubernetesCaCert: ${file("/path/to/cert")}
kubernetesHost: https://127.0.0.1:61233
path: kubernetes
serviceAccountJwtSecretRef:
key: attribute.token
name: example-secret
namespace: upbound-system
12 changes: 9 additions & 3 deletions internal/clients/vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const (
errUnmarshalCredentials = "cannot unmarshal vault credentials as JSON"
errNoServiceAccountToken = "no service account token found"
errNoRole = "no role provided"
errNoMount = "no mount provided"

// Service account token path
serviceAccountTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token"
Expand Down Expand Up @@ -187,11 +188,16 @@ func kubernetesAuth(pc *v1beta1.ProviderConfig, ps *terraform.Setup) error {
return errors.New(errNoRole)
}

if pc.Spec.Mount == nil {
Copy link
Member

@sergenyalcin sergenyalcin Aug 20, 2025

Choose a reason for hiding this comment

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

To be honest, I don't have a strong knowledge of provider-vault auth methods. But it seems that here we are making a breaking change for the secret cred auth and making the Mount required, right?

I see that there was a defaulting here (to kubernetes value before. And I assume that it worked that way. Therefore, I think we should preserve this old behaviour. Currently, this value is required, and we return an error when it is not provided. However, in the past, we defaulted when it was not provided.

From the perspective of preserving the old behaviour, I think we should continue to default instead of returning an error here. (Of course, I assume that the old situation, i.e., when ‘kubernetes’ was passed to Mount, worked.) What do you think?

Copy link
Author

@briantopping briantopping Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, I don't have a strong knowledge of provider-vault auth methods. But it seems that here we are making a breaking change for the secret cred auth and making the Mount required, right?

If you take a look at the git blame, this is new code. I found these errors before the code was merged but lost track of my commits before they were pushed. This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".

From the perspective of preserving the old behaviour, I think we should continue to default instead of returning an error here. (Of course, I assume that the old situation, i.e., when ‘kubernetes’ was passed to Mount, worked.) What do you think?

Yes, that makes good sense, thanks. I will update the PR.

Copy link
Contributor

@erhancagirici erhancagirici Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, I am also not a vault expert too, looking at the changes purely from the provider perspective. I am a bit confused, I would be happy if you can clarify the following:

This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".

I see that "capability-wise" Vault OSS users are not affected, but from ProviderConfig API perspective this is breaking. The kubernetes auth code (prior to this PR) is already shipped with v2.2.0.
So there can be potential adopters, and they can create a kubernetes auth ProviderConfig without specifying spec.mount. After this PR, their ProviderConfig would not work, and they would be forced to configure a parameter, which they actually has no other option.

My questions are:

  • (prior to this PR) did the kubernetes auth configuration never work or was it not valid at all, even for OSS? In other words, are we fixing a bug for something that never worked here? I am assuming that is not the case.

  • What are the potential spec.mount values for Vault Enterprise users that configure kubernetes auth? Would kubernetes still be a sane default for Enterprise users? I assume that Enterprise users would need to remember to configure this in any case since you said:

It's really only required for Vault Enterprise users, who will almost never use "kubernetes".

I am in favor of defaulting it to kubernetes,
but I am also trying to assess the scenario, where an Enterprise users "forgets" to set spec.mount and it gets defaulted to kubernetes. Would there be an adverse effect of this? Would it fail or result in an unintended valid configuration?

Copy link

@pstefka pstefka Sep 23, 2025

Choose a reason for hiding this comment

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

This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".

VOSS users can indeed have several kubernetes secret engines enabled = having an option to set mount point is not a Vault Enterprise feature. It is a must if several kubernetes secret engines exist on a Vault instance (OSS, or enterprise).

(prior to this PR) did the kubernetes auth configuration never work or was it not valid at all, even for OSS? In other words, are we fixing a bug for something that never worked here? I am assuming that is not the case.

It did work, with default kubernetes path / mount point only.

I am in favor of defaulting it to kubernetes,
but I am also trying to assess the scenario, where an Enterprise users "forgets" to set spec.mount and it gets defaulted to kubernetes. Would there be an adverse effect of this? Would it fail or result in an unintended valid configuration?

Fortunately keeping the default kubernetes shouldn't have side affects, i.e. setting wrong mount point wouldn't validate any auth token as it would be provided by wrong cluster / wouldn't match required role.

return errors.New(errNoMount)
}

ps.Configuration[keyAuthLoginJWT] = []any{
map[string]string{
"jwt": string(jwt),
"mount": "kubernetes",
"role": *pc.Spec.Role,
"jwt": string(jwt),
"mount": *pc.Spec.Mount,
"role": *pc.Spec.Role,
"namespace": pc.Spec.Namespace,
},
}

Expand Down
Loading
Loading