Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential panics due to nested block contains all optional sub-properties #11452

Merged
merged 8 commits into from
Apr 25, 2021

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Apr 23, 2021

For schemas where a non-computed nested block property of TypeList that only contains optional-only and non-default child properties, which has no AtLeastOneOf/ExactlyOneOf constraint. Then after getting this parent schema from schema.ResourceData, it might be a []interface{} with one nil element, which corresponds to the case that users has specify an empty block, like:

foo {}

I've created a linter rule to scan all the schemas that is a composite literal, and found ~100 schemas that need special attention. Otherwise, the use case above will cause panic as reported in #11422.

The fix in this PR follows following rules:

  1. If the children properties are addressable (i.e. all its parent properties (if any) are set MaxItem to 1), then these Optional properties are set the AtLeastOneOf constraint to invalidate the empty block use case.
  2. Otherwise, either if there is only one child property, or the child properties are not addressable: I will add a lint rule comment and ensure that the expand code for the parent schema does nil check. For this case, especially when there are multiple child properties, I'll further ensure there is no pre-size for the expanded arraies. Because we will continue the loop if the element under iteration is nil.

Note that there are also some data source which has set the property as Optional by mistake, which should be Computed actually. I've fixed those, too.

The lint rule comment is used for my local run, to ensure there are no more warnings. Once bflad/tfproviderlint#236 and bflad/tfproviderlint#237 are merged, then these comments can also benefit our code base, if we enable the rule XS003.

The last thing worth mentioning is that even we resolved all the lint warnings from the XS003, there are still cases that missed, including those schema definitions which are not pure composite literal.

@ghost ghost added the size/XL label Apr 23, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @magodo! LGTM 👍

@katbyte
Copy link
Collaborator

katbyte commented Apr 23, 2021

good to merge once test failure is fixed:

=== RUN   TestProvider
    provider_test.go:13: err: 3 errors occurred:
        	* AtLeastOneOf: delete_os_disk_on_deletion configuration block reference (features.0.virtual_machine.0.delete_os_disk_on_deletion) can only be used with TypeList and MaxItems: 1 configuration blocks
        	* resource azurerm_kubernetes_cluster: AtLeastOneOf: admin_group_object_ids references unknown attribute (role_based_access_control.0.azure_activity_directory.0.client_app_id) at part (azure_activity_directory)
        	* resource azurerm_application_gateway: AtLeastOneOf: policy_name configuration block reference (ssl_policy.0.disabled_protocols) can only be used with TypeList and MaxItems: 1 configuration blocks
        
--- FAIL: TestProvider (0.22s)

@katbyte katbyte added this to the v2.57.0 milestone Apr 23, 2021
@magodo
Copy link
Collaborator Author

magodo commented Apr 24, 2021

@katbyte I've resolved the UT failures. Additionally, I've removed the constraints in the migration files, as they are not necessary to be defined in the migration schemas.

@katbyte katbyte merged commit df7cd66 into hashicorp:master Apr 25, 2021
magodo added a commit to magodo/terraform-provider-azurerm that referenced this pull request Apr 25, 2021
…d the panic

These properties has special meanings when being an empty block. Hence,
rather than adding the constraints, make sure there will be no panic
when being empty block instead.
katbyte pushed a commit that referenced this pull request Apr 25, 2021
…ic (#11464)

This PR reverts some of the AtLeastOneOf added in #11452, for which properties that are valid to be left as an empty block.

These properties have special meanings when being an empty block. Hence, rather than adding the constraints, make sure there will be no panic when being empty block and add the lint comment instead.
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants