Default param to use_previous_value if omitted

[iAnomaly]

Updating an existing stack with optional parameters (ones that have a default value in the template) can have disastrous effects if the user forgets to include the existing optional parameter's current value.

Consider a persistent data store such as a SQL database that was created using a Snapshot parameter whose value is an exiting snapshot id. That parameter's default value is an empty string that results in no snapshot being used by the resource in the template if no value is passed in. If the user forgetfully omits or accidentally mangles the Snapshot parameter value, this will result in the recreation of the data store resource and subsequent deletion of the previous one. This would be catastrophic in a production environment.

Because of this I have changed the default parameter behavior of the update action to reuse the parameter's previous value if a parameter is omitted.

I am generally reluctant to change default behavior due to implications of breaking existing compatibility but this feels like a much safer default to me while still allowing users to pass any parameter value explicitly.

[jonaf]

Hmmm... I can see the value of this change. Here's how I deal with this situation. Let me know if you think any of these is good enough, or if you feel this change is still necessary.

1. Do a diff against the stack I want to update first. This will show me differences in parameters. Only the parameters I want to change should be there; if others appear, then I know not to proceed.
2. Set the :Immutable => true property of the parameter. This will cause the update stack call to fail if the existing value differs from the provided value. Just set this property on those sensitive values.
3. Don't set defaults for sensitive parameters. Especially, don't set defaults to empty strings or zeroes. In general, I think this is a bad idea, but it is especially bad for parameters that can produce destructive changes.

That said, I do think that it would be valuable to add a :UsePreviousValue property to parameters in general, and/or support that option on the command line.

[iAnomaly]

I have some opinions on these.

1. That's great and this should be the normal flow for any user of cloudformation-ruby-dsl, however my fix is specifically for the case where a user forgets to be safe and would shoot themselves in the foot.
2. I considered this but what happens when you do want to change a sensitive value such as a Snapshot or MasterUsernamePassword? Remove :Immutable => true, add a AWS::CloudFormation::WaitConditionHandle resource, update the stack, replace :Immutable => true and remove or rename AWS::CloudFormation::WaitConditionHandle one last time? Feels like too much effort to me.
3. I agree this would be the ideal practice, but I couldn't come up with a way to omit certain resource properties conditionally on input. For example, if Snapshot parameter isn't provided I want the template to exclude the DBSnapshotIdentifier property with some logic like this:

DBSnapshotIdentifier:
 fn_if('SnapshotIsBlank',
       # Unlike a blank string, this effectively deletes the
       # attribute
       ref('AWS::NoValue'),
       ref('Snapshot')
 ),

Same goes for MasterUsername and MasterUserPassword. I can imagine a desirable security scenario where the user running the stack update shouldn't actually know the MasterUserPassword How have you worked around this pattern specifically?

[jonaf]

In the case of 2, I don't think you need the WaitConditionHandle...but yes, you would have to modify the stack and remove the immutability of the parameter to update it. This is intentionally annoying, so that you have to think twice before updating such a parameter.

As for 3, you should be able to do this with ruby instead of using intrinsics. Using parameters['DBSnapshotIdentifier'] as the value to check, if it's not supplied, don't add that property to the resource. In general, the sooner you can get "real" values, the more testable your template becomes.

In any case, I think that it would be a good idea to add a :UsePreviousValue property to parameters and/or the command line. I don't think it would be a good idea to make this the default behavior, though.

[iAnomaly]

Alright, thanks @jonaf.

I'll move my conditional property definitions to the DSL layer instead of using the AWS::NoValue intrinsic.

I would think : UsePreviousValue should be mutually exclusive to :Default and the DSL should enforce that but can you imagine a scenario where you might use both?

[jonaf]

I'm on the fence about whether :UsePreviousValue should be in the template, or only supplied as a command line option. On the one hand, I prefer having everything in source control. On the other hand, I could see it being just as dangerous as having a default. That said, we already have a :Default, so it wouldn't be worse, at least.

I don't think the options are completely mutually exclusive, because you may specify a value (despite its default being some other value) and then want to use the previous value if one exists, otherwise use the default value. So the previous value would override the default value, but only in cases where a previous value already exists. Since we currently already check for existing values, I don't see a problem with doing that. My main concern is that it may be confusing, but I guess you should think twice about it before you try putting both properties on your parameter... This use-case strengthens the argument that it should be a property and not a command line option, though.

[jonaf]

One more thought: we could also default the update behavior to, in addition to comparing parameters and tags as it does now, refuse to actually effect the update if any changes are detected, unless a --override-parameters option is supplied. This more-or-less offers a default safety mechanism to prevent updating a stack with parameter values differing from the stack's current parameter values, but does add the complication that some use-cases will have to start adding this option very frequently (perhaps always), so I'm not ecstatic about changing the default behavior here, even if it does mean the default behavior can sometimes be considered unsafe.

[iAnomaly]

Alright. Based on all of your feedback I proceeded with the :UsePreviousValue extended parameter attribute/property approach (like :Immutable).

This required some refactoring for when we assign the :Default value to a parameter. It had to move out of dsl.rb and into cfntemplate.rb to facilitate logic around whether the user is explicitly passing a parameter on the command-line or whether the :Default should be used. The behavior is such that the previous value is used if it exists otherwise :Default is used if it exists.

:UsePreviousValue preempts/short-circuits :Immutable. That is to say, if the user elects to use both :Immutable and :UsePreviousValue on a parameter, the :Immutable behavior will not fail to update so long as the previous value is being used (since nothing is changing). If the user explicitly tries to change the value, then :Immutable will come into play and prevent that from occurring. In this way :Immutable continues to function as before (backwards compatible) but it also allows for scenarios where you can chain :UsePreviousValue so you don't have to include it on every update (think Master Username and Password for RDS) but also allows you to use :Immutable if you want another layer of security.

[iAnomaly]

I realize now that changing parameters' value to a hash instead of a single value is going to have breaking impact for downstream uses of parameters['param']. I'll need to rethink this a bit. Bottom line though we can't assign default or user provided values at the dsl.rb level without a way to determine:

• If a parameter value was set by the user or is using the default value
• What the default value is if the parameter was not set by the user

Maybe create our own parameter type/class or use the AWS SDK one such that we have default and use_previous_value attributes/functions of the object without changing the value returned by parameters['myparam'].

Thoughts?

[iAnomaly]

@jonaf, I finally got around to cleaning this up. I've implemented a custom Parameter class in the DSL that extends Ruby's String. In this way I have added two utility attributes to the class for the default value and the use_previous_value boolean of a parameter (if they are used as parameter attributes in a template).

These changes are well encapsulated within dsl.rb and cfntemplate.rb to ensure any existing downstream use of parameters['MyParam'] still act as a String value and don't break things once merged.

The template's excise_parameter_attribute method was also updated to support multiple parameter attributes in one call (since now we have :UsePreviousValue in addition to :Immutable). I'm doing this by passing an array now but please let me know if you prefer a variable length arguments using args.length within the method instead.

:Immutable and :UsePreviousValue can be combined on a parameter such that:

1. If an :Immutable parameter with :UsePreviousValue is omitted on an update or diff, the previous value is used.
2. If the parameter is set to a new value, it will still fail because of the existing :Immutable behavior.

Please let me know if there are any questions or concerns.

[temujin9]

This has been merged and released as 1.4.0.

[iAnomaly]

Thanks @temujin9! I found an issue with the merge though that's breaking things. I'll leave a comment in the commit.