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: Fix custom diffs for fields with diff supression #3032

Merged
Merged
36 changes: 36 additions & 0 deletions pkg/internal/provider/sdkv2enhancements/resource_data.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package sdkv2enhancements

import (
"reflect"
"unsafe"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

// CreateResourceDataFromResourceDiff allows to create schema.ResourceData out of schema.ResourceDiff.
// Unfortunately, schema.resourceDiffer is an unexported interface, and functions like schema.SchemaDiffSuppressFunc do not use the interface but a concrete implementation.
// One use case in which we needed to have schema.ResourceData from schema.ResourceDiff was to run schema.SchemaDiffSuppressFunc from inside schema.CustomizeDiffFunc.
// This implementation uses:
// - schema.InternalMap that exposes hidden schema.schemaMap (a wrapper over map[string]*schema.Schema)
// - (m schemaMap) Data method allowing to create schema.ResourceData from terraform.InstanceState and terraform.InstanceDiff
// - terraform.InstanceState and terraform.InstanceDiff are unexported in schema.ResourceDiff, so we get them using reflection
func CreateResourceDataFromResourceDiff(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) {
unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state")
stateFromResourceDiff := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface()

Check failure on line 20 in pkg/internal/provider/sdkv2enhancements/resource_data.go

View workflow job for this annotation

GitHub Actions / reviewdog

[golangci] reported by reviewdog 🐶 G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) Raw Output: pkg/internal/provider/sdkv2enhancements/resource_data.go:20:79: G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) stateFromResourceDiff := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface() ^
sfc-gh-jcieslak marked this conversation as resolved.
Show resolved Hide resolved
unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff")
diffFroResourceDif := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface()

Check failure on line 22 in pkg/internal/provider/sdkv2enhancements/resource_data.go

View workflow job for this annotation

GitHub Actions / reviewdog

[golangci] reported by reviewdog 🐶 G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) Raw Output: pkg/internal/provider/sdkv2enhancements/resource_data.go:22:75: G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) diffFroResourceDif := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface() ^
castState, ok := stateFromResourceDiff.(*terraform.InstanceState)
if !ok {
return nil, false
}
castDiff, ok := diffFroResourceDif.(*terraform.InstanceDiff)
if !ok {
return nil, false
}
resourceData, err := resourceSchema.Data(castState, castDiff)
if err != nil {
return nil, false
}
return resourceData, true
}
2 changes: 1 addition & 1 deletion pkg/resources/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func Account() *schema.Resource {
Delete: DeleteAccount,

CustomizeDiff: customdiff.All(
ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"),
ComputedIfAnyAttributeChanged(accountSchema, FullyQualifiedNameAttributeName, "name"),
),

Schema: accountSchema,
Expand Down
5 changes: 2 additions & 3 deletions pkg/resources/account_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,8 @@ func AccountRole() *schema.Resource {
Description: "The resource is used for role management, where roles can be assigned privileges and, in turn, granted to users and other roles. When granted to roles they can create hierarchies of privilege structures. For more details, refer to the [official documentation](https://docs.snowflake.com/en/user-guide/security-access-control-overview).",

CustomizeDiff: customdiff.All(
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment"),
ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"),
ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"),
ComputedIfAnyAttributeChanged(accountRoleSchema, ShowOutputAttributeName, "comment", "name"),
ComputedIfAnyAttributeChanged(accountRoleSchema, FullyQualifiedNameAttributeName, "name"),
),

Importer: &schema.ResourceImporter{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ func ApiAuthenticationIntegrationWithAuthorizationCodeGrant() *schema.Resource {
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint",
"oauth_token_endpoint", "oauth_allowed_scopes"),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ func ApiAuthenticationIntegrationWithClientCredentials() *schema.Resource {
CustomizeDiff: customdiff.All(
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_token_endpoint", "oauth_allowed_scopes"),
),
Importer: &schema.ResourceImporter{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func ApiAuthenticationIntegrationWithJwtBearer() *schema.Resource {
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint",
"oauth_token_endpoint", "oauth_assertion_issuer"),
),
Expand Down
46 changes: 26 additions & 20 deletions pkg/resources/custom_diffs.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ package resources

import (
"context"
"fmt"
"log"
"strconv"
"strings"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/sdkv2enhancements"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -83,33 +84,38 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc {
})
}

func ComputedIfAnyAttributeChanged(key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
// ComputedIfAnyAttributeChanged marks the given fields as computed if any of the listed fields changes.
// It takes field-level diffSuppress into consideration based on the schema passed.
// If the field is not found in the given schema, it continues without error.
func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
sfc-gh-jcieslak marked this conversation as resolved.
Show resolved Hide resolved
return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool {
var result bool
for _, changedKey := range changedAttributeKeys {
if diff.HasChange(changedKey) {
old, new := diff.GetChange(changedKey)
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, old, new)
}
result = result || diff.HasChange(changedKey)
}
return result
})
}

// TODO(SNOW-1629468): Adjust the function to make it more flexible
func ComputedIfAnyAttributeChangedWithSuppressDiff(key string, suppressDiffFunc schema.SchemaDiffSuppressFunc, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool {
for _, changedKey := range changedAttributeKeys {
if diff.HasChange(changedKey) {
oldValue, newValue := diff.GetChange(changedKey)
if !suppressDiffFunc(key, oldValue.(string), newValue.(string), nil) {
log.Printf("[DEBUG] ComputedIfAnyAttributeChangedWithSuppressDiff: changed key: %s", changedKey)
return true
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, oldValue, newValue)

if v, ok := resourceSchema[changedKey]; ok {
if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil {
resourceData, resourceDataOk := sdkv2enhancements.CreateResourceDataFromResourceDiff(resourceSchema, diff)
if !resourceDataOk {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: did not create resource data correctly, skipping\n")
continue
}
if !diffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), resourceData) {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey)
result = true
} else {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed but the diff is suppresed", changedKey)
sfc-gh-jcieslak marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and it does not have a diff suppressor", changedKey)
result = true
}
}
}
}
return false
return result
})
}

Expand Down
Loading
Loading