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.

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

ConfigMap with missing vault section should default to env vars #353

Merged
merged 1 commit into from
May 25, 2022
+46 −3
Merged

ConfigMap with missing vault section should default to env vars #353

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
43 changes: 43 additions & 0 deletions agent-inject/agent/container_env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package agent

import (
"encoding/base64"
"strconv"

corev1 "k8s.io/api/core/v1"
)
Expand Down Expand Up @@ -64,6 +65,48 @@ func (a *Agent) ContainerEnvVars(init bool) ([]corev1.EnvVar, error) {
Name: "VAULT_CONFIG",
Value: b64Config,
})
} else {
// set up environment variables to access Vault since "vault" section may not be present in the config
if a.Vault.Address != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_ADDR",
Value: a.Vault.Address,
})
}
Comment on lines +70 to +75
Copy link
Member

Choose a reason for hiding this comment

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

(Just making sure I understand) It looks like all these can be overridden by the user with annotations from https://www.vaultproject.io/docs/platform/k8s/injector/annotations#vault-annotations? So like for the vault address, the order of precedence would be:

  • vault stanza in configmap
  • This setting of VAULT_ADDR
  • The value of vault.hashicorp.com/service

We may want to add a note about this new behavior in or adjacent to https://www.vaultproject.io/docs/platform/k8s/injector/examples

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I believe that is correct. I'll double-check it before merging, and add a ticket to update those docs.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I confirmed that vault.hashicorp.com/service will override the agent container VAULT_ADDR settings.

if a.Vault.CACert != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_CACERT",
Value: a.Vault.CACert,
})
}
if a.Vault.CAKey != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_CAPATH",
Value: a.Vault.CAKey,
})
}
if a.Vault.ClientCert != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_CLIENT_CERT",
Value: a.Vault.ClientCert,
})
}
if a.Vault.ClientKey != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_CLIENT_KEY",
Value: a.Vault.ClientKey,
})
}
envs = append(envs, corev1.EnvVar{
Name: "VAULT_SKIP_VERIFY",
Value: strconv.FormatBool(a.Vault.TLSSkipVerify),
})
if a.Vault.TLSServerName != "" {
envs = append(envs, corev1.EnvVar{
Name: "VAULT_TLS_SERVER_NAME",
Value: a.Vault.TLSServerName,
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also include VAULT_NAMESPACE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. Namespace isn't part of the vault config block, only auto_auth. And if we set VAULT_NAMESPACE, it could end up overriding a namespace the user sets in their auto_auth config: https://github.com/hashicorp/vault/blob/9334ae5f338c90f9a6d65e749794593e50acd48d/command/agent.go#L367-L369. However, if they've explicitly set the namespace annotation, it does seem like a reasonable thing to add. Overall it's a difficult one to call, but it maybe feels safer to exclude it for now, to maintain the current behaviour?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly. I only targeted the things that are in the vault config block.

}

// Add IRSA AWS Env variables for vault containers
Expand Down
4 changes: 2 additions & 2 deletions agent-inject/agent/container_env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ func TestContainerEnvs(t *testing.T) {
expectedEnvs []string
}{
{Agent{}, []string{"VAULT_CONFIG"}},
{Agent{ConfigMapName: "foobar"}, []string{}},
{Agent{Vault: Vault{Address: "http://localhost:8200"}, ConfigMapName: "foobar"}, []string{"VAULT_SKIP_VERIFY", "VAULT_ADDR"}},
{Agent{Vault: Vault{ClientMaxRetries: "0"}}, []string{"VAULT_CONFIG", "VAULT_MAX_RETRIES"}},
{Agent{Vault: Vault{ClientTimeout: "5s"}}, []string{"VAULT_CONFIG", "VAULT_CLIENT_TIMEOUT"}},
{Agent{Vault: Vault{ClientMaxRetries: "0", ClientTimeout: "5s"}}, []string{"VAULT_CONFIG", "VAULT_MAX_RETRIES", "VAULT_CLIENT_TIMEOUT"}},
{Agent{ConfigMapName: "foobar", Vault: Vault{ClientMaxRetries: "0", ClientTimeout: "5s", LogLevel: "info", ProxyAddress: "http://proxy:3128"}}, []string{"VAULT_MAX_RETRIES", "VAULT_CLIENT_TIMEOUT", "VAULT_LOG_LEVEL", "HTTPS_PROXY"}},
{Agent{ConfigMapName: "foobar", Vault: Vault{Address: "http://localhost:8200", ClientMaxRetries: "0", ClientTimeout: "5s", LogLevel: "info", ProxyAddress: "http://proxy:3128"}}, []string{"VAULT_MAX_RETRIES", "VAULT_CLIENT_TIMEOUT", "VAULT_LOG_LEVEL", "HTTPS_PROXY", "VAULT_SKIP_VERIFY", "VAULT_ADDR"}},
{Agent{Vault: Vault{GoMaxProcs: "1"}}, []string{"VAULT_CONFIG", "GOMAXPROCS"}},
}

Expand Down
2 changes: 1 addition & 1 deletion agent-inject/agent/container_sidecar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ func TestContainerSidecarConfigMap(t *testing.T) {
t.Errorf("creating container sidecar failed, it shouldn't have: %s", err)
}

expectedEnvs := 2
expectedEnvs := 9
if len(container.Env) != expectedEnvs {
t.Errorf("wrong number of env vars, got %d, should have been %d", len(container.Env), expectedEnvs)
}
Expand Down