add supported properties which are not Terraform default to resource. Fixes #438 #441

ghost · 2020-12-22T19:26:42Z

No description provided.

…ondefaultproperty

docs/pages/bdd-references/given.md

eerkunt · 2020-12-22T20:31:48Z

terraform_compliance/extensions/terraform.py

@@ -134,6 +134,7 @@ def _parse_resources(self):
            actions = change.get('actions', [])
            if actions != ['delete']:
                resource['values'] = change.get('after', {}) # dict_merge(change.get('after', {}), change.get('after_unknown', {}))
+                resource['additional_supported_values'] = change.get('after_unknown')


This will break many other things.

It should be some how reconciled within the "value" key.

FYI: all the integration tests did pass on my end.

What we are really doing is pulling in a list of the properties that do not have any default values by any provider. Their values, if not given in the resource when creating it, can't/or isn't known by Terraform until after the resource is instantiated. I was expecting that if I added it under values it would make more of a problem as it will pull in additional values that won't necessarily be present on the deployed resource. I can try and debug a bit with it under values to confirm. That will be a tomorrow task.

@eerkunt, I just pushed some more tests to try and find possible regression failures in when/then functionality. Would you look over those and see if there is something missing there that is not elsewhere covered?

after re-looking at resource['values'], I really think that is the wrong place to add this data. Since we know only that these are possible values on the object and have no idea about what the actual values will be, it would create false positives or false negatives in the following Then / When conditions (the additional functional tests for regex matching which were added to test for backwards compatibility fail).

If we do end up using a well known section under values (like depicted below), then that will change the context of the values dictionary and we will also need to put logic in place to exclude that well known section for the when/then conditionals to preserve current application logic.

"values": { "allow_major_version_upgrade": null, "backtrack_window": null, "backup_retention_period": 1, "cluster_identifier": "aurora-cluster-demo", "copy_tags_to_snapshot": false, "database_name": "mydb", ... "additional_supported_properties_without_known_values": [ "endpoint", "engine_version", "hosted_zone_id", "id", "kms_key_id", "port", ] }

Maybe changing the name of the resource property would make more sense? Something like:
resource['additional_supported_properties_without_known_values'] and change value of that from an import of the after_unknown Terraform object to just include the keys, in which case it will be a list of the additional properties and not a dictionary?

I think this is a better approach. It will give us many other functionality opportunities. Thanks for the extra expalanation.

The only the thing I am a bit worried is the key name. We need ensure - although its practically impossible - that none of the custom providers will not have this key on their resources. Hence, maybe instead of additional_supported_properties_without_known_values we can change it to something like _terraform_compliance_additional_values etc. What do you think ?

Moving this into "values" will also let us not touching anything on the step functions, like these

If I understood it right, you want:

the key under values? and

changing the name to _terraform_compliance_additional_values ?

I'm good with the name change to _terraform_compliance_additional_values.

Please ignore the rest if I misunderstood on the first point and you are good with the location where it is and not under values.

I may not have explained it quite right. In it's current location, there is no additional need that I see to make changes to other when/then statements. Currently all the integration tests are passing as is. The only place this new list (or dict - whatever we land on) should be used, in my view, is this one given statement where it checks for support.

Since we don't really know the values of any of those properties, what their defaults are, I would think we want all those other when/then statements to be blind to this change and act as they currently do. If we do make the change in location to values, then changes to the other steps will be need to be modified in order not to make changes. The tests that I currently know will change are the below examples. In addition, functional test 'test_issue-279' fails when _terraform_compliance_additional_values is under values and I haven't dug into that one yet to see what the issue may be.

Two example cases which would change functionality when I move _terraform_compliance_additional_values to under values:

when the Terraform config creates the following example resource

resource "aws_rds_cluster" "db_cluster" { cluster_identifier = "aurora-cluster-demo" engine = "aurora-postgresql" database_name = "mydb" master_username = "postgres" master_password = "nothing" }

The following tests have their results changed from current when under values. When the new object is in its current location, or at the same level as values, then the new code will match the current code for these tests:

Scenario: Then it must contain functionality not changed Given I have aws_rds_cluster defined Then it must contain arn Scenario: Then it must not contain functionality not changed Given I have aws_rds_cluster defined Then it must not contain arn Scenario: When it does contain functionality not changed Given I have aws_rds_cluster defined When it contains arn Scenario: When it does not contain functionality not changed Given I have aws_rds_cluster defined When it does not contain arn Scenario: When it has functionality not changed Given I have aws_rds_cluster defined When it has id

Below are outputs of test runs (only relevant snippets of output):

Current code:

Scenario: Then it must contain functionality not changed Given I have aws_rds_cluster defined Failure: aws_rds_cluster.db_cluster (aws_rds_cluster) does not have arn property. Then it must contain arn Failure: Scenario: Then it must not contain functionality not changed Given I have aws_rds_cluster defined Then it must not contain arn Scenario: When it does contain functionality not changed Given I have aws_rds_cluster defined ❗ WARNING: "When it contains arn" step functionality will be changed on future versions and the functionality will be same as "When it has arn" step. Please use the latter. 💡 SKIPPING: Can not find any arn property for aws_rds_cluster resource in terraform plan. 💡 SKIPPING: Skipping the step since resource type does not have arn property. When it contains arn Scenario: When it does not contain functionality not changed Given I have aws_rds_cluster defined When it does not contain arn Scenario: When it has functionality not changed Given I have aws_rds_cluster defined 💡 SKIPPING: Can not find any id property for aws_rds_cluster resource in terraform plan. 💡 SKIPPING: Skipping the step since resource type does not have id property. When it has id

New code with the entry under values:

Scenario: Then it must contain functionality not changed Given I have aws_rds_cluster defined Then it must contain arn Scenario: Then it must not contain functionality not changed Given I have aws_rds_cluster defined Then it must not contain arn Failure: Scenario: When it does contain functionality not changed Given I have aws_rds_cluster defined ❗ WARNING: "When it contains arn" step functionality will be changed on future versions and the functionality will be same as "When it has arn" step. Please use the latter. When it contains arn When it contains arn Scenario: When it does not contain functionality not changed Given I have aws_rds_cluster defined 💡 SKIPPING: All objects (aws_rds_cluster) coming from previous step has arn property. When it does not contain arn Scenario: When it has functionality not changed Given I have aws_rds_cluster defined When it has id

…raform

eerkunt · 2020-12-23T20:53:49Z

terraform_compliance/extensions/terraform.py

@@ -134,6 +134,7 @@ def _parse_resources(self):
            actions = change.get('actions', [])
            if actions != ['delete']:
                resource['values'] = change.get('after', {}) # dict_merge(change.get('after', {}), change.get('after_unknown', {}))
+                resource['additional_supported_values'] = change.get('after_unknown')


I think this is a better approach. It will give us many other functionality opportunities. Thanks for the extra expalanation.

The only the thing I am a bit worried is the key name. We need ensure - although its practically impossible - that none of the custom providers will not have this key on their resources. Hence, maybe instead of additional_supported_properties_without_known_values we can change it to something like _terraform_compliance_additional_values etc. What do you think ?

Moving this into "values" will also let us not touching anything on the step functions, like these

docs/pages/bdd-references/given.md

…ondefaultproperty

…om/sleightsec/terraform-compliance into resourcesupportsnondefaultproperty

ghost · 2021-01-04T21:32:44Z

@eerkunt, just realized I never pushed the attribute name change - pushed along with merging new changes from master.

ghost · 2021-01-08T09:53:34Z

closing. Fix in #445 is superior.

sleightsec added 3 commits December 22, 2020 12:49

Add 'after_unknown' Terraform plan key to resource. Fixes #438

608b7eb

Merge remote-tracking branch 'upstream/master' into resourcesupportsn…

7863c5a

…ondefaultproperty

doc change - supported properties act different if Terraform default

229ed92

eerkunt requested changes Dec 22, 2020

View reviewed changes

sleightsec added 4 commits December 23, 2020 09:03

Added tests for when/then regression

1ce2694

Add more detail in docs about default/non-default properties from Ter…

aafe70e

…raform

change existing unittest to match on new key added

8ddf31a

fix my own functional test, chx new key back to dict from list

782dc6b

eerkunt requested changes Dec 23, 2020

View reviewed changes

sleightsec added 4 commits January 4, 2021 14:24

change name of property per request

3a2dea4

Merge branch 'master' into resourcesupportsnondefaultproperty

eef2328

Merge remote-tracking branch 'upstream/master' into resourcesupportsn…

48f6a35

…ondefaultproperty

Merge branch 'resourcesupportsnondefaultproperty' of https://github.c…

946af76

…om/sleightsec/terraform-compliance into resourcesupportsnondefaultproperty

Kudbettin mentioned this pull request Jan 7, 2021

Fix "resource that supports" to accommodate "after_unknown" values #445

Merged

Merge branch 'master' into resourcesupportsnondefaultproperty

e2be6bb

ghost closed this Jan 8, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add supported properties which are not Terraform default to resource. Fixes #438 #441

add supported properties which are not Terraform default to resource. Fixes #438 #441

ghost commented Dec 22, 2020

eerkunt Dec 22, 2020

ghost Dec 22, 2020

ghost Dec 23, 2020

ghost Dec 23, 2020

eerkunt Dec 23, 2020 •

edited

Loading

ghost Dec 23, 2020 •

edited by ghost

Loading

eerkunt Dec 23, 2020 •

edited

Loading

ghost commented Jan 4, 2021

ghost commented Jan 8, 2021

add supported properties which are not Terraform default to resource. Fixes #438 #441

add supported properties which are not Terraform default to resource. Fixes #438 #441

Conversation

ghost commented Dec 22, 2020

eerkunt Dec 22, 2020

Choose a reason for hiding this comment

ghost Dec 22, 2020

Choose a reason for hiding this comment

ghost Dec 23, 2020

Choose a reason for hiding this comment

ghost Dec 23, 2020

Choose a reason for hiding this comment

eerkunt Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

ghost Dec 23, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

eerkunt Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

ghost commented Jan 4, 2021

ghost commented Jan 8, 2021

eerkunt Dec 23, 2020 •

edited

Loading

ghost Dec 23, 2020 •

edited by ghost

Loading

eerkunt Dec 23, 2020 •

edited

Loading