provider/aws: Add normalizeJsonString and validateJsonString functions.

[kwilczynski]

This commit adds ValidateFunc to the policy attribute so that JSON parsing
errors can be caught early. Generally, when there is a ValidateFunc set for the
attribute, one can safely assume that before any of the creation and/or update
of the existing resource would happen it would have to succeed validation. Also
adds support for new helper function which is used to normalise JSON string.

Signed-off-by: Krzysztof Wilczynski [email protected]

[kwilczynski]

Related to #7967.

[kwilczynski]

@stack72 the idea here is to add validation of JSON to catch any potential issues at the planning stag by explicitly notifying user about an error.

At the moment, the normalizeJson function would only return a string, regardless whether it was an error or a genuine value (JSON). This causes an error message originating from the JSON parser to be shown as a value for the attribute, and since the normalizeJson is often used (almost always) as to provide StateFunc, the outcome is a "green" plan (appearing as having no errors), which is expected given what the StateFunc is designed to do - a string with error message is still a string, thus no issues from the StateFunc mechanics point of view.

The changes here are to make the normalizeJson so that it would return an error. This allows for an appropriate ValidateFunc to be introduced, so that errors related to JSON parsing can be caught early in the process and reported accordingly.

Example usage:

(before)

+ aws_s3_bucket.test123
    acl:              "" => "private"
    arn:              "" => "<computed>"
    bucket:           "" => "kwilczynski-test-bucket-123"
    force_destroy:    "" => "false"
    hosted_zone_id:   "" => "<computed>"
    policy:           "" => "Error parsing JSON: invalid character '[' looking for beginning of object key string"
    region:           "" => "<computed>"
    website_domain:   "" => "<computed>"
    website_endpoint: "" => "<computed>"


Plan: 1 to add, 0 to change, 0 to destroy.

(after)

There are warnings and/or errors related to your configuration. Please
fix these before continuing.

Errors:

  * aws_s3_bucket.test123: "policy" contains an invalid JSON: invalid character '[' looking for beginning of object key string

Let me know what do you think.

[apparentlymart]

I think it's best if we return exactly what the user provided (return jsonString.(string), err) here so that normalizeJsonStateFunc will just return the value unchanged when it's invalid.

The reason for this is that StateFunc is called in some situations where ValidateFunc isn't called, such as normalizing a value set from the upstream API during a resource's Read, and so preserving the invalid value will allow it to show in a diff, rather than showing as the empty string.

This is definitely an edge case and mainly a cosmetic thing; I took a similar approach in #8350, which does something similar for normalizing/validating IAM policy JSON.

[kwilczynski]

Makes sense!

[apparentlymart]

Hi @kwilczynski. Thanks for doing this! That behavior of returning the error string as the normalized value has been bugging me for a while now.

This is a bit of an intimidating patch to accept because it touches many resources all at once and so would require running a lot of acceptance tests before merge. Could you possibly split this up in to one commit that adds the necessary shared code, and then a separate commit to update the S3 bucket resource, something like what I did in #8350? Then we can review and merge that smaller increment and attack the rest of these as separate PRs that can each be tested and merged in isolation.

I'm happy to help do the split and re-submit as a new PR if you don't have time. I want to get this in. 😀

[kwilczynski]

@apparentlymart thank you! I did bother me too (the lack of correct error handling), which is why this attempt to fix this.

No issues with splitting the commits, I will get to it.

I had a look at #8350, which is amazing, and it gave me an idea - what if we would to design an interface that would allow for abstracting JSON reading (parsing), writing and validation of us?

The reason why I thought about an interface that could provide a concrete implementation for a specific use case e.g. IAM role, S3 bucket, etc., is because what I did was simply improve upon the normalizeJson function which is being used in few places whereas your change in #8350 goes beyond that and actively concerns itself with how the IAM policy document is represented, which is far superior to simply changing the behaviour of a function (a low-hanging fruit I saw and decided to grab it).

After merging changes from #8350, it would make very little sense to add my changes, which is why I am thinking that perhaps there would be a better way and abstract universal elements out so that we could re-use these.

What do you think?

[apparentlymart]

@kwilczynski it is true that #8350 overlaps with the parts of this change that concern IAM policies. What you did here does seem like it'd be useful for other JSON-ish things that aren't IAM policies, though.

Do you have some thoughts about what a common abstraction (like you propose here) would look like?

We already have one abstraction here in the idea of StateFunc and ValidateFunc being generic across all schemas, and thus we can re-use certain functions like normalizeJson between them. It sounds like you're proposing another layer above that that handles some aspect of the JSON parsing/reserialization. I'm not opposed to such a thing, but I'm not sure I can imagine yet what such a thing would look like.

It would still be nice to get your improvements to normalizeJson and the validation function merged, even if we stop using it for IAM policies after #8350 is merged... what do you think about continuing down the path you were already on for the moment, and then once we have both of these merged we can use a separate issue/PR to discuss any generalizations between the two?

[kwilczynski]

@apparentlymart 👍

I fully agree. I will continue. We can revise and refractor things later, if you want to, exactly like you say. :)

[kwilczynski]

Things to do:

• Provide normalizeJsonString which would improve upon normalizeJson function
• Provide a StateFunc to wrap normalizeJsonString accordingly
• Provide validateJsonFunc to wrap normalizeJsonString and provide feedback early
• Manually review change making sure that the behaviour is as expected for each resource
• Update acceptance tests for reach resource (as needed)
• Run acceptance tests to verify that core functionality is intact for each resource
• Update documentation so that it describes new behaviour (as needed)

[kwilczynski]

Acceptance tests to run:

• Resources affected (AWS):
  • aws_cloudformation_stack
  • aws_cloudwatch_event_rule
  • aws_elasticsearch_domain
  • aws_glacier_vault
  • aws_kms_key
  • aws_s3_bucket
  • aws_s3_bucket_policy
  • aws_sns_topic
  • aws_sns_topic_policy
  • aws_sqs_queue
  • aws_sqs_queue_policy
  • aws_vpc_endpoint
  • opsworks_layers (expands to each of the resources using this file)
    • aws_opsworks_application
    • aws_opsworks_custom_layer
    • aws_opsworks_instance
    • aws_opsworks_permission
    • aws_opsworks_stack
    • aws_opsworks_user_profile
  • data_source_aws_cloudformation_stack
• Resources affected (AzureRM):
  • arm_template_deployment

Results tracked here: https://gist.github.com/kwilczynski/f89e7d1f0cfc4f744a98bdac15567954

[kwilczynski]

The opsworks_layers resources seem to error a lot, but for may reasons. I need to look into these.

[kwilczynski]

Related to #8615, since it removes the normalizeJson function.

[kwilczynski]

@apparentlymart @stack72 if you don't mind, I could use your help. I cannot run the AzureRM test.

[kwilczynski]

I have rebased against master to accommodate recent changes.

[kwilczynski]

After having a conversation with @radeksimko , who was very kind and provided me with an idea about making this more manageable, I will split this commit into a series of Pull Requests. This one will be revised and subsequent Pull Requests will be pushed.

[kwilczynski]

Unit tests are passing:

$ make test TEST=./builtin/providers/aws TESTARGS='-v -run=TestNormalizeJsonString'
==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/14 09:07:36 Generated command/internal_plugin_list.go
TF_ACC= go test ./builtin/providers/aws -v -run=TestNormalizeJsonString -timeout=30s -parallel=4
=== RUN   TestNormalizeJsonString
--- PASS: TestNormalizeJsonString (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    0.016s

$ make test TEST=./builtin/providers/aws TESTARGS='-v -run=TestValidateJsonString'
==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/14 09:09:05 Generated command/internal_plugin_list.go
TF_ACC= go test ./builtin/providers/aws -v -run=TestValidateJsonString -timeout=30s -parallel=4
=== RUN   TestValidateJsonString
--- PASS: TestValidateJsonString (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    0.016s

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.