Conversation
maithyton
left a comment
There was a problem hiding this comment.
@liad5h Thank you for taking this on and being patient! The PR is generally on a good path :). I have some change requests. I'm happy to sync up and help out with addressing them if you need.
Would you mind dropping a small description in the PR and something like below in CHANGELOG.md under Unreleased?
Improvements:
* Add support to configure sidecar type (proxy or agent) and proxy's auto auth token use through Pod Annotations [GH-496](https://github.com/hashicorp/vault-k8s/pull/496)
| "vault.hashicorp.com/agent-telemetry-stackdriver_namespace": "foo", | ||
| "vault.hashicorp.com/agent-telemetry-stackdriver_debug_logs": "false", | ||
| "vault.hashicorp.com/agent-telemetry-prefix_filter": `["+vault.token", "-vault.expire", "+vault.expire.num_leases"]`, | ||
| "vault.hashicorp.com/agent-telemetry-prefix_filter": `["+vault.token", "-vault.expire", "+vault.expire.num_leases"]`, |
There was a problem hiding this comment.
Could you revert the accidental space?
| } | ||
|
|
||
| func (a *Agent) sidecarType() string { | ||
| if a.SidecarType != "" && !(a.SidecarType == "agent" || a.SidecarType == "proxy") { |
There was a problem hiding this comment.
| if a.SidecarType != "" && !(a.SidecarType == "agent" || a.SidecarType == "proxy") { | |
| if a.SidecarType != "" && (a.SidecarType == "agent" || a.SidecarType == "proxy") { |
I think you meant if a.SidecarType is not empty, and a.SidecarType is "agent" or "proxy", then return a.SidecarType?
| arg := fmt.Sprintf(DefaultContainerArg, agent.SidecarType) | ||
| if container.Args[0] != arg { | ||
| t.Errorf("arg value wrong, should have been %s, got %s", arg, container.Args[0]) |
There was a problem hiding this comment.
Could we also add a similar assertion for agent.ProxyUseAutoAuthToken?
|
|
||
| if container.Args[0] != DefaultContainerArg { | ||
| t.Errorf("arg value wrong, should have been %s, got %s", DefaultContainerArg, container.Args[0]) | ||
| arg := fmt.Sprintf(DefaultContainerArg, agent.SidecarType) |
There was a problem hiding this comment.
Could you add different test cases under TestContainerSidecar() to test varying annotation sets containing 2 new annotations?
vault-k8s/agent-inject/agent/annotations.go
Lines 20 to 28 in 412e109
vault-k8s/agent-inject/agent/container_sidecar_test.go
Lines 210 to 216 in 412e109
|
@thyton is this the official pr for this topic? Vault agent api proxy will be removed in 2026 Q2, I think this will break a lot of use-cases of this project. |
No description provided.