diff --git a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy index 3dcadf8cdd7..c418243ec90 100644 --- a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy +++ b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy @@ -129,7 +129,7 @@ class OpenstackOrchestrationV1Provider implements OpenstackOrchestrationProvider if (parts && parts.size() > 2) { String instanceResourceId = parts.get(2) resources?.find { - it.type == stack.parameters.get(ServerGroupConstants.SUBTEMPLATE_FILENAME) && it.resourceName == instanceResourceId + it.type == ServerGroupParameters.resolveResourceFilename(stack.parameters) && it.resourceName == instanceResourceId } } else { null diff --git a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParameters.groovy b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParameters.groovy index 8160b6499d5..dd8f858e752 100644 --- a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParameters.groovy +++ b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParameters.groovy @@ -18,7 +18,6 @@ package com.netflix.spinnaker.clouddriver.openstack.deploy.description.servergro import com.fasterxml.jackson.annotation.JsonCreator import com.fasterxml.jackson.databind.ObjectMapper -import com.google.common.base.Splitter import com.netflix.spinnaker.clouddriver.openstack.deploy.ops.servergroup.ServerGroupConstants import groovy.transform.AutoClone import groovy.transform.Canonical @@ -54,6 +53,12 @@ class ServerGroupParameters { List zones Map schedulerHints + // This is only used when migrating a stack from a previous version of clouddriver + static String resolveResourceFilename(Map paramsMap) { + return paramsMap.get(ServerGroupConstants.LEGACY_RESOURCE_FILENAME_KEY) ?: ServerGroupConstants.SUBTEMPLATE_FILE + } + String resourceFilename + static final ObjectMapper objectMapper = new ObjectMapper() Map toParamsMap() { @@ -80,12 +85,24 @@ class ServerGroupParameters { source_user_data : sourceUserData ?: null, tags : objectMapper.writeValueAsString(tags ?: [:]) ?: null, user_data : rawUserData ?: null, - zones : zones?.join(',') ?: null, - scheduler_hints : objectMapper.writeValueAsString(schedulerHints ?: [:]) ?: null, ] if (floatingNetworkId) { params << [floating_network_id: floatingNetworkId] } + + // This is only used when migrating a stack from a previous version of clouddriver + if (resourceFilename) { + params << [resource_filename: resourceFilename] + } + + // These are new properties. We include them conditionally so as not to mess up resize operations on older, pre-existing stacks. + if (zones) { + params << [zones: zones.join(',')] + } + if (schedulerHints) { + params << [scheduler_hints: objectMapper.writeValueAsString(schedulerHints ?: [:])] + } + params } @@ -118,8 +135,9 @@ class ServerGroupParameters { tags: unescapePythonUnicodeJsonMap(params.get('tags') ?: '{}'), sourceUserDataType: params.get('source_user_data_type'), sourceUserData: params.get('source_user_data'), - zones: unescapePythonUnicodeJsonList(params.get('zones')), + zones: unescapePythonUnicodeJsonList(params.get('zones') ), schedulerHints: unescapePythonUnicodeJsonMap(params.get('scheduler_hints') ?: '{}'), + resourceFilename: params.get('resource_filename') ) } @@ -132,13 +150,14 @@ class ServerGroupParameters { * @return */ static List unescapePythonUnicodeJsonList(String string) { - string?.split(",")?.collect { s -> + List result = string?.split(",")?.collect { s -> s.replace("u'", "").replace("'", "").replace("[", "").replace("]", "").replaceAll("([ ][ ]*)", "") } ?: [] + return result } /** - * Stack parameters of type 'comma_delimited_list' come back as a unicode json string. We need to split that up. + * Some stack parameters of type 'json' come back as a unicode json string. We need to split that up. * * TODO See https://bugs.launchpad.net/heat/+bug/1613415 * @@ -146,8 +165,18 @@ class ServerGroupParameters { * @return */ static Map unescapePythonUnicodeJsonMap(String string) { - String parsed = string?.replace("u'", "")?.replace("'", "")?.replace("{", "")?.replace("}", "")?.replace("\"", "")?.replaceAll("([ ][ ]*)", "") - parsed ? Splitter.on(",").withKeyValueSeparator(":").split(parsed) : [:] + String parsed = string + ?.replaceAll(':\\p{javaWhitespace}*None\\p{javaWhitespace}*([,}])', ': null$1') // first replace python None with json null + ?.replaceAll("u'(.*?)'", '"$1"') // replace u'python strings' with "python strings" (actually json strings) + ?.replaceAll('u"(.*?\'.*?)"', '"$1"') // replace u"python strings containing a ' char" with "python strings containing a ' char" (actually json) + def m = objectMapper.readValue(parsed, Map) + def result = m.collectEntries { k, v -> + if (v instanceof Collection || v instanceof Map) { + return [(k): objectMapper.writeValueAsString(v)] + } + [(k): v] + } + return result } /** diff --git a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/ServerGroupConstants.groovy b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/ServerGroupConstants.groovy index 45405bb369c..4596a332878 100644 --- a/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/ServerGroupConstants.groovy +++ b/clouddriver-openstack/src/main/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/ServerGroupConstants.groovy @@ -21,31 +21,25 @@ class ServerGroupConstants { public static final String HEAT_ASG_RESOURCE = "OS::Heat::AutoScalingGroup" final static String SERVERGROUP_RESOURCE_NAME = 'servergroup' - final static String SUBTEMPLATE_FILENAME = 'resource_filename' - final static String SUBTEMPLATE_SERVER_OUTPUT = 'servergroup_server' - final static String SUBTEMPLATE_SERVER_OUTPUT_FLOAT = 'servergroup_server_float' final static String SUBTEMPLATE_OUTPUT = 'servergroup_resource' - final static String SUBTEMPLATE_OUTPUT_FLOAT = 'servergroup_resource_float' final static String MEMBERTEMPLATE_OUTPUT = 'servergroup_resource_member' //this is the file name of the heat template used to create the auto scaling group, //and needs to be loaded into memory as a String final static String TEMPLATE_FILE = 'servergroup.yaml' - final static String TEMPLATE_FILE_FLOAT = 'servergroup_float.yaml' //this is the name of the subtemplate referenced by the template, //and needs to be loaded into memory as a String - final static String SUBTEMPLATE_SERVER_FILE = "${SUBTEMPLATE_SERVER_OUTPUT}.yaml" - //with floating ip for each instance - final static String SUBTEMPLATE_SERVER_FILE_FLOAT = "${SUBTEMPLATE_SERVER_OUTPUT_FLOAT}.yaml" + final static String SUBTEMPLATE_SERVER_FILE = "servergroup_server.yaml" //this is the name of the subtemplate referenced by the template, //and needs to be loaded into memory as a String final static String SUBTEMPLATE_FILE = "${SUBTEMPLATE_OUTPUT}.yaml".toString() - //with floating ip for each instance - final static String SUBTEMPLATE_FILE_FLOAT = "${SUBTEMPLATE_OUTPUT_FLOAT}.yaml".toString() //this is the name of the member template referenced by the subtemplate, //and is contructed on the fly final static String MEMBERTEMPLATE_FILE = "${MEMBERTEMPLATE_OUTPUT}.yaml".toString() + + // Only used for backward compatibility when interacting with heat stacks from before clouddriver v1.772.0 + final static String LEGACY_RESOURCE_FILENAME_KEY = 'resource_filename' } diff --git a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParametersSpec.groovy b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParametersSpec.groovy index 336e1e9fb98..f6ad31ac399 100644 --- a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParametersSpec.groovy +++ b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/description/servergroup/ServerGroupParametersSpec.groovy @@ -16,26 +16,58 @@ package com.netflix.spinnaker.clouddriver.openstack.deploy.description.servergroup +import com.netflix.spinnaker.clouddriver.openstack.deploy.ops.servergroup.ServerGroupConstants import spock.lang.Ignore import spock.lang.Specification +import spock.lang.Unroll class ServerGroupParametersSpec extends Specification { - def "test toParamsMap"() { + @Unroll + def "resolves resource filename: #description"() { + expect: + ServerGroupParameters.resolveResourceFilename(params) == expected + + where: + description | params | expected + 'from stack params if present' | ['resource_filename': 'valueinparam'] | 'valueinparam' + 'from constants if not in stack params' | [:] | ServerGroupConstants.SUBTEMPLATE_FILE + } + + def "converts params object to map"() { given: ServerGroupParameters params = createServerGroupParams() Map expected = getMap() + when: + Map result = params.toParamsMap() + + then: + //need to compare string values due to some map formatting issue + result.sort{ a, b -> a.key <=> b.key }.toString() == expected.sort{ a, b -> a.key <=> b.key }.toString() + } + + @Unroll + def "toParamsMap - output excludes #mapKey when null in the input"() { + given: + ServerGroupParameters params = createServerGroupParams() + params[serverGroupProperty] = null when: Map result = params.toParamsMap() then: //need to compare string values due to some map formatting issue - result.toString() == expected.toString() + ! result.containsKey(mapKey) + + where: + description | mapKey | serverGroupProperty + "no zones present" | "zones" | "zones" + "no scheduler hints present" | "scheduler_hints" | "schedulerHints" + "no resource filename present" | "resource_filename" | "resourceFilename" } - def "test fromParamsMap"() { + def "converts map to params object"() { given: ServerGroupParameters expected = createServerGroupParams() Map params = getMap() @@ -47,7 +79,8 @@ class ServerGroupParametersSpec extends Specification { result == expected } - def "test handle unicode list"() { + @Unroll + def "converts unicode list to list: #input"() { when: List result = ServerGroupParameters.unescapePythonUnicodeJsonList(input) @@ -57,13 +90,25 @@ class ServerGroupParametersSpec extends Specification { where: input | expected 'test' | ['test'] + 'test, test2' | ['test', 'test2'] '[test]' | ['test'] '[u\'test\']' | ['test'] 'u\'test\'' | ['test'] '[u\'test\',u\'test\',u\'test\']' | ['test', 'test', 'test'] + "[u\'\',u'']" | ["", ""] +// Is this really how it should behave? + null | [] + "[]" | [""] +// Shouldn't these work, too? +// '["test"]' | ["test"] +// '["test", "test"]' | ["test", "test"] } - def "test handle unicode map"() { + @Unroll + def "converts #inType to map: #description"() { + // Older versions of OpenStack return python-encoded dictionaries for several fields. Newer ones have converted them + // to JSON. For forward and backward compatibility, we need to handle either. + when: Map result = ServerGroupParameters.unescapePythonUnicodeJsonMap(input) @@ -71,10 +116,38 @@ class ServerGroupParametersSpec extends Specification { result == expected where: - input | expected - '{"test":"test"}' | ['test': 'test'] - '{u\'test\':\'test\'}' | ['test': 'test'] - '{u\'test\':u\'test\',u\'a\':u\'a\'}' | ['test': 'test', 'a': 'a'] + inType | description | input | expected + 'python' | 'one entry' | '{u\'test\':u\'test\'}' | ['test': 'test'] + 'python' | 'multiple entries' | '{u\'test\':u\'test\',u\'a\':u\'a\'}' | ['test': 'test', 'a': 'a'] + 'python' | 'spaces in value' | '{u\'test\': u\'this is a string\'}' | ['test': "this is a string"] + 'python' | 'comma in value' | '{u\'test\':u\'test1,test2\'}' | ['test': 'test1,test2'] + 'python' | 'colon in value' | '{u\'url\':u\'http://localhost:8080\'}' | ['url': 'http://localhost:8080'] + 'python' | 'value is Empty' | '{u\'test\':u\'\'}' | ['test': ''] + 'python' | 'value is None' | '{u\'test\': None}' | ['test': null] + 'python' | 'multiple None values' | "{u'test': \t None \t \n, u'test2': None\n}" | ['test': null, 'test2': null] + 'python' | 'string contains "None"' | '{u\'test\': u\'And None Either\'}' | ['test': "And None Either"] + 'python' | 'integer value' | '{u\'port\': 1337}' | ['port': 1337] + 'python' | 'single quotes in value' | '{u\'test\': u"\'this is a string\'"}' | ['test': "'this is a string'"] + 'python' | '1 single quote in value' | '{u\'test\': u"Surf\'s up!"}' | ['test': "Surf\'s up!"] + 'python' | 'string ends in "u"' | '{u\'SuperValu\':u\'test\',u\'a\':u\'a\'}' | ['SuperValu': 'test', 'a': 'a'] + 'python' | 'json object in value' | '{u\'health\':{u\'http\':u\'http://lh:80\',u\'a\':u\'b\'}}' | ['health': '{"http":"http://lh:80","a":"b"}'] + 'python' | 'layers of objects' | '{u\'a\': {u\'b\': {u\'c\': u\'d\', u\'e\': u\'f\'}}}' | ['a': '{"b":{"c":"d","e":"f"}}'] + 'json' | 'empty map' | '{}' | [:] + 'json' | 'one entry' | '{"test":"test"}' | ['test': 'test'] + 'json' | 'multiple entries' | '{"test":"test","a": "a"}' | ['test': 'test', 'a': 'a'] + 'json' | 'spaces in value' | '{"test": "this is a string"}' | ['test': "this is a string"] + 'json' | 'comma in value' | '{"test":"test1,test2"}' | ['test': 'test1,test2'] + 'json' | 'colon in value' | '{"url":"http://lh:80"}' | ['url': 'http://lh:80'] + 'json' | 'value is Empty' | '{"test": ""}' | ['test': ''] + 'json' | 'value is null' | '{"test": null}' | ['test': null] + 'json' | 'multiple null values' | '{"test": \t null \t, "test2":\tnull\n}' | ['test': null, 'test2': null] + 'json' | 'string contains "None"' | '{"test": "And None Either"}' | ['test': "And None Either"] + 'json' | 'integer value' | '{"port": 1337}' | ['port': 1337] + 'json' | 'single quotes in value' | '{"test": "\'this is a string\'"}' | ['test': "'this is a string'"] + 'json' | '1 single quote in value' | '{"test": "Surf\'s up!"}' | ['test': "Surf\'s up!"] + 'json' | 'string ends in "u"' | '{"SuperValu":"test","a":"a"}' | ['SuperValu': 'test', 'a': 'a'] + 'json' | 'json object in value' | '{"health":{"http":"http://lh:80", "a": "b"}}' | ['health': '{"http":"http://lh:80","a":"b"}'] + 'json' | 'layers of objects' | '{"a": {"b": {"c": "d", "e": "f"}}}' | ['a': '{"b":{"c":"d","e":"f"}}'] } @Ignore @@ -96,7 +169,8 @@ class ServerGroupParametersSpec extends Specification { sourceUserDataType: 'Text', sourceUserData: 'echo foobar', zones: ["az1", "az2"], - schedulerHints: ["key": "value"]) + schedulerHints: ["key": "value"], + resourceFilename: "fileMcFileface") } @Ignore @@ -124,7 +198,8 @@ class ServerGroupParametersSpec extends Specification { tags: '{"foo":"bar"}', user_data: "echo foobar", zones: "az1,az2", - scheduler_hints: '{"key":"value"}' + scheduler_hints: '{"key":"value"}', + resource_filename: "fileMcFileface" ] } diff --git a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/AbstractStackUpdateOpenstackAtomicOperationSpec.groovy b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/AbstractStackUpdateOpenstackAtomicOperationSpec.groovy index ebd029f0207..d79af09e90b 100644 --- a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/AbstractStackUpdateOpenstackAtomicOperationSpec.groovy +++ b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/servergroup/AbstractStackUpdateOpenstackAtomicOperationSpec.groovy @@ -60,7 +60,7 @@ class AbstractStackUpdateOpenstackAtomicOperationSpec extends Specification { stack = Mock(Stack) { it.id >> { serverGroupName } it.name >> { serverGroupName } - it.parameters >> { [(ServerGroupConstants.SUBTEMPLATE_FILENAME):'asg_resource.yaml'] } + it.parameters >> { [:] } it.outputs >> { [[output_key: ServerGroupConstants.SUBTEMPLATE_OUTPUT, output_value: yaml], [output_key: ServerGroupConstants.MEMBERTEMPLATE_OUTPUT, output_value: yaml]] } it.tags >> { tags } } diff --git a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/provider/agent/OpenstackServerGroupCachingAgentSpec.groovy b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/provider/agent/OpenstackServerGroupCachingAgentSpec.groovy index a14b63031f9..087d3865fb1 100644 --- a/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/provider/agent/OpenstackServerGroupCachingAgentSpec.groovy +++ b/clouddriver-openstack/src/test/groovy/com/netflix/spinnaker/clouddriver/openstack/provider/agent/OpenstackServerGroupCachingAgentSpec.groovy @@ -218,7 +218,7 @@ class OpenstackServerGroupCachingAgentSpec extends Specification { } } Stack stackDetail = Mock(Stack) { getParameters() >> ['load_balancers': loadBalancerId] } - OpenstackServerGroup openstackServerGroup = OpenstackServerGroup.builder().account(account).name(serverGroupName).build() + OpenstackServerGroup openstackServerGroup = OpenstackServerGroup.builder().account(account).name(serverGroupName).tags([foo:'the bar', port:'5050']).build() Map serverGroupAttributes = objectMapper.convertValue(openstackServerGroup, OpenstackInfrastructureProvider.ATTRIBUTES) CacheResultBuilder cacheResultBuilder = new CacheResultBuilder() @@ -297,7 +297,7 @@ class OpenstackServerGroupCachingAgentSpec extends Specification { String subnetId = UUID.randomUUID().toString() Stack stack = Mock(Stack) { it.name >> { 'name' } - it.parameters >> { [subnet_id: subnetId, tags:'{foo: bar}'] } + it.parameters >> { [subnet_id: subnetId, tags:'{"foo": "the bar","port":"5050"}'] } it.creationTime >> { DateTimeFormatter.ISO_LOCAL_DATE_TIME.format(createdTime) } } @@ -322,7 +322,7 @@ class OpenstackServerGroupCachingAgentSpec extends Specification { .buildInfo(buildInfo) .disabled(disabled) .subnetId(subnetId) - .tags([foo:'bar']) + .tags([foo:'the bar', port:'5050']) .build() when: