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

enhancement(ci): support in helm vector-agent to specify command #7107

Merged
merged 3 commits into from May 12, 2021

Conversation

martinlevesque
Copy link
Contributor

@martinlevesque martinlevesque commented Apr 13, 2021

Ability to specify a command to the daemonsets containers. This is required to pass secrets with berglas (https://github.com/GoogleCloudPlatform/berglas) with Google Secret Manager (see here https://github.com/GoogleCloudPlatform/berglas/tree/main/examples/kubernetes#limitations )

@martinlevesque martinlevesque requested review from a team and jszwedko and removed request for a team April 13, 2021 13:23
Signed-off-by: Martin Levesque <levesque.martin@gmail.com>
@martinlevesque
Copy link
Contributor Author

martinlevesque commented May 7, 2021

@binarylogic @JeanMertz Hi guys, could you let me know if the pull request makes sense, and if there is anything to improve? Need to have it to be able to set this parameter, otherwise vector cannot be used with berglas to store secrets. Having it working by manually updating the yamls, but it's not ideal, better to use the helm.

@spencergilbert
Copy link
Contributor

@binarylogic @JeanMertz Hi guys, could you let me know if the pull request makes sense, and if there is anything to improve? Need to have it to be able to set this parameter, otherwise vector cannot be used with berglas to store secrets. Having it working by manually updating the yamls, but it's not ideal, better to use the helm.

Sorry for the delay - I'll take a look at this tonight.

I think the CI failure just needs the ./scripts/helm-template-snapshot.sh update run? I'm not super familiar with that section so I'll try and confirm with my review.

@spencergilbert spencergilbert self-requested a review May 11, 2021 22:05
@spencergilbert
Copy link
Contributor

I was right about the update command - at least locally. ./scripts/helm-template-snapshot.sh update then check exits 0, I check if this is documented in the contributing doc

@spencergilbert
Copy link
Contributor

@martinlevesque this looks fine and works - but I'm wondering if we need to add support to customize the args as well? Does just updating the command work for you? I'm having some trouble locally - what does your working config look like?

@martinlevesque
Copy link
Contributor Author

martinlevesque commented May 12, 2021

 @spencergilbert Yes it's probably better to add support for both cmd and args, with valid default values. Just updating the command worked. I didn't really test the helm chart thus, tested by changing the daemonset manually.

The daemonset I used is the following (only pasting the relevant part):

spec:
  minReadySeconds: 1
  revisionHistoryLimit: 10
  selector:
    matchLabels:
      app.kubernetes.io/instance: xxx
      app.kubernetes.io/name: vector-agent
  template:
    metadata:
      creationTimestamp: null
      labels:
        app.kubernetes.io/instance: xxx
        app.kubernetes.io/name: vector-agent
        vector.dev/exclude: "true"
    spec:
      containers:
      - args:
        - --config
        - /etc/vector/*.toml
        command:
        - /usr/bin/vector

Signed-off-by: Spencer Gilbert <spencer.gilbert@gmail.com>
@spencergilbert
Copy link
Contributor

@martinlevesque would you mind updating the helm-snapshots with ./scripts/helm-template-snapshot.sh update and I'll open another issue to allow configuration of the args as well?

@martinlevesque
Copy link
Contributor Author

@martinlevesque would you mind updating the helm-snapshots with ./scripts/helm-template-snapshot.sh update and I'll open another issue to allow configuration of the args as well?

@spencergilbert any idea why i'm getting the following:

$ ./scripts/helm-template-snapshot.sh update
Error: unknown flag: --create-namespace
helm.go:76: [debug] unknown flag: --create-namespace

@spencergilbert
Copy link
Contributor

@martinlevesque just checked locally and it looks to be a Helm v2 vs Helm v3 thing - It works with Helm v3.5.4.

I'm going to plan some work to review/update the documentation and the scripts

@martinlevesque
Copy link
Contributor Author

@spencergilbert i don't have that version currently unfortunately. Just added your account as collaborator in the cloned repository

@martinlevesque
Copy link
Contributor Author

@spencergilbert awesome, thx!

Signed-off-by: Spencer Gilbert <spencer.gilbert@gmail.com>
@spencergilbert spencergilbert merged commit 230a27e into vectordotdev:master May 12, 2021
@spencergilbert
Copy link
Contributor

Merging since this can't affect the benchmarks

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.

None yet

3 participants