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: Allow TriggerAuthentication defaults via ENV variables for HashiCorp Vault #5196

Closed

Conversation

BojanZelic
Copy link
Contributor

@BojanZelic BojanZelic commented Nov 20, 2023

This PR sets vault defaults if specified via ENV variables. When configuring the operator, you can set ENV variables so users don't have to redefine vault settings every time they use vault authentication;

example:

hashiCorpVault:                                     # Optional.
  address: {hashicorp-vault-address}                # Optional. set via ENV: $VAULT_ADDR
  namespace: {hashicorp-vault-namespace}            # Optional. set via ENV: $VAULT_NAMESPACE
  authentication: token | kubernetes                # Optional. set via ENV: $VAULT_AUTH
  role: {hashicorp-vault-role}                      # Optional. set via ENV: $VAULT_ROLE
  mount: {hashicorp-vault-mount}                    # Optional. set via ENV: $VAULT_MOUNT
  credential:                                       # Optional.
    token: {hashicorp-vault-token}                  # Optional. set via ENV: $VAULT_TOKEN
    serviceAccount: {path-to-service-account-file}  # Optional. set via ENV: $VAULT_JWT_PATH
  secrets:                                          # Required.
  - parameter: {scaledObject-parameter-name}        # Required.
    key: {hasicorp-vault-secret-key-name}           # Required.
    path: {hasicorp-vault-secret-path}              # Required.

Checklist

Fixes # #4965

Relates to #

@BojanZelic BojanZelic requested a review from a team as a code owner November 20, 2023 23:53
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

@BojanZelic BojanZelic changed the title Vault triggerauth env values Vault: Allow TriggerAuthentication Defaults via ENV variables Nov 20, 2023
Signed-off-by: Bojan Zelic <bnzelic@gmail.com>
@tomkerkhove tomkerkhove changed the title Vault: Allow TriggerAuthentication Defaults via ENV variables feat: Allow TriggerAuthentication defaults via ENV variables for HashiCorp Vault Nov 21, 2023
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

I like this feature as it helps the vault integration (majority of the use cases have a single vault).
I'm not sure about the naming of the envs (I know that they are the hashicorp envs but KEDA supports more than it), maybe adding something like HASHICORP_* prefix we can be more clear (or even more, DEFAULT_HASHICORP_*).
WDYT @tomkerkhove @zroubalik ?

Could you open PR's to helm chart and docs explaining this feature?

@zroubalik
Copy link
Member

I like this feature as it helps the vault integration (majority of the use cases have a single vault). I'm not sure about the naming of the envs (I know that they are the hashicorp envs but KEDA supports more than it), maybe adding something like HASHICORP_* prefix we can be more clear (or even more, DEFAULT_HASHICORP_*). WDYT @tomkerkhove @zroubalik ?

Could you open PR's to helm chart and docs explaining this feature?

I do understand the problem, though I would actually prefer if we solve this in general for all config - thus having some global config for KEDA. If we start adding this kind of config via ENV variables the list would growth a lot in the future.

@tomkerkhove
Copy link
Member

I like this feature as it helps the vault integration (majority of the use cases have a single vault). I'm not sure about the naming of the envs (I know that they are the hashicorp envs but KEDA supports more than it), maybe adding something like HASHICORP_* prefix we can be more clear (or even more, DEFAULT_HASHICORP_*). WDYT @tomkerkhove @zroubalik ?
Could you open PR's to helm chart and docs explaining this feature?

I do understand the problem, though I would actually prefer if we solve this in general for all config - thus having some global config for KEDA. If we start adding this kind of config via ENV variables the list would growth a lot in the future.

I tend to agree on this

@BojanZelic
Copy link
Contributor Author

@zroubalik @tomkerkhove I understand that the original design might lead to a some complexity long-term.

What are your thoughts on any of the following alternatives for setting user-specified default values?

Option 1) Mutating Webhook
Extend the existing validating webhook & use a mutating-webhook to set user-configurable defaults if they're not specified?

A user could specify a scaledObject, triggerAuthentication, ect... yaml in a configfile; The mutating webhook would load this config file and set defaults for custom resources that keda manages. It would be generic & there wouldn't be any extra maintenance burden since the values would only be declared once.

example:
Keda mutating-webhook would parse this yaml file from a configmap specified on startup:

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
spec:
  hashiCorpVault:
    address: https://my-vault-url
    authentication: kubernetes
    credential:
      serviceAccount: /var/run/secrets/kubernetes.io/serviceaccount/token
    mount: kubernetes
    role: my-role

and set defaults whenever the TriggerAuthentication CR is created to the above values. The defaults will be persisted on the custom resource itself.

Option 2) Dynamic ENV variables
Use something like this package https://github.com/caarlos0/env to add env flags to custom resources that we wish to override; Keda would parse the ENV flags and set default values if the ENV variable is defined whenever the resources are loaded from kubernetes. The defaults wouldn't be persisted on the custom resource itself. Every value that we wish to override, would have to have an env flag set;

example:

type HashiCorpVault struct {
	Secrets []VaultSecret `json:"secrets"`
	// +optional
	Address string `json:"address" env:"DEFAULT_HASHICORP_VAULT_ADDR"`
	// +optional
	Authentication VaultAuthentication `json:"authentication" env:"DEFAULT_HASHICORP_AUTH"`
	// +optional
	Namespace string `json:"namespace,omitempty" env:"DEFAULT_HASHICORP_NAMESPACE"`
	// +optional
	Credential *Credential `json:"credential,omitempty" env:"DEFAULT_HASHICORP_CREDENTIAL"`
	// +optional
	Role string `json:"role,omitempty" env:"DEFAULT_HASHICORP_ROLE"`
	// +optional
	Mount string `json:"mount,omitempty" env:"DEFAULT_HASHICORP_MOUNT"`
}

If you have any other suggestions/approaches please let me know; I'd really like to get this feature implemented

@tomkerkhove
Copy link
Member

I think we should discuss this on the issue and take #5229 (from @majusmisiak) in to mind as they both have similar goal - Make it easier to pull configuration from outside the scaledjob/triggerauth. In this case, envFrom or so would be better.

Created #5233 to consolidate proposals

Copy link

stale bot commented Jan 30, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Jan 30, 2024
Copy link

stale bot commented Feb 7, 2024

This issue has been automatically closed due to inactivity.

@stale stale bot closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale All issues that are marked as stale due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants