[APIE-848] Fix for schema_registry_cluster drift during migration#947
[APIE-848] Fix for schema_registry_cluster drift during migration#947Cynthia Qin (cqin-confluent) wants to merge 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds drift suppression logic to handle migration scenarios for the confluent_schema_registry_cluster_config resource. Specifically, it addresses drift detection issues when users migrate from resource-level configuration (Option 1) to provider-level configuration (Option 2) for Schema Registry cluster settings.
Changes:
- Added
suppressSchemaRegistryClusterConfigMigrationDifffunction to suppress diffs during migration from resource-level to provider-level configuration - Integrated the suppression function into the
CustomizeDiffsequence for the schema registry cluster config resource
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The diff suppression function only handles rest_endpoint and schema_registry_cluster, but it doesn't suppress the credentials block during migration. When migrating from resource-level configuration (Option 1) to provider-level configuration (Option 2), users would typically remove the credentials block from the resource, which could cause an unwanted diff if the old credentials match the provider's credentials. Consider adding similar logic to suppress the credentials diff when the old credentials match the provider's schema_registry_api_key and schema_registry_api_secret.
| // Verify the old credentials match the provider's credentials and new param is empty/null, then suppress the diff | |
| if diff.HasChange(paramCredentials) { | |
| oldCredentialsBlock, newCredentialsBlock := diff.GetChange(paramCredentials) | |
| oldCredentialsList, oldOk := oldCredentialsBlock.([]interface{}) | |
| newCredentialsList, newOk := newCredentialsBlock.([]interface{}) | |
| if oldOk && len(oldCredentialsList) > 0 && (!newOk || len(newCredentialsList) == 0) { | |
| oldCredentialsMap, ok := oldCredentialsList[0].(map[string]interface{}) | |
| if !ok { | |
| return nil | |
| } | |
| oldApiKey, okKey := oldCredentialsMap[paramKey].(string) | |
| oldApiSecret, okSecret := oldCredentialsMap[paramSecret].(string) | |
| if !okKey || !okSecret || oldApiKey == "" || oldApiSecret == "" { | |
| return nil | |
| } | |
| if oldApiKey == client.schemaRegistryApiKey && oldApiSecret == client.schemaRegistryApiSecret { | |
| if err := diff.SetNew(paramCredentials, oldCredentialsBlock); err != nil { | |
| return fmt.Errorf("error suppressing credentials diff: %s", createDescriptiveError(err)) | |
| } | |
| } | |
| } | |
| } |
| func suppressSchemaRegistryClusterConfigMigrationDiff(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) error { | ||
| client := meta.(*Client) | ||
|
|
||
| // Supress iff provider has Schema Registry metadata set (Option 2) | ||
| if !client.isSchemaRegistryMetadataSet { | ||
| return nil | ||
| } | ||
|
|
||
| // Verify the old rest_endpoint matches the provider's rest_endpoint and new param is empty/null, then suppress the diff | ||
| if diff.HasChange(paramRestEndpoint) { | ||
| oldRestEndpoint, newRestEndpoint := diff.GetChange(paramRestEndpoint) | ||
| oldRestEndpointStr, oldOk := oldRestEndpoint.(string) | ||
| newRestEndpointStr, newOk := newRestEndpoint.(string) | ||
|
|
||
| if oldOk && oldRestEndpointStr != "" && (!newOk || newRestEndpointStr == "") { | ||
| if oldRestEndpointStr == client.schemaRegistryRestEndpoint { | ||
| if err := diff.SetNew(paramRestEndpoint, oldRestEndpointStr); err != nil { | ||
| return fmt.Errorf("error suppressing rest_endpoint diff: %s", createDescriptiveError(err)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Verify the old cluster ID matches the provider's cluster ID and new param is empty/null, then suppress the diff | ||
| if diff.HasChange(paramSchemaRegistryCluster) { | ||
| oldClusterBlock, newClusterBlock := diff.GetChange(paramSchemaRegistryCluster) | ||
| oldClusterList, oldOk := oldClusterBlock.([]interface{}) | ||
| newClusterList, newOk := newClusterBlock.([]interface{}) | ||
|
|
||
| if oldOk && len(oldClusterList) > 0 && (!newOk || len(newClusterList) == 0) { | ||
| oldClusterMap, ok := oldClusterList[0].(map[string]interface{}) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| oldClusterId, ok := oldClusterMap[paramId].(string) | ||
| if !ok || oldClusterId == "" { | ||
| return nil | ||
| } | ||
|
|
||
| if oldClusterId == client.schemaRegistryClusterId { | ||
| if err := diff.SetNew(paramSchemaRegistryCluster, oldClusterBlock); err != nil { | ||
| return fmt.Errorf("error suppressing schema_registry_cluster diff: %s", createDescriptiveError(err)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
There are no tests for the new suppressSchemaRegistryClusterConfigMigrationDiff function. Consider adding a test case that verifies the migration scenario from resource-level configuration to provider-level configuration, ensuring that diffs are properly suppressed when the configurations match.
| func suppressSchemaRegistryClusterConfigMigrationDiff(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) error { | ||
| client := meta.(*Client) | ||
|
|
||
| // Supress iff provider has Schema Registry metadata set (Option 2) |
There was a problem hiding this comment.
There is a spelling error in the comment. "Supress" should be "Suppress".
| // Supress iff provider has Schema Registry metadata set (Option 2) | |
| // Suppress iff provider has Schema Registry metadata set (Option 2) |
| }, | ||
| Schema: map[string]*schema.Schema{ | ||
| paramSchemaRegistryCluster: schemaRegistryClusterBlockSchema(), | ||
| paramSchemaRegistryCluster: { |
There was a problem hiding this comment.
unrelated quick question: why did we stop using schemaRegistryClusterBlockSchema()?
There was a problem hiding this comment.
schemaRegistryClusterBlockSchema() has a static ForceNew. So I added a separate function schemaRegistryClusterConfigForceNewCustomDiff instead, that applies custom ForceNew logic to prevent TF force recreation when migrating resources within the same cluster.
Release Notes
New Features
Bug Fixes
Examples
Checklist
Test & Reviewsection below.Blast Radiussection below.What
There is a Terraform issue that can cause drift when migrating the provider type from Option 1 to Option 2 for the confluent_schema_registry_cluster_config resource.
Specifically, both rest_endpoint and the schema_registry_cluster block are marked ForceNew in existing Terraform logic, which causes Terraform to incorrectly plan a recreation during the migration even though it’s the same underlying cluster.
The fix is is to suppress the diff during migration, while still preserving the current behavior where user-initiated changes to these fields should still trigger ForceNew
Blast Radius
This is a bug fix so no existing workflow should be impacted.
References
APIE-852: Migrate provider type (from #Option 1 to #Option2)
Test & Review
Integration test passes
manual test of Option 1 -> Option 2 migration (reproduce the drift)
manual test when it’s the same underlying cluster(after the fix)
manual test when user-initiated changes to these fields should still trigger ForceNew (after the fix)