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

add agent-enable-quit annotation #330

Merged
merged 4 commits into from
Apr 5, 2022
Merged

add agent-enable-quit annotation #330

merged 4 commits into from
Apr 5, 2022

Conversation

tvoran
Copy link
Member

@tvoran tvoran commented Apr 1, 2022

New annotation to enable the /agent/v1/quit endpoint on an injected agent. This option defaults to false, and will be set if true on the existing cache listener, or a new localhost listener with a basic cache stanza configured since that's required for an agent listener.

The agent-cache-listener-port annotation can be used to change the port.

Example Job:

apiVersion: batch/v1
kind: Job
metadata:
  name: quit

spec:
  template:
    metadata:
      annotations:
        vault.hashicorp.com/agent-inject: "true"
        vault.hashicorp.com/role: "internal-app"
        vault.hashicorp.com/agent-inject-secret-database-config.txt: "internal/data/database/config"
        vault.hashicorp.com/agent-inject-template-database-config.txt: |
          {{- with secret "internal/data/database/config" -}}
          postgresql://{{ .Data.data.username }}:{{ .Data.data.password }}@postgres:5432/wizard
          {{- end -}}
        vault.hashicorp.com/agent-enable-quit: "true"
    spec:
      serviceAccountName: internal-app
      containers:
      - name: nginx
        image: nginx
        command: ["/bin/bash", "-c"]
        args:
          - |
            sleep 10
            curl -X POST http://localhost:8200/agent/v1/quit

      restartPolicy: Never
  backoffLimit: 4

tvoran added 2 commits March 31, 2022 22:48
New annotation to enable the /agent/v1/quit endpoint on an injected
agent. This option has no default, and will be set on the existing
cache listener, or a new localhost listener with a basic cache stanza
configured since that's required for an agent listener.
@tvoran tvoran requested review from tomhjp, swenson and imthaghost April 1, 2022 06:08
// that's required for an agent listener.
if a.EnableQuit != nil {
if len(config.Listener) > 0 {
config.Listener[0].AgentAPI = &AgentAPI{
Copy link
Contributor

Choose a reason for hiding this comment

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

In the vast majority of cases, there will only be 1 listener, but should we maybe set it on all listeners? Just choosing the first one seems surprising, and for users who want to finesse which listeners it is and isn't on, I think it's ok to not give them a dedicated annotation for that but let them fully define their own config.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the injector can currently create a config with more than one listener, can it? I think the only way to inject an agent that has multiple listener is to specify the full config in a configmap, and in that case this code wouldn't be triggered.

But I can add a loop since it does look a little jarring to only reference listener[0] here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I forgot about that - makes sense.

// If EnableQuit has been set, set it on the listener. If a listener hasn't
// been defined, set it on a new one. Also add a simple cache stanza since
// that's required for an agent listener.
if a.EnableQuit != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do anything if it's false, given it defaults to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, and I went back and forth on this quite a bit. It would certainly be simpler to only set the enable_quit config if the annotation has been set to true, and maybe that's the best to do for now.

Currently EnableQuit is setup as a pointer so the config is set only if the annotation was specified (true or false), but since it defaults to false in the agent, just omitting the config setting here should be sufficient. And it's probably very unlikely that we'd ever default to enabling the quit endpoint on the vault agent side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that seems like a safe bet for now 👍

Copy link
Member Author

@tvoran tvoran Apr 4, 2022

Choose a reason for hiding this comment

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

Updated in b316106 to only set the config if EnableQuit is true.

tvoran added 2 commits April 4, 2022 13:54
Since the agent is defaulting to false, and it's very unlikely that
will change. This also meant the Agent.EnableQuit field doesn't need
to be a pointer any longer, so refactored that to make the logic a
little simpler.
@tvoran tvoran requested a review from tomhjp April 4, 2022 23:36
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tvoran tvoran merged commit c25fcef into main Apr 5, 2022
@tvoran tvoran deleted the VAULT-5282/quit-annotation branch April 5, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants