Regression: expanding default parameters no longer works

[roman-parkhunovskyi]

Tested with 1.4.2 and if you have a default parameter it's substituted neither in expand nor in created stack thus failing. 1.2.1 does not have this issue.

[Noirbot]

Do you mean that, when you have a parameter with a Default value set, and you don't pass in a value, no value is set inside the template?

[roman-parkhunovskyi]

Correct. Default value is just an empty string.

[temujin9]

@rparkhunovsky I cannot reproduce your issue: the default value on my test template gets expanded correctly in 1.4.3 (which is just an unrelated patch away from 1.4.2). Can you try again, and then give me the template you're using if it still fails?

[temujin9]

For reference, this works:

#!/usr/bin/env ruby
require 'bundler/setup'
require 'cloudformation-ruby-dsl/cfntemplate'

tmpl = template do
  @stack_name = "hello-bucket-example"

  parameter "Test", :Type => "String", :Default => "blahblahblahyaddayadda"
  resource "HelloBucket", :Type => "AWS::S3::Bucket", :Properties => { :BucketName => ref("Test") }
end

tmpl.exec!

[roman-parkhunovskyi]

@temujin9 thanks for verifying. I could not reproduce by myself now as well. Let's have it closed .

[roman-parkhunovskyi]

Ok, I was able to reproduce this now. When you do expand there are all Default values in the generated json output. But once you do diff with the current stack you get the following:

-PARAMETER "ElasticsearchDesiredCapacity=3"
-PARAMETER "ElasticsearchInitialCapacity=3"
-PARAMETER "ElasticsearchInstanceType=m3.xlarge"
-PARAMETER "ElasticsearchMaximumCapacity=12"
-PARAMETER "ElasticsearchMinimumCapacity=3"
+PARAMETER "ElasticsearchDesiredCapacity="
+PARAMETER "ElasticsearchInitialCapacity="
+PARAMETER "ElasticsearchInstanceType="
+PARAMETER "ElasticsearchMaximumCapacity="
+PARAMETER "ElasticsearchMinimumCapacity="

However update operation will fails with Error: unable to update immutable parameter 'ElasticsearchInitialCapacity=3' to 'ElasticsearchInitialCapacity='. Versions used:

GEM
  remote: https://rubygems.org/
  specs:
    aws-sdk (2.7.2)
      aws-sdk-resources (= 2.7.2)
    aws-sdk-core (2.7.2)
      aws-sigv4 (~> 1.0)
      jmespath (~> 1.0)
    aws-sdk-resources (2.7.2)
      aws-sdk-core (= 2.7.2)
    aws-sigv4 (1.0.0)
    cloudformation-ruby-dsl (1.4.3)
      aws-sdk (>= 2.5.1)
      bundler
      detabulator
      diffy
      highline
      json
      rake
    detabulator (0.1.0)
    diffy (3.1.0)
    highline (1.7.8)
    jmespath (1.3.1)
    json (2.0.3)
    rake (12.0.0)

PLATFORMS
  ruby

DEPENDENCIES
  aws-sdk
  cloudformation-ruby-dsl

BUNDLED WITH
   1.13.6

[temujin9]

@rparkhunovsky Can you write a minimal example template and commands which demonstrate this problem? That will let us reproduce it, which will help a lot with locating and addressing it.

[roman-parkhunovskyi]

Test template:

#!/usr/bin/env ruby
require 'bundler/setup'
require 'cloudformation-ruby-dsl/cfntemplate'
require 'yaml'

template do
  value :AWSTemplateFormatVersion => '2010-09-09'
  value :Description => 'TEST stack'

  parameter 'AMILinuxVersion',
            :Description   => 'Amazon Linux version.',
            :Type          => 'String',
            :Default       => '2016.09.0'

  parameter 'Team',
            :Description   => 'Team',
            :Type          => 'String',
            :AllowedValues => [ 'teamqa', 'teamprod' ],
            :ConstraintDescription => 'Must be a valid team name.',
            :Default       => 'teamprod'

  parameter 'DeployTag',
            :Description   => 'Tag name to use for puppet configuration',
            :Type          => 'String'

  resource 'LogstashWaitConditionHandle', :Type => 'AWS::CloudFormation::WaitConditionHandle', :Properties => {}

  tag 'team',
      :Value => parameters['Team']
end.exec!

You need a running stack to reproduce the bug. Try diff to see how unspecified parameters and tags are set to empty strings "".

$ ./test-dsl.rb create test-stack --parameters "DeployTag=ddtag1"
$ ./test-dsl.rb diff test-stack --parameters "DeployTag=ddtag3"
====== Tags ======
-TAG "team=teamprod"
+TAG "team="
==================

====== Parameters ======
-PARAMETER "AMILinuxVersion=2016.09.0"
-PARAMETER "DeployTag=ddtag1"
-PARAMETER "Team=teamprod"
+PARAMETER "AMILinuxVersion="
+PARAMETER "DeployTag=ddtag3"
+PARAMETER "Team="
========================

[asadasivan]

@temujin9 I am having the same issue. Not able to use the default values. Any idea when this issue will be fixed?

[thebearmayor]

@temujin9 I think we can now remove the calls to apply_parameter_defaults at https://github.com/bazaarvoice/cloudformation-ruby-dsl/blob/master/lib/cloudformation-ruby-dsl/cfntemplate.rb#L345 and https://github.com/bazaarvoice/cloudformation-ruby-dsl/blob/master/lib/cloudformation-ruby-dsl/cfntemplate.rb#L534.

As it is now, there is an issue when a parameter's value is the empty string. In apply_parameter_defaults, if the value of a parameter is empty, it looks up v.default, which no longer exists after this change.

[thebearmayor]

In fact, I think this change breaks the interaction of :Default and :UsePreviousValue on parameters. I think apply_parameter_defaults exists for update so that defaults do not override previous values. After this change, the default is applied first and the previous value isn't checked.

Since this issue is about diff, I think the right thing to do is to add the same sort of parameter handling done in update to diff.

[temujin9]

Ah, damn. You're right, I didn't adequately test that.

We need some better spec tests.

[thebearmayor]

I agree. I tried to write up a test case for this one this morning, but got stuck because UsePreviousValue pretty much requires an existing stack. I guess you'd need to mock out the API.

Would you mind reverting this change and releasing a 1.4.5? It would save me some work, since I don't currently pin a version, but I'd need to to avoid this release.

[temujin9]

Okay, did it the right way around, instead. Let me know if you see further issues.

[temujin9]

(Also, I've nuked v1.4.4 in Rubygems.)

[roman-parkhunovskyi]

Please, reopen. This is not fixed.

GEM
  remote: https://rubygems.org/
  specs:
    aws-sdk (2.8.7)
      aws-sdk-resources (= 2.8.7)
    aws-sdk-core (2.8.7)
      aws-sigv4 (~> 1.0)
      jmespath (~> 1.0)
    aws-sdk-resources (2.8.7)
      aws-sdk-core (= 2.8.7)
    aws-sigv4 (1.0.0)
    cloudformation-ruby-dsl (1.4.6)
      aws-sdk (>= 2.5.1)
      bundler
      detabulator
      diffy
      highline
      json
      rake
    detabulator (0.1.0)
    diffy (3.2.0)
    highline (1.7.8)
    jmespath (1.3.1)
    json (2.0.3)
    rake (12.0.0)

PLATFORMS
  ruby

DEPENDENCIES
  aws-sdk
  cloudformation-ruby-dsl

BUNDLED WITH
   1.13.6

Here's a pseudo template to reproduce:

#!/usr/bin/env ruby
require 'bundler/setup'
require 'cloudformation-ruby-dsl/cfntemplate'
require 'yaml'

template do
  value :AWSTemplateFormatVersion => '2010-09-09'
  value :Description => 'TEST stack'

  parameter 'AMILinuxVersion',
            :Description   => 'Amazon Linux version.',
            :Type          => 'String',
            :Default       => '2016.09.0'

  parameter 'InstanceType',
            :Description   => 'Amazon instance type.',
            :Type          => 'String',
            :Default       => 't2.small'

  parameter 'Team',
            :Description   => 'Team',
            :Type          => 'String',
            :AllowedValues => [ 'teamqa', 'teamprod' ],
            :ConstraintDescription => 'Must be a valid team name.',
            :Default       => 'teamprod'

  resource 'testresource', :Type => 'Any::Resource', :Version => '1.0', :Properties => {
    :Version      => parameters['AMILinuxVersion'],
    :InstanceType => parameters['InstanceType'],
  }

  tag 'team',
      :Value => parameters['Team']
end.exec!

$./test.rb expand test-stack --parameters "DeployTag=ddtag1" | less

{
  "AWSTemplateFormatVersion": "2010-09-09",
  "Description": "TEST stack",
  "Parameters": {
    "AMILinuxVersion": {
      "Description": "Amazon Linux version.",
      "Type": "String",
      "Default": "2016.09.0"
    },
    "InstanceType": {
      "Description": "Amazon instance type.",
      "Type": "String",
      "Default": "t2.small"
    },
    "Team": {
      "Description": "Team",
      "Type": "String",
      "AllowedValues": [
        "teamqa",
        "teamprod"
      ],
      "ConstraintDescription": "Must be a valid team name.",
      "Default": "teamprod"
    }
  },
  "Resources": {
    "testresource": {
      "Type": "Any::Resource",
      "Version": "1.0",
      "Properties": {
        "Version": "",
        "InstanceType": ""
      }
    }
  }
}

Empty parameter values still there.

[thebearmayor]

Based on that comment, I would clarify the issue as: defaults are not set in the parameters hash. That is caused by the addition of :UsePreviousValue and moving defaults out of the parameter method. I'm trying to put together a branch which sets the default in parameter, and sets a is_default flag, so later comparisons with previous values can know whether to override. But it's pretty slow going because of all the different possibilities, and different ways of handling parameters depending on the action.

[thebearmayor]

I've tried and failed to fix this. Here's a partially working branch: https://github.com/thebearmayor/cloudformation-ruby-dsl/tree/110. I set defaults in the parameter function, and then overwrite them later. It still doesn't handle tags correctly.

I'm not very familiar with the code, so take this with a grain of salt, but the problem I see is that UsePreviousValue violates assumptions about when parameters will be known. The template is evaluated early and expects to have all its parameters set via commandline, interactive, or defaults. Changing the parameters later with UsePreviousValue, after fetching them from AWS, would require re-evaluating the template. But even that is tricky because the template expects parameters to come from the commandline.

Another internal team hit this issue yesterday after inadvertently upgrading cloudformation-ruby-dsl. My team uses UsePreviousValue, but we'd give it up in favor of defaults working with the parameters array.