Uses label when searching secrets in [Spring]VaultEnvironmentRepository

[kvmw]

Like other EnvironmentRepositories, VaultEnvironmentRepository::findOne and SpringVaultEnvironmentRepository::findOne should use the label argument passed to them when searching for secrets in vault.

So calling findOne("my-app", "my-profile", "my-label") should search for secrets in vault in the following locations with the exact order:

• secret/myapp,my-profile,my-label
• secret/application,my-profile,my-label (Assuming application is the default-key)
• secret/myapp,my-label
• secret/application,my-label (Assuming application is the default-key)

Currently the search locations are the followings (note the missing label):

• secret/myapp,my-profile
• secret/application,my-profile (Assuming application is the default-key)
• secret/myapp
• secret/application (Assuming application is the default-key)

This PR fixes the above issue.

Breaking Change

Since this wrong behaviour has been there from the day one, based on my knowledge, everyone using the vault backend has been relying on it and they have been adding their vault secrets in the wrong location (without label). Fixing this behaviour is going to break all those applications using the vault backend.

Update 23 Sep. 2024

I've updated the PR with the following changes:

• Added a feature flag (enableLabel) for the new changes (using label in search). The feature flag (enableLabel) is off by default.

• Added the defaultLabel to Vault properties. It is master by default and only used when above feature flag is on.

• When above feature flag is on, the vault secrets should have always all three segments ( [application name | 'application'],[profile | 'default'],[label | 'master'] ) in their paths.

• Updated the vault docs.

[ryanjbaxter]

So this will break existing apps since the location in vault will be different now?

[kvmw]

So this will break existing apps since the location in vault will be different now?

Yes, the secret keys should include a label (mostly master or main ) at the end. Just like this one in spring-cloud-config-sample which I've changed in the PR.

[ryanjbaxter]

IMO this will be problematic to include in anything but a major in its current form.

What if we didn't include the label in the key if it is the same as the default label? That would preserve the existing behavior for the more part.

I also want to run this by the rest of the team to see what they think.

[kvmw]

Right, If we skip appending label to the key if it is same as default label (master or main?) for sure we will have less broken apps. But still apps with labels other than default one will be impacted. So as you pointed out this change might need to go to a major version.

[ryanjbaxter]

master or main?

The default is main currently, but I believe there is a property which also controls the default label.

But still apps with labels other than default one will be impacted.

I guess I wonder if anyone is using a label and if so why if it was never actually used with Vault?

[kvmw]

Default label is only configurable for some backends like git or jdbc.
CredHub uses master as default label which is not configurable.
Vault was not using the label therefore it had no default label. In this PR, following CredHub approach, I've set it to master which is again is not configurable.

Regarding your second point:
The client application can set spring.cloud.config.label together with profile and application name when calling the config-server. config-server uses those values to call all active environment repositories (including vault). So imagine a config-server with git and vault backend. client app can pass a git branch name as label intended for git backend but the same value will be used by vault backend too.

[ryanjbaxter]

Got it.

After discussing with the team we think that this new functionality should be wrapped in a feature flag which is disabled by default and should only go into main.

[kvmw]

@ryanjbaxter
I pushed a new commit with a feature flag (enableLabel) as suggested. Let me know if the flag needs a better name.

[kvmw]

@ryanjbaxter would you mind having a look at latest changes (feature flag) ?

[ryanjbaxter]

Looks good, can you document the new property?

[kvmw]

@ryanjbaxter while trying to update the documents, I just realised an ambiguity in the secret path which is introduced by my changes. A complete secret key would be something like myapp,myprofile,mylabel but a key like myapp,foo can be interpreted as application myapp and either profile foo or label foo.
That is because:

• The default profile (default) is not included in the secret path. (existing behaviour)
• In this PR, the profileSeparator (comma by default) is reused by me to separate the label from the rest of the secret key.

The simplest solution which I can think of is always including the profile (even if it is default) in the valut key. So that we have a fix key format consist of 3 separate segments (name,profile,label). What do you think?

Note: Since we have the feature flag for the changes in this PR, It should not break any existing functionality.

[ryanjbaxter]

The simplest solution which I can think of is always including the profile (even if it is default) in the valut key. So that we have a fix key format consist of 3 separate segments (name,profile,label). What do you think?

I agree with this. When making a request to the config server only the label is optional in the path, you must include the application name and profile even if that profile is default. So it makes sense that we require the application name and profile in the key.

Note: Since we have the feature flag for the changes in this PR, It should not break any existing functionality.

Does that mean today that we don't require the profile when creating the key today?

[kvmw]

Does that mean today that we don't require the profile when creating the key today?

yes, the profile part is optional. secrets can be stored in the following paths in valut:

• myapp,my-profile -> Both app and profile are provided
• application,my-profile -> Default app name(application) and profile are provided
• myapp -> Only app name is provided which means the profile is implicitly default.
• application -> Default app name (application) and, implicitly, default profile (default).

To keep the backward compatibility, the behaviour should stay the same when the enableLabel flag is false.
But when flag is enabled the path should always have 3 segments. [app name | 'application'],[profile | 'default'],[label | 'master']

[ryanjbaxter]

Got it.

But I would say that it should always have 2 segments, application and profile, label being optional. Again much like the rest API label is the only optional part.

[kvmw]

But I would say that it should always have 2 segments, application and profile, label being optional. Again much like the rest API label is the only optional part.

Label is optional in API level but when omitted a default label (currently master but can be configurable later) will be used. So in the vault path it is not optional, just like Git, CredHub or other backends.

[kvmw]

@ryanjbaxter I've updated the PR and I believe it is ready for the final review.

[ryanjbaxter]

@kvmw one small this, I think the default label should be main instead of master. We moved away from using master a while ago.

[kvmw]

@kvmw one small this, I think the default label should be main instead of master. We moved away from using master a while ago.

@ryanjbaxter Updated the PR with main as default label.