SetupFn can silently panic

[eric-carlsson]

Hi,

After upgrading a provider generated from this template from v1 to v2, we started experiencing issues where it would silently fail. No logs, no events, but MR:s didn't sync.

After much effort I found that root cause was that we had accidentally dropped initializing the Configuration struct during the migration:

ps.Configuration = map[string]any{}

This caused Go to panic because of the nil pointer reference (understandable), which was seemingly recovered at some point without any form of logging (!?). This leads me to two questions/suggestions:

• How can we make sure recovered panics are logged? I'm not sure what is doing the recovery (controller runtime?), so I'm unsure where to look for this.

• Does it make sense to adjust the template so ps.Configuration is initialized by default? Even if it's just empty and doesn't do anything, it would save others from the same issue.

  ps := terraform.Setup{
  	Version: version,
  	Requirement: terraform.ProviderRequirement{
  		Source:  providerSource,
  		Version: providerVersion,
  	},
  	Configuration: terraform.ProviderConfiguration{},
  }

[jeanduplessis]

Thanks for bringing this to our attention @eric-carlsson, I'll ask the team to take a look at it later in the week.

[jeanduplessis]

@eric-carlsson, I discussed this with the team and here's some context...

Panic Recovery

The panic recovery is a controller-runtime feature that's enabled by default. This is intentional - it prevents a problem with one resource type from crashing the entire provider. The recovery happens automatically through controller-runtime's reconciliation wrapper (see the implementation
here).

Why Logs Weren't Visible

By default, we suppress controller-runtime logs because they're quite verbose even at info level. Panic logs should be visible when you pass the --debug flag to the provider. In local development (make run), this flag is set by default, but in production-like deployments without --debug, these logs won't appear.

Immediate workaround: Run your provider with --debug to make panic logs visible.

Design Intent vs. Developer Experience

The Configuration field is intentionally designed to be manually implemented by provider authors.
An empty configuration almost never makes sense - this is where you map credentials and provider-specific settings from your ProviderConfig into the Terraform provider configuration (as shown in the commented example on lines
54-57).

This is also documented in our provider generation guide. In a sense, the panic highlights something that requires implementation.

However, we acknowledge that a silent failure with no clear feedback is poor developer experience, especially during migrations.

Potential Improvements

We're considering several options:

1. Build-time guardrails: Add an intentional syntax error or build-breaking placeholder that forces provider authors to address Configuration setup explicitly.
2. Auto-generation: Potentially auto-generate the ProviderConfig schema and basic Configuration mapping from the Terraform provider schema.
3. Better in-code hints: Improve comments and structure to make it more obvious that Configuration requires implementation.
4. Default initialization: Initialize Configuration as an empty map by default (as you suggested), though this might mask cases where configuration is genuinely needed.

We'd be interested to hear which approach you think would work best.

Your Specific Case

For your immediate needs:

• Initializing Configuration: map[string]any{} is correct if your provider doesn't need configuration
• Use the --debug flag in deployments to ensure panic logs are visible
• See the provider generation docs for more details on configuration setup

Let us know if you have thoughts on the proposed improvements.

[eric-carlsson]

Thanks for the thorough reply, @jeanduplessis.

Immediate workaround: Run your provider with --debug to make panic logs visible.

I did, but even with the debug flag I got no output when the provider panicked. I was only able to catch it by adding an extra recovery clause in the setup function. I need to look into this some more if that's not expected behavior. 🤔

The Configuration field is intentionally designed to be manually implemented by provider authors. An empty configuration almost never makes sense—this is where you map credentials and provider-specific settings from your ProviderConfig into the Terraform provider configuration (as shown in the commented example on lines 54-57).

4. Default initialization: Initialize Configuration as an empty map by default (as you suggested), though this might mask cases where configuration is genuinely needed.

I think that's a fair concern, and you are right that the provider template already clearly indicates that it should be created and populated.

I used some of the official providers, like provider-upjet-azure, as reference. They first initialize a more or less empty Configuration, and then populate it. In that case, it may make sense to initialize the Configuration field on terraform.Setup, even if only to an empty struct.

[eric-carlsson]

I've done some more testing and found a couple of issues with the way loggers are commonly set up. These can suppress expected log output, including panics.

Not all of these issues are inherent to this template, and I will create separate issues in affected provider repos. However, I think it would be good to address them here as well.

1. Problem: The controller-runtime controller must be passed a logger, otherwise it errors.

   This has been reported in multiple provider repos. If a logger for the controller is not explicitly set, it errors/panics like so:

   [controller-runtime] log.SetLogger(...) was never called, logs will not be displayed:
   ...stack trace...

   It will fall back to a default logger, so the pattern used in this repo works, but it's not ideal UX.

   Fix: Always set a controller-runtime logger, even if it just pipes output to io.Discard.

2. Problem: A global controller-runtime logger can only be set once. Subsequent calls to ctrl.SetLogger have no effect.

   It's extremely common in official providers (Azure, GCP, AWS, etc.) to see a pattern like this:

   import ctrl "sigs.k8s.io/controller-runtime"
   
   ...
   
   ctrl.SetLogger(zap.New(zap.WriteTo(io.Discard)))
   
   zl := zap.New(zap.UseDevMode(*debug))
   if *debug {
       ctrl.SetLogger(zl)
   }

   As a result, the controller logger will always be set to discard, even if the debug flag is specified. This is due to how the global SetLogger function is implemented: https://github.com/sbueringer/controller-runtime/blob/67242822fa14d36e8ef7a5f7933022334b11a976/pkg/log/log.go#L48-L52

   Fix: Only set a controller-runtime logger once. Updated example with this fix:

   zl := zap.New(zap.UseDevMode(*debug))
   if *debug {
       ctrl.SetLogger(zl)
   } else {
       ctrl.SetLogger(zap.New(zap.WriteTo(io.Discard)))
   }

   This isn't technically the fault of this template, but fixing (1) requires that the template implements this fix too.

Additional points (not necessarily errors, but worth noting):

• Currently, we mute log output from the underlying Terraform provider. This output is useful for debugging, so maybe it makes sense to include it when debug is enabled?

  if *debug {
      // Default logger is used by Terraform provider internally.
      // It is also *very* verbose, so only enable in debug mode.
      log.Default().SetOutput(os.Stderr)
  } else {
      log.Default().SetOutput(io.Discard)
  }

• Finding these issues was complicated by the use of (default) global loggers. Perhaps it would be best to avoid global variables and pass loggers explicitly instead. This probably requires updating more than just this template.

[ulucinar]

Hi @eric-carlsson,
Thank you for the detailed analysis on logging. I think we should definitely address [1] and [2] in this repo and in the other repositories affected by [2]. We should treat [2] as a bug as setting the controller-runtime logger twice does not have the desired effect of replacing the discarding logger previously configured.

For the following discussion, my context is upjet's no-fork runtime architecture. The issues with the Terraform provider logs were: Some of them were too verbose and enabling them disrupts the Crossplane provider's structured logging. That's why we had chosen to suppress them. But I also agree they have value for debugging. Maybe a middle ground could be:

• To put log.Default().SetOutput(io.Discard) and log.Default().SetOutput(os.Stderr) as you suggest above but commented out. There's an unconditional discard to be safe & structured by default.
• To add proper documentation comments for informing the provider author about the underlying Terraform provider logging.

We would also like to prevent leaking sensitive information (like secrets, passwords, etc.), or PII or similar with the provider logs. When we enable Terraform provider logging, the Terraform provider's logs become part of the Crossplane provider's logs and it's difficult for the Crossplane provider author to have control on what's being logged at the Terraform layer in production.

Regarding the terraform.Setup.Configuration initialization, as it's been noted above, it's Terraform provider specific. I personally prefer the current commented-out code section instead of an empty initialization.

Would you have some time to introduce the logging improvements @eric-carlsson? Please let me know if you'd like introduce them yourself or would like us to pursue those changes?