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

Prevent panic on null values when iterating over map elements #413

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

LouisBrunner
Copy link
Contributor

📚 Context

I was writing a small module to create organizations from a list:

module "orgs" {
  source                 = "./orgs"
  orgs = [
    {
      name    = "org1-id"
      display = "Org1 Name"
      logo    = "https://some-url.example"
    },
    {
      name    = "org2-id"
      display = "Org2 Name"
      logo    = "https://some-url.example"
      branding = {
        primary    = "#7da5de"
        background = "#000000"
      }
    },
  ]
}

then defined my module as such

variable "orgs" {
  type = list(object({
    name    = string,
    display = string,
    logo    = string,
    branding = optional(object({
      primary    = string,
      background = string,
    }))
  }))
}

resource "auth0_organization" "org" {
  for_each     = { for org in var.orgs : org.name => org }
  name         = each.key
  display_name = each.value.display

  branding {
    logo_url = each.value.logo
    colors = {
      primary         = try(each.value.branding.primary, null)
      page_background = try(each.value.branding.background, null)
    }
  }
}

However, when I tried to apply, I got the following crash from this provider:

Stack trace from the terraform-provider-auth0_v0.40.0 plugin:

panic: value is null

goroutine 113 [running]:
github.com/hashicorp/go-cty/cty.Value.AsString({{{0x10117e0e0?, 0x1400003a189?}}, {0x0?, 0x0?}})
	github.com/hashicorp/[email protected]/cty/value_ops.go:1176 +0x11c
github.com/auth0/terraform-provider-auth0/internal/value.MapOfStrings({{{0x10117e188?, 0x1400078c170?}}, {0x1010836e0?, 0x1400078e270?}})
	github.com/auth0/terraform-provider-auth0/internal/value/value.go:99 +0x108
github.com/auth0/terraform-provider-auth0/internal/provider.expandOrganizationBranding.func1({{{0x140002472f0?, 0x1400078c1b0?}}, {0x101034fc0?, 0x14000796018?}}, {{{0x10117e1c0?, 0x1400066a050?}}, {0x1010836e0?, 0x1400078e2a0?}})
	github.com/auth0/terraform-provider-auth0/internal/provider/resource_auth0_organization.go:158 +0x8c
github.com/hashicorp/go-cty/cty.Value.ForEachElement({{{0x10117e150?, 0x1400078c1b0?}}, {0x101034fc0?, 0x14000796018?}}, 0x14000708dc8)
	github.com/hashicorp/[email protected]/cty/value_ops.go:1066 +0x98
github.com/auth0/terraform-provider-auth0/internal/provider.expandOrganizationBranding({{{0x10117e150?, 0x1400078c1b0?}}, {0x101034fc0?, 0x14000796018?}})
	github.com/auth0/terraform-provider-auth0/internal/provider/resource_auth0_organization.go:155 +0x4c
github.com/auth0/terraform-provider-auth0/internal/provider.expandOrganization(0x0?)
	github.com/auth0/terraform-provider-auth0/internal/provider/resource_auth0_organization.go:142 +0x98
github.com/auth0/terraform-provider-auth0/internal/provider.updateOrganization({0x10117da88, 0x1400010c720}, 0x1400053d500, {0x1010b6980?, 0x140003aa9a0?})
	github.com/auth0/terraform-provider-auth0/internal/provider/resource_auth0_organization.go:113 +0x54
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).update(0x1400047c9a0, {0x10117dac0, 0x140002558f0}, 0xd?, {0x1010b6980, 0x140003aa9a0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:741 +0xe8
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0x1400047c9a0, {0x10117dac0, 0x140002558f0}, 0x140001405b0, 0x1400053d100, {0x1010b6980, 0x140003aa9a0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:847 +0x67c
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0x1400053a000, {0x10117dac0?, 0x140002557d0?}, 0x14000568f00)
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:1021 +0xb70
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ApplyResourceChange(0x14000540000, {0x10117dac0?, 0x140002551a0?}, 0x140000ab960)
	github.com/hashicorp/[email protected]/tfprotov5/tf5server/server.go:818 +0x3b8
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ApplyResourceChange_Handler({0x101140680?, 0x14000540000}, {0x10117dac0, 0x140002551a0}, 0x140000ab8f0, 0x0)
	github.com/hashicorp/[email protected]/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:385 +0x170
google.golang.org/grpc.(*Server).processUnaryRPC(0x14000440000, {0x1011814e0, 0x14000103040}, 0x1400070c000, 0x14000514660, 0x1016712a0, 0x0)
	google.golang.org/[email protected]/server.go:1295 +0x9c4
google.golang.org/grpc.(*Server).handleStream(0x14000440000, {0x1011814e0, 0x14000103040}, 0x1400070c000, 0x0)
	google.golang.org/[email protected]/server.go:1636 +0x82c
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	google.golang.org/[email protected]/server.go:932 +0x84
created by google.golang.org/grpc.(*Server).serveStreams.func1
	google.golang.org/[email protected]/server.go:930 +0x290

Error: The terraform-provider-auth0_v0.40.0 plugin crashed!

This is always indicative of a bug within the plugin. It would be immensely
helpful if you could report the crash with the plugin's maintainers so that it
can be fixed. The output above should help diagnose the issue.

🔧 Changes

This checks IsNull on cty.Value before calling AsString which ensures that the provider doesn't panic if a value is missing. The crash is then replaced by:

╷
│ Error: 400 Bad Request: Payload validation error: 'Missing required property: page_background' on property branding.colors (Color scheme used to customize the login pages). (also) Payload validation error: 'Missing required property: primary' on property branding.colors (Color scheme used to customize the login pages).
│
│   with auth0_organization.my_organization,
│   on main.tf line 20, in resource "auth0_organization" "my_organization":
│   20: resource "auth0_organization" "my_organization" {
│
╵

🔬 Testing

I expanded the unit tests to check the edge case. Also I used this minimal reproducible example to check (crash with v0.40):

resource "auth0_organization" "my_organization" {
  name         = "auth0-inc"
  display_name = "Auth0 Inc."

  branding {
    logo_url = "https://example.com/assets/icons/icon.png"
    colors = {
      primary         = null
      page_background = null
    }
  }
}

Note: you will only get the crash on apply, not during planning, so if there are no difference to apply, it will just exit normally. Change the display name and try to apply and the crash will happen.

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • [N/A] I have added documentation for all new/changed functionality (or N/A)

@LouisBrunner LouisBrunner requested a review from a team as a code owner December 8, 2022 16:27
@sergiught sergiught changed the title fix: panic "value is null" Prevent panic on null values when iterating over map elements in value pkg funcs Dec 12, 2022
Copy link
Contributor

@sergiught sergiught left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @LouisBrunner! Good catch!

@sergiught sergiught changed the title Prevent panic on null values when iterating over map elements in value pkg funcs Prevent panic on null values when iterating over map elements Dec 12, 2022
@sergiught sergiught merged commit 72ce2b1 into auth0:main Dec 12, 2022
@LouisBrunner LouisBrunner deleted the feature/fix-panic branch December 12, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants