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

Handles multiple application names in CredhubEnvironmentRepository #2447

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

kvmw
Copy link
Contributor

@kvmw kvmw commented Aug 9, 2024

The PR updates CredhubEnvironmentRepository to handle following cases:

  1. application name can be a comma separated string like foo,bar,baz. In this case, It should be spilted and each entry should be treated as app name. So the repository should search for foo/profile/label, bar/profile/label, baz/profile/label and finally application/profile/label. This was handled in other repositories like JGit or Valut but not in CredHub.

  2. If the default application name (application) is included in that comma separated string (like for example: foo,application,bar) it should be removed and added back to the end of the list. To make sure the default property source is added to the end of the list. Again other repositories are taking care of this but not CredHub.

  3. same for default profile (default). If it's included in the profile string (profile1,default,profile2), It should be removed and added back to the end of the list. Again other repositories are taking care of this but not CredHub.

Also, In current implementation, If there are no secrets at all in CredHub an empty PropertySource will be added to the environment ( envrionment.propertySources will have one PropertySource which has no properties) . But this PR skips that and envrionment.propertySources will be empty. If this is not desired, I can add that empty ProperySource back.

@ryanjbaxter
Copy link
Contributor

Thanks! Could you submit this against the 4.1.x branch and we can merge it forward?

@spencergibb what do you think about this change?

Also, In current implementation, If there are no secrets at all in CredHub an empty PropertySource will be added to the environment ( envrionment.propertySources will have one PropertySource which has no properties) . But this PR skips that and envrionment.propertySources will be empty. If this is not desired, I can add that empty ProperySource back.

That could be considered breaking, so I am not sure we want to make that change in 4.1.x. I guess we could do it in main.

@ryanjbaxter ryanjbaxter added this to the 4.1.4 milestone Aug 9, 2024
@kvmw kvmw changed the base branch from main to 4.1.x August 9, 2024 15:20
Signed-off-by: kvmw <mshamsi@broadcom.com>
@kvmw
Copy link
Contributor Author

kvmw commented Aug 13, 2024

Also, In current implementation, If there are no secrets at all in CredHub an empty PropertySource will be added to the environment ( envrionment.propertySources will have one PropertySource which has no properties) . But this PR skips that and envrionment.propertySources will be empty. If this is not desired, I can add that empty ProperySource back.

That could be considered breaking, so I am not sure we want to make that change in 4.1.x. I guess we could do it in main.

The second commit adds the main PropertySource, even if it is empty. The only caveat is the name of the PropertySource, in case application has more than one name. For example, given appOne,appTwo as application name in the current code, PropertySource name would be credhub-appOne,appTwo-myProfile-myLabel but the new code changes it to credhub-appOne-myProfile-myLabel.

@ryanjbaxter
Copy link
Contributor

Why change the property source name?

@kvmw
Copy link
Contributor Author

kvmw commented Aug 13, 2024

Why change the property source name?

PropertySource name has the following format: credhub-<APPLICATION-NAME>-<PROFILE>-<LABEL>.
Until now, because of this bug which I am trying to fix, for a given application name like appOne,appTwo we would have only one PropertySource which was named credhub-appOne,appTwo-default-master . But with the new changes we will have 2 property sources with the following names: credhub-appOne-default-master and credhub-appTwo-default-master. Assuming they are both empty, we just need to keep the first one.

@ryanjbaxter
Copy link
Contributor

Is this how other EnvironmentRepositories handle this situation as well?

@kvmw
Copy link
Contributor Author

kvmw commented Aug 13, 2024

If you mean splitting the application name, yes. In other repos application name is converted to an array and each individual item in array makes a separate property source.

If you are referring to empty property sources, no. not all of them. For example, Vault does not create empty property source.

@ryanjbaxter
Copy link
Contributor

Yes I was referring to the name, I just want to be as consistent as possible.

@ryanjbaxter ryanjbaxter merged commit eaa06aa into spring-cloud:4.1.x Aug 13, 2024
@kvmw
Copy link
Contributor Author

kvmw commented Aug 13, 2024

Yes I was referring to the name, I just want to be as consistent as possible.

JGit behaves the same.

a sample config like this:

spring:
  profiles:
    active: development,production
  application:
    name: cook,chef

will have the following property sources :

"configserver:https://github.com/cook-config/chef-production.properties"
"configserver:https://github.com/cook-config/cook-production.properties"
"configserver:https://github.com/cook-config/application-production.properties"
"configserver:https://github.com/cook-config/chef-development.properties"
"configserver:https://github.com/cook-config/cook-development.properties"
"configserver:https://github.com/cook-config/application-development.properties"
"configserver:https://github.com/cook-config/chef.properties"
"configserver:https://github.com/cook-config/cook.properties"
"configserver:https://github.com/cook-config/application.properties"

@kvmw kvmw deleted the kvmw/tmp branch October 17, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants