fix(provider/openstack) - handle operations on stacks that were deplo…

…yed with the previous HEAT template. (#2375)

Address errors on stack operations that return 400 with "Action request failed with fault The Parameter (resource_filename) was not provided."
Significantly improve handling of python-style json strings from the OST response.
Fix an issue where newly-created stacks could not have their instances terminated.

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

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<String> zones
   Map<String, String> schedulerHints
 
+  // This is only used when migrating a stack from a previous version of clouddriver
+  static String resolveResourceFilename(Map<String, String> paramsMap) {
+    return paramsMap.get(ServerGroupConstants.LEGACY_RESOURCE_FILENAME_KEY) ?: ServerGroupConstants.SUBTEMPLATE_FILE
+  }
+  String resourceFilename
+
   static final ObjectMapper objectMapper = new ObjectMapper()
 
   Map<String, String> 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,22 +150,33 @@ class ServerGroupParameters {
    * @return
    */
   static List<String> 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
    *
    * @param string
    * @return
    */
   static Map<String, String> 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
   }
 
   /**

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'
 }

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<String> result = ServerGroupParameters.unescapePythonUnicodeJsonList(input)
 
@@ -57,24 +90,64 @@ 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<String, String> result = ServerGroupParameters.unescapePythonUnicodeJsonMap(input)
 
     then:
     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"
     ]
   }
 

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 }
     }

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<String, Object> 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: