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

Delimiter annotation #664

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Delimiter annotation #664

merged 6 commits into from
Nov 5, 2024

Conversation

Diliz
Copy link
Contributor

@Diliz Diliz commented Jul 8, 2024

Rebased @cailtlinelfring annotations branch with delimiter annotations feature

rebased content of #226 instead of making my own (I was using leftdelim and rightdelim annotations in my version, nearly the same as @caitlinelfring version

@Diliz Diliz requested a review from a team as a code owner July 8, 2024 12:28
Copy link

hashicorp-cla-app bot commented Jul 8, 2024

CLA assistant check
All committers have signed the CLA.

@Diliz
Copy link
Contributor Author

Diliz commented Jul 8, 2024

fix hashicorp/vault-helm#348 as well

@Diliz
Copy link
Contributor Author

Diliz commented Jul 17, 2024

@tvoran @benashz How much time does it take for pull requests from outsiders to be checked and merged? (Just to know if this will be available soon or not on my side)

@benashz
Copy link
Contributor

benashz commented Jul 18, 2024

The original pull request talks a bit about handling templates included in a Helm chart. I wonder if we could recommend escaping the template instead of adding support for specifying alternate template delimiters? We are suggesting that approach here: hashicorp/vault-secrets-operator#619 (comment)

@Diliz
Copy link
Contributor Author

Diliz commented Jul 22, 2024

The original pull request talks a bit about handling templates included in a Helm chart. I wonder if we could recommend escaping the template instead of adding support for specifying alternate template delimiters? We are suggesting that approach here: hashicorp/vault-secrets-operator#619 (comment)

Humm, this helm approache is a workaround, not something to fix the real issue, currently it's a hassle to use go templating inside vault templates in kubernetes, if you want to make templating over templating over termplating, it becomes impossible to use if you don't know beforehand how many times the template will be templated, so escaping it is not a solution in this case.

Why not simply adding this delim annotations to make it convenient and be able to differentiate the default go templating from the one for the vault agent?

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Thanks for updating this! It tested good locally for me; just left a couple thoughts.

Something else I think may be useful here is a way to set the default template delimiters for all the templates in a Pod, instead of per-template. Perhaps the delimiter annotations by themselves (without the secret suffix) could be used for this? Just something I was thinking about while reviewing this, but we can do it in a follow-up PR too.

agent-inject/agent/annotations.go Outdated Show resolved Hide resolved
agent-inject/agent/annotations.go Show resolved Hide resolved
@Diliz Diliz force-pushed the delimiter-annotation branch from 7beb30b to 2528e4d Compare August 5, 2024 08:40
@markush81
Copy link

Just stumbled over this and it is exactly what i need as well. Doing the escaping kinda gets easily a hell if you want to have parts replaced by helm template and parts by vault.

Any ETA for this PR?

@Diliz
Copy link
Contributor Author

Diliz commented Aug 28, 2024

Just stumbled over this and it is exactly what i need as well. Doing the escaping kinda gets easily a hell if you want to have parts replaced by helm template and parts by vault.

Any ETA for this PR?

Waiting for @tvoran and @benashz comments and approval, I'm still following this case as I need it implemented as well 👍

agent-inject/agent/annotations.go Outdated Show resolved Hide resolved
agent-inject/agent/annotations.go Outdated Show resolved Hide resolved
agent-inject/agent/annotations.go Show resolved Hide resolved
agent-inject/agent/annotations.go Outdated Show resolved Hide resolved
@Diliz Diliz force-pushed the delimiter-annotation branch from 2e1441b to 0b1833d Compare October 28, 2024 07:10
@Diliz
Copy link
Contributor Author

Diliz commented Oct 28, 2024

Made the requested changes, it should be good now :)

  • Rebased as well

@tvoran tvoran added this to the 1.5.0 milestone Oct 31, 2024
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Just a couple more minor things.

agent-inject/agent/annotations.go Outdated Show resolved Hide resolved
agent-inject/agent/config_test.go Outdated Show resolved Hide resolved
agent-inject/agent/config_test.go Outdated Show resolved Hide resolved
@tvoran
Copy link
Member

tvoran commented Nov 5, 2024

Thanks so much! We'll get this into the next release.

@tvoran tvoran merged commit 1063509 into hashicorp:main Nov 5, 2024
10 checks passed
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.

5 participants