Possibility to use template and template-file annotations without sec… #505

saturn4er · 2023-07-30T00:00:29Z

Summary

This Pull Request introduces enhancements to the Vault Kubernetes integration by simplifying the secret injection process. Specifically, it removes the need for the redundant and ineffective vault.hashicorp.com/agent-inject-secret-foo: someKey annotation.

Motivation

In the current implementation, users are required to include the vault.hashicorp.com/agent-inject-secret-foo: someKey annotation in their configuration. However, this annotation doesn't contribute to the functionality and only adds unnecessary complexity.

Changes Introduced

With this PR, users will no longer need to specify the vault.hashicorp.com/agent-inject-secret-foo: someKey annotation. Instead, the necessary information about secrets can be fully determined by the provided templates or template files, such as vault.hashicorp.com/agent-inject-template-foo: "{{- with secret "someKey" -}} {{- end }}" or vault.hashicorp.com/agent-inject-template-file-foo: "someFile".
This results in a cleaner and more intuitive configuration.

…ret annotation

hashicorp-cla · 2023-07-30T00:00:32Z

All committers have signed the CLA.

tomhjp

I really like the concept for this, and would love to get it merged. My comments just resolve around wanting to preserve backwards compatibility so we don't break existing users. We can discuss a breaking change if you think it's necessary, but my read is that it shouldn't be too hard to avoid it.

agent-inject/agent/annotations.go

agent-inject/agent/annotations_test.go

agent-inject/agent/config_test.go

saturn4er · 2023-08-04T10:37:43Z

@tomhjp Ready, old test cases are now works and I added new ones to cover case when only template or template-file annotation set up

tomhjp · 2023-08-04T12:43:11Z

agent-inject/agent/annotations.go

+	for _, secret := range templateSecrets {
+		secrets = append(secrets, secret)
+	}

-			templateName := fmt.Sprintf("%s-%s", AnnotationAgentInjectTemplate, raw)
-			if val, ok := a.Annotations[templateName]; ok {
-				s.Template = val
-			}
-			if s.Template == "" {
-				templateFileAnnotation := fmt.Sprintf("%s-%s", AnnotationAgentInjectTemplateFile, raw)
-				if val, ok := a.Annotations[templateFileAnnotation]; ok {
-					s.TemplateFile = val
-				}
-			}
+	for _, secret := range simpleSecrets {
+		if templateSecrets[secret.Name] == nil && templateFileSecrets[secret.Name] == nil {
+			secrets = append(secrets, secret)
+		}
+	}

-			s.MountPath = a.Annotations[AnnotationVaultSecretVolumePath]
-			mountPathAnnotationName := fmt.Sprintf("%s-%s", AnnotationVaultSecretVolumePath, raw)
-			if val, ok := a.Annotations[mountPathAnnotationName]; ok {
-				s.MountPath = val
-			}
+	for _, secret := range templateFileSecrets {
+		if templateSecrets[secret.Name] == nil {
+			secrets = append(secrets, secret)
+		}
+	}


The order of precedence here doesn't look quite right to me. To make the new behaviour a superset of the previous behaviour, we should always set secret.Path and secret.Template if those annotations are present. Then we should set secret.TemplateFile if that annotation is present and secret.Template == "".

I think we could probably simplify by separating the logic for parsing secret names from the logic for reading in config for each secret. It might not be quite right, but I pushed b00bf8d to illustrate the idea.

As you can check, Path is only required in case when Template is not set. to put this value in template, so my changes won't affect anything. Here's the only one place where Path is used: https://github.com/hashicorp/vault-k8s/blob/main/agent-inject/agent/config.go#L186.

So if we're setting Path unconditionally, why we should set TemplateFile conditionally? I pushed some changes, please check if you're ok with them

saturn4er · 2023-08-12T22:07:21Z

@tomhjp any issues with this implementation?

tomhjp

Sorry for the delay doing another round of reviewing. This looks really close now, one change that I think is a requirement and the other comments are just nits.

tomhjp · 2023-08-14T09:52:29Z

agent-inject/agent/annotations_test.go

-		if len(agent.Secrets) == 0 {
-			t.Error("Secrets length was zero, it shouldn't have been")
-		}
+			var found bool


The updated assertions for this test are reasonable, but please could we tighten them a bit further? The test case input should probably now specify an array of struct { key, template string } to expect so it can handle the fourth test case precisely.

I rewrote it to maps

tomhjp · 2023-08-14T10:09:46Z

agent-inject/agent/annotations_test.go

@@ -495,7 +526,7 @@ func TestSecretTemplateFileAnnotations(t *testing.T) {
 				"vault.hashicorp.com/agent-inject-secret-foobar":        "test1",
 				"vault.hashicorp.com/agent-inject-template-foobar":      "foobarTemplate",
 				"vault.hashicorp.com/agent-inject-template-file-foobar": "/etc/config.tmpl",
-			}, "foobar", "foobarTemplate", "",
+			}, "foobar", "foobarTemplate", "/etc/config.tmpl",


I don't think this change makes sense in the context of what the test is trying to assert. And I think it's highlighting a backwards incompatibility. I'll leave another comment on the actual fix site.

As you wish :)

tomhjp · 2023-08-14T10:19:16Z

agent-inject/agent/annotations.go

+	for _, secret := range secrets {
+		secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, secret.Path)
+		secret.Template = a.annotationsSecretValue(AnnotationAgentInjectTemplate, secret.RawName, secret.Template)
+		secret.TemplateFile = a.annotationsSecretValue(AnnotationAgentInjectTemplateFile, secret.RawName, secret.TemplateFile)


Suggested change

secret.TemplateFile = a.annotationsSecretValue(AnnotationAgentInjectTemplateFile, secret.RawName, secret.TemplateFile)

if secret.Template == "" {

secret.TemplateFile = a.annotationsSecretValue(AnnotationAgentInjectTemplateFile, secret.RawName, secret.TemplateFile)

}

I know you disagreed with this elsewhere, but I still think this is worth doing. Here's where the downstream processing of secret.Template and secret.TemplateFile takes place:

vault-k8s/agent-inject/agent/config.go

Lines 180 to 214 in 02af6ab

var templates []*Template

for _, secret := range a.Secrets {

template := secret.Template

templateFile := secret.TemplateFile

if templateFile == "" {

template = secret.Template

if template == "" {

switch a.DefaultTemplate {

case "json":

template = fmt.Sprintf(DefaultJSONTemplate, secret.Path)

case "map":

template = fmt.Sprintf(DefaultMapTemplate, secret.Path)

}

}

}

filePathAndName := fmt.Sprintf("%s/%s", secret.MountPath, secret.Name)

if secret.FilePathAndName != "" {

filePathAndName = filepath.Join(secret.MountPath, secret.FilePathAndName)

}

tmpl := &Template{

Source: templateFile,

Contents: template,

Destination: filePathAndName,

LeftDelim: "{{",

RightDelim: "}}",

Command: secret.Command,

}

if secret.FilePermission != "" {

tmpl.Perms = secret.FilePermission

}

templates = append(templates, tmpl)

}

return templates

The logic is kind of confusingly written, but take the case where both the template and template-file annotations are non-empty. Previously, secret.Template is non-empty and secret.TemplateFile is empty, and the same happens in tmpl. Now, both are non-empty in secret and the same in tmpl. Maybe we could chase the behaviour of what happens next all the way down into consul-template, but I'd rather just take the high confidence route of keeping the localised behaviour the same here.

tomhjp · 2023-08-14T10:24:31Z

agent-inject/agent/annotations.go

-			secrets = append(secrets, s)
-		}
+	for _, secret := range secrets {
+		secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, secret.Path)


It reads a little strangely to me to use the values of fields we haven't set yet as the default. Could we just pass in "" as the default for all the lines except volume path to make it a little easier to read?

Suggested change

secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, secret.Path)

secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, "")

Yeah, I wrote like this just in case if there will be some default values setted at the top, so this one will not overwrite them.

tomhjp · 2023-08-16T10:34:21Z

@saturn4er I'm going to start kicking off a release soon, but I'd love to get this merged first. LMK if you're able to address the remaining comments - I pushed a commit that implements the comments here if that's helpful: 92153f6

saturn4er · 2023-08-16T11:37:26Z

@tomhjp, I've made revisions based on your suggestions. Could you please review the changes?

tomhjp

Thanks! Looks great 👍

Possibility to use template and template-file annotations without sec…

05d348a

…ret annotation

tomhjp reviewed Aug 4, 2023

View reviewed changes

agent-inject/agent/annotations.go Show resolved Hide resolved

agent-inject/agent/annotations_test.go Outdated Show resolved Hide resolved

agent-inject/agent/annotations_test.go Outdated Show resolved Hide resolved

agent-inject/agent/config_test.go Outdated Show resolved Hide resolved

saturn4er added 2 commits August 4, 2023 13:31

Backward-compatibility

be5a1af

Returned back old test cases

d9112d6

Remove debug log

1eb477f

tomhjp reviewed Aug 4, 2023

View reviewed changes

saturn4er added 3 commits August 4, 2023 16:36

CR suggestions

c46e1a1

Uncomment tests

808ff51

Remove spew

02af6ab

tomhjp reviewed Aug 14, 2023

View reviewed changes

CR suggestions

0707531

tomhjp approved these changes Aug 16, 2023

View reviewed changes

tomhjp merged commit e813ad9 into hashicorp:main Aug 16, 2023

tomhjp mentioned this pull request Aug 16, 2023

Prepare for 1.3.0 release #511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possibility to use template and template-file annotations without sec… #505

Possibility to use template and template-file annotations without sec… #505

saturn4er commented Jul 30, 2023

hashicorp-cla commented Jul 30, 2023 •

edited

Loading

tomhjp left a comment

saturn4er commented Aug 4, 2023

tomhjp Aug 4, 2023

saturn4er Aug 4, 2023 •

edited

Loading

saturn4er Aug 4, 2023

saturn4er commented Aug 12, 2023

tomhjp left a comment

tomhjp Aug 14, 2023

saturn4er Aug 16, 2023

tomhjp Aug 14, 2023

saturn4er Aug 16, 2023

tomhjp Aug 14, 2023

tomhjp Aug 14, 2023

saturn4er Aug 16, 2023

tomhjp commented Aug 16, 2023

saturn4er commented Aug 16, 2023

tomhjp left a comment

	var templates []*Template
	for _, secret := range a.Secrets {
	template := secret.Template
	templateFile := secret.TemplateFile
	if templateFile == "" {
	template = secret.Template
	if template == "" {
	switch a.DefaultTemplate {
	case "json":
	template = fmt.Sprintf(DefaultJSONTemplate, secret.Path)
	case "map":
	template = fmt.Sprintf(DefaultMapTemplate, secret.Path)
	}
	}
	}

	filePathAndName := fmt.Sprintf("%s/%s", secret.MountPath, secret.Name)
	if secret.FilePathAndName != "" {
	filePathAndName = filepath.Join(secret.MountPath, secret.FilePathAndName)
	}

	tmpl := &Template{
	Source: templateFile,
	Contents: template,
	Destination: filePathAndName,
	LeftDelim: "{{",
	RightDelim: "}}",
	Command: secret.Command,
	}
	if secret.FilePermission != "" {
	tmpl.Perms = secret.FilePermission
	}
	templates = append(templates, tmpl)
	}
	return templates

	secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, secret.Path)
	secret.Path = a.annotationsSecretValue(AnnotationAgentInjectSecret, secret.RawName, "")

Possibility to use template and template-file annotations without sec… #505

Possibility to use template and template-file annotations without sec… #505

Conversation

saturn4er commented Jul 30, 2023

Summary

Motivation

Changes Introduced

hashicorp-cla commented Jul 30, 2023 • edited Loading

tomhjp left a comment

Choose a reason for hiding this comment

saturn4er commented Aug 4, 2023

Choose a reason for hiding this comment

saturn4er Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saturn4er commented Aug 12, 2023

tomhjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomhjp commented Aug 16, 2023

saturn4er commented Aug 16, 2023

tomhjp left a comment

Choose a reason for hiding this comment

hashicorp-cla commented Jul 30, 2023 •

edited

Loading

saturn4er Aug 4, 2023 •

edited

Loading