rackspace/runway

[BUG] Crash when providing an int tag directly to a cfngin stack definition

jake-skipper opened this issue · 5 comments

Bug Description

Providing a tag such as CostCenter: 1234 and passing it directly in to a cfngin stack causes a crash due to Boto doing type validation on tag values (and only allowing str). This does not occur when using the top level tags field.

For instance, this works without issues:

namespace: ${namespace}
cfngin_bucket: ""

tags:
  CostCenter: 1234

stacks:
  example-stack:
    template_path: template.yaml

And this will crash with a type error:

namespace: ${namespace}
cfngin_bucket: ""

stacks:
  example-stack:
    template_path: template.yaml
    tags:
      CostCenter: 1234

We should probably determine what type of int to str conversion is happening to the top level tags field and apply the same to the stack level tags field.

In the meantime, a workaround is to quote the offending tag:

namespace: ${namespace}
cfngin_bucket: ""

stacks:
  example-stack:
    template_path: template.yaml
    tags:
      CostCenter: "1234"

Expected Behavior

Tag value handling at the stack level should be the same as the top level tags field.

Steps To Reproduce

Already provided in description.

Runway version

2.6.7

Installation Type

pypi (pip, pipenv, poetry, etc)

OS / Environment

N/A

Anything else?

N/A

Further information needed about error message produced.

I have been able to reproduce the different behavior between defining tags at the root level versus the stack level. They do both fail now after the pydantic conversion.

The root level error is now:

[runway] 1 validation error for CFNgin Config File
tags.CostCenter
  Input should be a valid string [type=string_type, input_value=1234, input_type=int]
    For further information visit https://errors.pydantic.dev/2.8/v/string_type

and the stack level error is:

[runway] Parameter validation failed:
Invalid type for parameter Tags[1].Value, value: 1234, type: <class 'int'>, valid types: <class 'str'>

Even though the errors are different, they do now at least both indicate the same action to be taken, which is use a string value. I think this is sufficient to indicate what needs to be done to address the issue.

Do you agree @jake-skipper ?

Looks good to me. Most of the time I ran into this was during Runway upgrades, and it was very confusing when some would work and others fail. If it fails consistently now (and has a nice readable error) I think that's plenty resolved.

An enhancement might be to convert it to a string rather than have the user worry about it, but I don't know how big that lift is.

This is Jake btw, personal GitHub account lol.

After further discussion, auto-conversion as proposed may make troubleshooting more difficult in certain cases and is best to let this return an error if input is not a string.