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

Use a different policy for property overrides #22

Closed
wants to merge 1 commit into from

Conversation

dsyer
Copy link
Member

@dsyer dsyer commented Feb 7, 2019

Normally in the Environment it is better to let user-defined
properties take precedence, rather than trying to simply omit
them. This change makes the ClientSecurityAutoConfiguration rely
on that existing feature, rather than trying to inspect the property
sources manually and add guess which properties are set.

See #21

Normally in the `Environment` it is better to let user-defined
properties take precedence, rather than trying to simply omit
them. This change makes the `ClientSecurityAutoConfiguration` rely
on that existing feature, rather than trying to inspect the property
sources manually and add guess which properties are set.

See spring-projects#21
@jxblum
Copy link
Contributor

jxblum commented Feb 11, 2019

2 things:

  1. First, I am not omitting user-defined properties and they are/were in fact taking precedence. The method(s) you removed was preventing the cloud environment (PCF in this case, when using the PCC service) from overriding the user-supplied (Security) properties from a Spring Boot application.properties file, or potentially another source, when deployed with the app. You could argue the cloud environment should take precedence, but then how does 1 override the cloud security credentials? Additionally, a Spring Boot/GemFire ClientCache app can, when bound to a PCC instance in PCC, be assigned and granted multiple users/roles. Furthermore, this needs to work outside of PCF since SBDG is not specific to PCF (with PCC). Plus, I only recently changed the order precedence by adding the cloud environment supplied security credentials, last.

  2. Second, this does not excuse Spring Cloud Services from stripping out, then throwing away EnumerablePropertySources. This would have happened to any of the core Spring Framework PropertySource objects (e.g. systemProperties or systemEnvironment) or even Spring Boot added PropertySource objects (e.g. ConfigurationPropertySourcesPropertySource) if any of these were "enumerable". It turns out, all custom Spring Boot PropertySource objects were not enumerable (i.e. they did not use nor extend a PropertySource subclass (e.g. PropertiesPropertySource that was an EnumerablePropertySource) and is the sole reason these PropertySource objects were not lost. If any PropertySource gets added to the SCS ExtendedDefaultPropertySource, then they would be discarded from the main Environment. Why does that happen? Why only EnumerablePropertySources?

Having stated the above, my changes are still more appropriate then the changes proposed in this PR.

@dsyer
Copy link
Member Author

dsyer commented Feb 11, 2019

Feel free to ignore this change. You have something that works for you apparently. I was just trying to clarify the analysis.

I’m pretty sure nothing is discarded in Spring Cloud, but if you have a more focused test case that shows only that aspect of your analysis, please open or attach to an issue in spring cloud commons.

@jxblum
Copy link
Contributor

jxblum commented Feb 12, 2019

Thank you @dsyer.

Yep, I continued to work with @spencergibb yesterday in Slack to dig a bit deeper/perform more analysis. I was able to reproduce this problem without SBDG on the classpath, in this little example/test application.

I added a comment with this example/test to SCC Issue-476.

@spencergibb also wrote an example/test that he was able to use to reproduce the problem. He also verified his fix (PR-480) resolved the issue for both his and my example/test as well as SBDG.

I think we are all good.

@wxlund - FYI ^^^^

@jxblum
Copy link
Contributor

jxblum commented Feb 13, 2019

Closing for the time being. May consider these changes at a later time.

@jxblum jxblum closed this Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants