spring-cloud / spring-cloud-config

External configuration (server and client) for Spring Cloud
Apache License 2.0
1.97k stars 1.29k forks source link

Option.PROFILE_SPECIFIC is missing for profile specific PropertySources from Vault #1997

Closed marbon87 closed 2 years ago

marbon87 commented 3 years ago

PropertySources from cloud config get the wrong order in spring boot applications, if the property sources from cloud config contain profile specific properties from vault and the path in vault is defined as advised in reference documentation with the postfix ,profile, e.g. /secret/my-app,profile

The cloud config server serves the correct order when requesting properties trought http:

{
  "name": "my-app",
  "profiles": [
    "profile_a"
  ],
  "label": null,
  "version": null,
  "state": null,
  "propertySources": [
    {
      "name": "vault:my-app,profile_a",
      "source": { }
    },
    {
      "name": "vault:my-app",
      "source": { }
    },
    {
      "name": "ssh://git@mygit.git.git/application-profile_a.yml",
      "source": { }
    },
    {
      "name": "ssh://git@mygit.git.git/application.yml",
      "source": { }
    }
  ]
}

But when i look at the environment or health endpoint of the app, that requests the properties, the resulting property sources of the app are:

{
  status: "UP",
  components: {
    clientConfigServer: {
      status: "UP",
      details: {
        propertySources: [
          "configserver:ssh://git@mygit.git/application-profile_a.yml",
          "configserver:vault:my-app,profile_b",
          "configserver:vault:my-app",
          "configserver:ssh://git@mygit.git/application.yml",
          "configClient"
        ]
      }
    }
  }
}

The problem is that ConfigServerConfigDataLoader.java checks if the name of the property source contains "-profile." before adding the Option.PROFILE_SPECIFIC.

My suggestion to fix this, is to add another if else-block, that checks if the name starts with "vault" and ends with the profile name.

ryanjbaxter commented 3 years ago

Do you specify the order in your config server configuration? Maybe providing a sample would help.

marbon87 commented 3 years ago

Here is the example: https://github.com/marbon87/spring-cloud-config-issue-1997

See the readme.md to get it running.

It contains a server with native and vault backend. The first request against the server (curl -s http://localhost:8080/appl/profile | jq .) shows that the vault properties have higher priority in cloud config. But the second request (curl -s http://localhost:8081/actuator/health | jq .) shows, that the property sources in the client have a different order because the profile specific properties from vault are not detected as profile specific.

ryanjbaxter commented 3 years ago

Ah I see, any interest in submitting a PR?

Outside of the list being in the wrong order for the actuator endpoint is there any other impacts on functionality?

marbon87 commented 3 years ago

I can submit a PR, but i am not sure what the best solution is.

My local workaround is to rename the property source from vault to vault:appname_profile.vault to match the condition in ConfigServerConfigDataLoader. Otherwise i have to wait for a fix of cloudconfigclient and until all applications are using the new version, before i can rollout vault in production. The problem with the workaround is, that i have to copy a lot of code to get the possibility to rename the property source name for vault.

Therefore i would extract the creation of the property source to a method, that can be overriden, on the server side to simplify a temporary workaround on the server side. In addition i would add another if else-block in ConfigServerConfigDataLoader, that checks if the name starts with "vault" and ends with the profile name.

ryanjbaxter commented 3 years ago

In addition i would add another if else-block in ConfigServerConfigDataLoader, that checks if the name starts with "vault" and ends with the profile name.`

Sounds like the best approach is to modify ConfigServerConfigDataLoader with the new logic that accounts for the vault property source name.

spring-cloud-issues commented 3 years ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

marbon87 commented 3 years ago

Hi @ryanjbaxter , i created https://github.com/spring-cloud/spring-cloud-config/pull/2004. It took me some time to create an integration test because the error only occurs if a profile specific import is used in the client application.

marbon87 commented 2 years ago

Hi @ryanjbaxter, could this be merged?

ryanjbaxter commented 2 years ago

Sorry for the slow response. I think it might make sense to have the PR made against the 3.0.x branch and we can merge the changes forward into the other branches.

marbon87 commented 2 years ago

Hi @ryanjbaxter, i rebased the branch on the main branch, but that is seems to be version 4 already. Do you cherry-pick the commit on the 3.x.x-Branch after merging it into the main branch?

ryanjbaxter commented 2 years ago

I was able to cherry pick it to the 3.1.x branch and main, but 3.0.x required more changes. We have no current plans to release 3.0.x at the moment so I am not sure its a big deal