Cloud config client reversing remote property sources with certain settings

[jeffbswope]

There seems to be a regression in Hoxton.SR1 (spring-cloud-context:2.2.1.RELEASE) which results in the property sources returned by the cloud config server being reversed on the client when settings dictate to position them relative to the system properties source.

This code incorrectly reverses the order of the property sources in the original incoming composite:

https://github.com/spring-cloud/spring-cloud-commons/blob/master/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java#L196-L198

	for (PropertySource<?> p : composite) {
		propertySources.addAfter(SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, p);
	}

As does this code:

https://github.com/spring-cloud/spring-cloud-commons/blob/master/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java#L201-L203

	for (PropertySource<?> p : reversedComposite) {
		propertySources.addBefore(SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, p);
	}

(The former should be iterating on the reversedComposite so that .addAfter unfurls them in their original order, and the latter vice-versa.)

The first block is hit when the following settings exist:

spring.cloud.config.allow-override=true
spring.cloud.config.override-none=false
spring.cloud.config.override-system-properties=false

As an aside, the second block on line 201-203 appears to be dead code? If remoteProperties.isOverrideSystemProperties() then we seem to return on line 186 if !remoteProperties.isOverrideNone() and line 192 if remoteProperties.isOverrideNone(), assuming those are non-volitile. I'm not sure what assumptions are being made about which sources might be in the environment when this is hit, but it seems like "not first, not last, but before system properties" and "first" are treated as equivalent on 181-182 such that lines 201-203 aren't needed. This might just be dodging some complexity around systemProperties vs. systemEnvironment and fallback when one or both are missing?

As requested on Gitter, a heavily pruned/redacted result of /actuator/env on a client with these settings in Hoxton.SR1, where the bootstrapProperties-ssh://... sources are in the opposite order as returned from the config server at config-server:8888/my-app-name/foo:

{
    "activeProfiles": [
        "foo"
    ],
    "propertySources": [
        {
            "name": "server.ports",
            "properties": {
                "local.server.port": {
                    "value": 18880
                }
            }
        },
        {
            "name": "servletContextInitParams",
            "properties": {}
        },
        {
            "name": "systemProperties",
            "properties": {
            }
        },
        {
            "name": "systemEnvironment",
            "properties": {
            }
        },
        {
            "name": "bootstrapProperties-ssh://[email protected]:7999/prop-repo.git/application.properties",
            "properties": {
            }
        },
        {
            "name": "bootstrapProperties-ssh://[email protected]:7999/prop-repo.git/my-app-name.properties",
            "properties": {
            }
        },
        {
            "name": "bootstrapProperties-ssh://[email protected]:7999/prop-repo.git/application-foo.properties",
            "properties": {
            }
        },
        {
            "name": "bootstrapProperties-ssh://[email protected]:7999/prop-repo.git/my-app-name-foo.properties",
            "properties": {
            }
        },
        {
            "name": "bootstrapProperties-configClient",
            "properties": {
                "config.client.version": {
                    "value": "97d53a32cf62c34bcc4eb21fa194b57f00a9eb66"
                }
            }
        },
        {
            "name": "springCloudClientHostInfo",
            "properties": {
            }
        },
        {
            "name": "applicationConfig: [file:version.properties]",
            "properties": {
            }
        },
        {
            "name": "applicationConfig: [classpath:/application.properties]",
            "properties": {
            }
        },
        {
            "name": "springCloudDefaultProperties",
            "properties": {}
        }
    ]
}

[spencergibb]

I'm unable to replicate the problem. Can you please provide the configuration and build files for the client and server (if not a complete, minimal, verifiable sample that reproduces the problem? It should be available as a GitHub (or similar) project or attached to this issue as a zip file.)

[pcornelissen]

https://github.com/pcornelissen/spring-cloud-bug-668 shows that the error occurs in SR1 but not in the release version

[spencergibb]

I don't think these are related, but I could be wrong.

[spencergibb]

After adding boot web and actuator I see the reversed property sources as mentioned above.

[spencergibb]

Indeed, swapping the collections iterated over does fix the problem. Now to write a test.

[spencergibb]

FYI PR with test #670

[spencergibb]

@jeffbswope and @pcornelissen after snapshots are built would you test?

[jeffbswope]

@spencergibb Confirmed with spring-cloud-commons.version=2.2.2.BUILD-SNAPSHOT the properties are back into the correct order. Thanks!

[pcornelissen]

FYI: in hoxton.SR2 the config client version is:

     <spring-cloud-config.version>2.2.1.RELEASE</spring-cloud-config.version>

So the bug is not fixed in SR2!

[spencergibb]

Ah yes, it will be in sr3 in a week

[dyroberts]

@spencergibb This bug also seems to appear in Greenwich.SR5.

[MrMarkW]

@spencergibb I hit this issue with Greenwich.SR5 also with spring-cloud-commons 2.1.5.RELEASE. Will you guys be fixing the issues in Greenwich for this and #704 ?

[spencergibb]

@maximusfloydus likely. Greenwich has entered a special maintenance mode. Likely what will happen is that there would be a commons 2.1.6.RELEASE without a Greenwich.SR6

[spencergibb]

I've created #712 to track this.