From 60e2c0cd20d465889a1271bad9f1f6a7a222ee6c Mon Sep 17 00:00:00 2001 From: Erhan Cagirici Date: Wed, 24 Jan 2024 17:45:56 +0300 Subject: [PATCH 1/4] fix Secret/secretmanagers.aws update loop for replica field Signed-off-by: Erhan Cagirici (cherry picked from commit 456d8d324bbf22d3c90e646117f229a292f4ffa0) --- config/secretsmanager/config.go | 199 +++++++++++++++++- .../secretsmanager/secret-withreplica.yaml | 14 ++ examples/secretsmanager/secret.yaml | 2 + 3 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 examples/secretsmanager/secret-withreplica.yaml diff --git a/config/secretsmanager/config.go b/config/secretsmanager/config.go index 6f3f8323ab..0c0ddb7e63 100644 --- a/config/secretsmanager/config.go +++ b/config/secretsmanager/config.go @@ -1,9 +1,18 @@ package secretsmanager -import "github.com/crossplane/upjet/pkg/config" +import ( + "fmt" + "strconv" + + "github.com/crossplane/upjet/pkg/config" + "github.com/crossplane/upjet/pkg/resource/json" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/pkg/errors" +) // Configure adds configurations for the secretsmanager group. -func Configure(p *config.Provider) { +func Configure(p *config.Provider) { //nolint:gocyclo p.AddResourceConfigurator("aws_secretsmanager_secret", func(r *config.Resource) { // Use aws_secretsmanager_secret_rotation. config.MoveToStatus(r.TerraformResource, "rotation_rules", "rotation_lambda_arn") @@ -13,5 +22,191 @@ func Configure(p *config.Provider) { r.TerraformConfigurationInjector = func(_ map[string]any, params map[string]any) { params["name_prefix"] = "" } + r.TerraformCustomDiff = func(diff *terraform.InstanceDiff, state *terraform.InstanceState, config *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { + // skip diff customization on create + if state == nil || state.Empty() { + return diff, nil + } + if config == nil { + return nil, errors.New("resource config cannot be nil") + } + // skip no diff or destroy diffs + if diff == nil || diff.Empty() || diff.Destroy || diff.Attributes == nil { + return diff, nil + } + + currentReplicaSet, ok := r.TerraformResource.Data(state).Get("replica").(*schema.Set) + if !ok { + return nil, errors.New("could not read \"replica\" from state") + } + nisReplica, ok := config.Get("replica") + if !ok { + return nil, errors.New("could not read replica block from config") + } + desiredReplicaList := nisReplica.([]interface{}) + // this is the hash implementation of *schema.Set, which is unexported + hashFunc := func(val interface{}) string { + code := currentReplicaSet.F(val) + if code < 0 { + code = -code + } + return strconv.Itoa(code) + } + // the number of + desiredReplicasWithRegionOnly := make(map[string]int) + // + // map[region]map[kms_key_id]hash + regionsInCurrentState := make(map[string]map[string]string) + type replica struct { + KMSKeyID string `json:"kms_key_id"` + Region string `json:"region"` + } + + // traverse state + for _, v := range currentReplicaSet.List() { + // v is an interface{} type, which is a replica + // marshal then unmarshal to convert it to the internal type replica for easier access to fields + replicaBytes, err := json.JSParser.Marshal(v) + if err != nil { + return nil, errors.Wrap(err, "cannot serialize replica") + } + cReplica := &replica{} + if err := json.JSParser.Unmarshal(replicaBytes, cReplica); err != nil { + return nil, err + } + if cReplica.Region == "" || cReplica.KMSKeyID == "" { + // we should not be here, replicas at current state always have their region set and kms_key_id computed + return nil, errors.New("replica in current state does not have region or kms key id set") + } + + // add the kms_key_id to the region's kms_key_id map. For convenience, store the hash of (region,kms_key_id) pair + if replicaHashesByKMSKeysOfRegion, ok := regionsInCurrentState[cReplica.Region]; ok { + replicaHashesByKMSKeysOfRegion[cReplica.KMSKeyID] = hashFunc(v) + } else { + // if we are adding a kms_key_id for the region for the first time, initialize the kms_key_id map + regionsInCurrentState[cReplica.Region] = map[string]string{ + cReplica.KMSKeyID: hashFunc(v), + } + } + } + + // a convenience function for deleting diff entries for a particular replica entry + removeReplicaFromDiffViaHash := func(hash string) { + delete(diff.Attributes, fmt.Sprintf("replica.%s.kms_key_id", hash)) + delete(diff.Attributes, fmt.Sprintf("replica.%s.region", hash)) + delete(diff.Attributes, fmt.Sprintf("replica.%s.status", hash)) + delete(diff.Attributes, fmt.Sprintf("replica.%s.status_message", hash)) + delete(diff.Attributes, fmt.Sprintf("replica.%s.last_accessed_date", hash)) + } + // traverse the desired Replica list at resource config (params) + // we want to count the Replicas, that has only region specified (no explicit kms_key_id specified, kms_key_id is left to cloud API to be automatically selected) + // then record this per region + for _, v := range desiredReplicaList { + // v is an interface{} type, which is a replica + // marshal then unmarshal to convert it to the internal type replica for easier access to fields + replicaBytes, err := json.JSParser.Marshal(v) + if err != nil { + return nil, err + } + dReplica := &replica{} + if err := json.JSParser.Unmarshal(replicaBytes, dReplica); err != nil { + return nil, err + } + + // count the region-only replicas (i.e. with automatically assigned KMS Key IDs) at parameters + if dReplica.KMSKeyID == "" { + if count, ok := desiredReplicasWithRegionOnly[dReplica.Region]; ok { + desiredReplicasWithRegionOnly[dReplica.Region] = count + 1 + } else { + desiredReplicasWithRegionOnly[dReplica.Region] = 1 + } + } else { + // this is a Replica at params, with explicit KMS Key ID specified in region + // check whether we have an exact match in current state + if replicaHashesByKMSKeyOfRegion, ok := regionsInCurrentState[dReplica.Region]; ok { + // we have an exact matching region,kms_key_id pair in the current state + // there should be no diff involved, remove the diff if it got calculated somehow + removeReplicaFromDiffViaHash(replicaHashesByKMSKeyOfRegion[dReplica.KMSKeyID]) + createdHash := hashFunc(map[string]interface{}{ + "kms_key_id": dReplica.KMSKeyID, + "region": dReplica.Region, + }) + removeReplicaFromDiffViaHash(createdHash) + delete(replicaHashesByKMSKeyOfRegion, dReplica.KMSKeyID) + } + } + } + + // now try to match the region-only desired KMS Key IDs with the ones in the left in current state (after filtering out the explicit matches above). + for region, unmatchedDesiredKMSKeyCount := range desiredReplicasWithRegionOnly { + kmsKeysOfRegionInCurrentState, ok := regionsInCurrentState[region] + if !ok { + // this is a Replica with brand-new region, no action needed. it already shows up on diff + continue + } + + switch { + case len(kmsKeysOfRegionInCurrentState) > unmatchedDesiredKMSKeyCount: + // for the particular region, we have more KMS Key IDs present in the current state than we desire, e.g. + // current state for region1 = { region1_kmsKeyA, region1_kmsKeyB, region1_kmsKeyC } + // desired state for region1 = { region1_kmsKeyANY } + // due to set difference implementation in TF, this will show up as 3 deletions, 1 creation in DIFF + // instead, in this case, we want to have 2 deletions, 0 creation DIFF + // thus, remove all (unmatchedDesiredKMSKeyCount=1) creation diffs and remove (unmatchedDesiredKMSKeyCount=1) deletion diff + // Arbitrarily choose which replica to delete, since they're indistinguishable + i := 0 + for _, hash := range kmsKeysOfRegionInCurrentState { + if i >= unmatchedDesiredKMSKeyCount { + break + } + removeReplicaFromDiffViaHash(hash) + i++ + } + creationHash := hashFunc(map[string]interface{}{ + "kms_key_id": "", + "region": region, + }) + removeReplicaFromDiffViaHash(creationHash) + case len(kmsKeysOfRegionInCurrentState) < unmatchedDesiredKMSKeyCount: + // this might not be possible at all, due to Replica hash function + for _, hash := range kmsKeysOfRegionInCurrentState { + removeReplicaFromDiffViaHash(hash) + } + default: + // for the particular region, we have matching number of KMS Key IDs to desired, i.e. there should be no diff for these + // example + // current state for region2 KMS Key IDs = { region2_kmsKeyX} + // desired state for region2 KMS Key IDs = { region2_kmsKeyANY } + // due to set difference implementation in TF, this will show up as 1 deletion, 1 creation in DIFF + // instead, in this case, we want to have no diff + // thus, remove all creation diffs and remove all deletion diffs + for _, hash := range kmsKeysOfRegionInCurrentState { + removeReplicaFromDiffViaHash(hash) + } + creationHash := hashFunc(map[string]interface{}{ + "kms_key_id": "", + "region": region, + }) + removeReplicaFromDiffViaHash(creationHash) + } + } + // compare the total desired Replica count and current Replica count + // adjust the diff for replica.# + if len(desiredReplicaList) == len(currentReplicaSet.List()) { + // no diff, therefore remove diff if exists + delete(diff.Attributes, "replica.#") + } else if replicaCount, ok := diff.Attributes["replica.#"]; ok { + // there is a diff in unmodified diff, make sure it is correct after modifications + replicaCount.Old = strconv.Itoa(len(currentReplicaSet.List())) + replicaCount.New = strconv.Itoa(len(desiredReplicaList)) + } else { + // there was no diff in unmodified diff, but there is on the customized. Add this diff + diff.Attributes["replica.#"] = &terraform.ResourceAttrDiff{ + Old: strconv.Itoa(len(currentReplicaSet.List())), + New: strconv.Itoa(len(desiredReplicaList)), + } + } + return diff, nil + } }) } diff --git a/examples/secretsmanager/secret-withreplica.yaml b/examples/secretsmanager/secret-withreplica.yaml new file mode 100644 index 0000000000..9b529c6d26 --- /dev/null +++ b/examples/secretsmanager/secret-withreplica.yaml @@ -0,0 +1,14 @@ +apiVersion: secretsmanager.aws.upbound.io/v1beta1 +kind: Secret +metadata: + name: example-withreplica + annotations: + meta.upbound.io/example-id: secretsmanager/v1beta1/secret + labels: + testing.upbound.io/example-name: secretsmanager +spec: + forProvider: + name: example-withreplica-${Rand.RFC1123Subdomain} + region: us-west-1 + replica: + - region: us-west-2 diff --git a/examples/secretsmanager/secret.yaml b/examples/secretsmanager/secret.yaml index da60ef77bd..655dc856f2 100644 --- a/examples/secretsmanager/secret.yaml +++ b/examples/secretsmanager/secret.yaml @@ -2,6 +2,8 @@ apiVersion: secretsmanager.aws.upbound.io/v1beta1 kind: Secret metadata: name: example + annotations: + meta.upbound.io/example-id: secretsmanager/v1beta1/secret labels: testing.upbound.io/example-name: secretsmanager spec: From 63bab9d209f1966b0a674df7b7ca7db7397d594d Mon Sep 17 00:00:00 2001 From: Erhan Cagirici Date: Fri, 9 Feb 2024 17:57:00 +0300 Subject: [PATCH 2/4] fix Secret/secretmanager.aws custom diff logic when replica config is empty Signed-off-by: Erhan Cagirici (cherry picked from commit 8d35a8294227cc41406682a1b34428877bf6b1c0) --- config/secretsmanager/config.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/config/secretsmanager/config.go b/config/secretsmanager/config.go index 0c0ddb7e63..702090ffcc 100644 --- a/config/secretsmanager/config.go +++ b/config/secretsmanager/config.go @@ -35,13 +35,25 @@ func Configure(p *config.Provider) { //nolint:gocyclo return diff, nil } + resData, err := schema.InternalMap(r.TerraformResource.SchemaMap()).Data(state, diff) + if err != nil { + return nil, errors.New("could not construct resource data") + } + + // do not customize diff if replica field has no change + if !resData.HasChange("replica") { + return diff, nil + } currentReplicaSet, ok := r.TerraformResource.Data(state).Get("replica").(*schema.Set) if !ok { return nil, errors.New("could not read \"replica\" from state") } + nisReplica, ok := config.Get("replica") if !ok { - return nil, errors.New("could not read replica block from config") + // config is empty for replica, no need for custom diff logic + // this is already handled correctly with the built-in diff logic + return diff, nil } desiredReplicaList := nisReplica.([]interface{}) // this is the hash implementation of *schema.Set, which is unexported From 326e818a058dc4cb20f291ad060eb97f881beb25 Mon Sep 17 00:00:00 2001 From: Matt Bush Date: Wed, 14 Feb 2024 18:24:34 -0800 Subject: [PATCH 3/4] Add examples with update parameter for secretsmanager secret (cherry picked from commit 3df0d8de85c8beb87fcec918013f96c9fd088c97) --- examples/secretsmanager/secret-withreplica.yaml | 2 ++ examples/secretsmanager/secret.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/examples/secretsmanager/secret-withreplica.yaml b/examples/secretsmanager/secret-withreplica.yaml index 9b529c6d26..1742e3163c 100644 --- a/examples/secretsmanager/secret-withreplica.yaml +++ b/examples/secretsmanager/secret-withreplica.yaml @@ -4,11 +4,13 @@ metadata: name: example-withreplica annotations: meta.upbound.io/example-id: secretsmanager/v1beta1/secret + uptest.upbound.io/update-parameter: '{"tags":{"updated-by":"crossplane"}}' labels: testing.upbound.io/example-name: secretsmanager spec: forProvider: name: example-withreplica-${Rand.RFC1123Subdomain} + recoveryWindowInDays: 0 region: us-west-1 replica: - region: us-west-2 diff --git a/examples/secretsmanager/secret.yaml b/examples/secretsmanager/secret.yaml index 655dc856f2..380c30735d 100644 --- a/examples/secretsmanager/secret.yaml +++ b/examples/secretsmanager/secret.yaml @@ -4,9 +4,11 @@ metadata: name: example annotations: meta.upbound.io/example-id: secretsmanager/v1beta1/secret + uptest.upbound.io/update-parameter: '{"tags":{"updated-by":"crossplane"}}' labels: testing.upbound.io/example-name: secretsmanager spec: forProvider: name: example-${Rand.RFC1123Subdomain} region: us-west-1 + recoveryWindowInDays: 0 From e54d24ce15258f968c46316f038cfd48f8a610b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Thu, 15 Feb 2024 17:00:36 +0300 Subject: [PATCH 4/4] Use Schema instead of SchemaMap func MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- config/secretsmanager/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/secretsmanager/config.go b/config/secretsmanager/config.go index 702090ffcc..944de47273 100644 --- a/config/secretsmanager/config.go +++ b/config/secretsmanager/config.go @@ -35,7 +35,7 @@ func Configure(p *config.Provider) { //nolint:gocyclo return diff, nil } - resData, err := schema.InternalMap(r.TerraformResource.SchemaMap()).Data(state, diff) + resData, err := schema.InternalMap(r.TerraformResource.Schema).Data(state, diff) if err != nil { return nil, errors.New("could not construct resource data") }