Skip to content

core: prevent panics with null objects in nested attrs#35090

Merged
jbardin merged 1 commit intomainfrom
jbardin/plan-valid-nested-null
May 6, 2024
Merged

core: prevent panics with null objects in nested attrs#35090
jbardin merged 1 commit intomainfrom
jbardin/plan-valid-nested-null

Conversation

@jbardin
Copy link
Copy Markdown
Member

@jbardin jbardin commented Apr 26, 2024

When descending into structural attributes, don't try to extract attributes from null objects. Unlike with blocks, nested attributes allow the possibility of assigning null values which could be overridden by the provider.

Technically nested object attributes like this should not be allowed to to change from the configuration (Computed and Optional do not apply to the individual container elements, only to the parent attribute), but due to existing providers we need to continue to let these pass through. Luckily the cases where it might cause problems are relatively few, since a corresponding configuration entry must have been assigned as null to allow the new value through in the first place, and any non-null attributes would already have to be defined as Computed to pass further validation. This behavior of the attribute acting like it's computed by virtue of having computed attributes itself is less surprising than if it actually had a schema setting that were being contradicted, and the fact that it is resolved early during the plan means it's less likely that fatal errors will pop up during apply.

Fixes #35083

@jbardin jbardin added the 1.8-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 26, 2024
@jbardin jbardin requested a review from a team April 26, 2024 19:27
@jbardin jbardin force-pushed the jbardin/plan-valid-nested-null branch from d904032 to f511356 Compare April 26, 2024 19:43
When descending into nested structural attributes, don't try to extract
attributes from null objects. Unlike with blocks, nested attributes
allow the possibility of assigning null values. While these technically
aren't allowed to be altered, we need to accept these for compatibility.
@jbardin jbardin force-pushed the jbardin/plan-valid-nested-null branch from f511356 to 2562420 Compare April 29, 2024 14:22
Copy link
Copy Markdown
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

This looks good, I'm pretty sure these null checks make sense. Just to clarify: in a situation like the following,

resource "pf9_cluster" "cluster" {
  name = var.cluster_name
  addons = {
    "kubernetes-dashboard" = null
    "something-else" = {
      # ...nested object attrs
    }
  }
}

...setting an entire key to null in that map of nested objects (e.g. "kubernetes-dashboard") is the act that results in attrS.NestedType being nil. Right?

Edit: Ahh that's not quite right. James filled me in in slack — NestedType is nil if it's a plain attr rather than a nested objects attr, so the changed behavior to stop the panic is all relevant to the non-nil NestedType case. The real issue with the example config would be something like: the config value is a null value of the relevant object type, but (due to a provider framework bug) the planned value is a non-null value of that object type whose keys are all null. So we need to cope with the potential nullness of the various value sources independently.

@jbardin jbardin merged commit 147186c into main May 6, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 6, 2024

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 6, 2024

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 Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

1.8-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic: interface conversion: interface {} is nil, not map[string]interface {}

3 participants