Additional k8s secret labels

[jamiedel818]

This pull requests extends Pentagon's secret mapping config to allow for extra labels to be added to the generated Kubernetes secret(s).

additionalSecretLabels can optionally be added to a mapping config. additionalSecretLabels will be merged with the top level default label config value.

Extended existing test cases to account for scenarios where no additional labels have been provided, ensuring the default label still exists. Introduced new test cases where additional labels are provided, ensuring the default label still exists.

[dfinkel]

nit: this is equivalent to labels := maps.Clone(mapping.AdditionalSecretLabels)

Another option would be to only replace the make call with the map literal it's replacing (map[string]string{LabelKey: r.labelValue}). Then it's clear that we don't expect to have additional labels most of the time.

[jamiedel818]

Thanks for the feedback. I just pushed a change that uses the latter approach you suggested to maintain clarity.

[sergiosalvatore]

Sorry for the back and forth...

[sergiosalvatore]

I know David already commented on this code, but I think it actually needs to be written the other way in order to not mess with the reconciliation logic here and here.

Suggested change

	labels := map[string]string{LabelKey: r.labelValue}
	maps.Copy(labels, mapping.AdditionalSecretLabels)
	labels := maps.Clone(mapping.AdditionalSecretLabels)
	labels[LabelKey] = r.labelValue

This ensures that the label key that pentagon uses to find its secrets is always set and can't be accidentally overwritten. I'd suggest adding a test that confirms this. I suppose we can also add some extra validation to the config to make sure that the user isn't trying to overwrite the pentagon label with one of their own.

[jamiedel818]

When writing the tests for this I noticed that if AdditionalSecretLabels is not passed in the config, making it nil, the result of labels := maps.Clone(mapping.AdditionalSecretLabels) is nil.

To avoid this, I've decided to initialize AdditionalSecretLabels in SetDefaults if they are not specified by the user. This avoids any potential nil map panics in the code.

Otherwise, I can do a nil check on AdditionalSecretLabels in the reflector code after cloning.

Let me know what you think.

[sergiosalvatore]

Thanks... While what you did is perfectly fine, it might actually be more "ergonomic" to do a nil check in the reflector code and not bother defaulting it to an empty map in SetDefaults. That would allow you to avoid initializing it in the tests and everything "just works."