From d89554fa97cbb7912129214c01e3a08b0833dbc9 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Mon, 2 Sep 2024 20:34:44 +0200 Subject: [PATCH 1/8] chore: validate whether `secret_suffix` contains a trailing `-` --- .../backend/remote-state/kubernetes/backend.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index 85d3037c6734..c3d621278345 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -10,6 +10,8 @@ import ( "log" "os" "path/filepath" + "strconv" + "strings" "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/legacy/helper/schema" @@ -50,6 +52,21 @@ func New() backend.Backend { Type: schema.TypeString, Required: true, Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`.", + ValidateFunc: func(v interface{}, s string) ([]string, []error) { + value := v.(string) + // Split the suffix by '-' and check the last part + parts := strings.Split(value, "-") + if len(parts) > 1 { + lastPart := parts[len(parts)-1] + if _, err := strconv.Atoi(lastPart); err == nil { + // If the last segment is a number, it's considered invalid. + // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. + // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. + return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got '%s'", value)} + } + } + return nil, nil + }, }, "labels": { Type: schema.TypeMap, From fbcc800e61a8599057aaed0e3a94f841866291df Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Mon, 2 Sep 2024 20:40:13 +0200 Subject: [PATCH 2/8] docs: update description of `secret_suffix` to reflect the state file chunking mechanism --- internal/backend/remote-state/kubernetes/backend.go | 2 +- website/docs/language/settings/backends/kubernetes.mdx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index c3d621278345..1b13d3d240fa 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -51,7 +51,7 @@ func New() backend.Backend { "secret_suffix": { Type: schema.TypeString, Required: true, - Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`.", + Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`.", ValidateFunc: func(v interface{}, s string) ([]string, []error) { value := v.(string) // Split the suffix by '-' and check the last part diff --git a/website/docs/language/settings/backends/kubernetes.mdx b/website/docs/language/settings/backends/kubernetes.mdx index 5b5d7ed82ec0..30794c4e5884 100644 --- a/website/docs/language/settings/backends/kubernetes.mdx +++ b/website/docs/language/settings/backends/kubernetes.mdx @@ -48,7 +48,7 @@ data "terraform_remote_state" "foo" { The following configuration options are supported: -* `secret_suffix` - (Required) Suffix used when creating secrets. Secrets will be named in the format: `tfstate-{workspace}-{secret_suffix}`. +* `secret_suffix` - (Required) Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`. * `labels` - (Optional) Map of additional labels to be applied to the secret and lease. * `namespace` - (Optional) Namespace to store the secret and lease in. Can be sourced from `KUBE_NAMESPACE`. * `in_cluster_config` - (Optional) Used to authenticate to the cluster from inside a pod. Can be sourced from `KUBE_IN_CLUSTER_CONFIG`. From c4fe5afc0eb7ff9f5f16802fd46dcde647c93d0a Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Mon, 2 Sep 2024 20:50:13 +0200 Subject: [PATCH 3/8] chore: add comment for clarity on what's happening --- internal/backend/remote-state/kubernetes/client.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/backend/remote-state/kubernetes/client.go b/internal/backend/remote-state/kubernetes/client.go index 9e290dd1f9e5..5c44cb662755 100644 --- a/internal/backend/remote-state/kubernetes/client.go +++ b/internal/backend/remote-state/kubernetes/client.go @@ -102,6 +102,8 @@ func (c *RemoteClient) getSecrets() ([]unstructured.Unstructured, error) { for _, item := range res.Items { name := item.GetName() nameParts := strings.Split(name, "-") + // Because large Terraform state files are split into multiple secrets, + // we parse the index from the secret name. index, err := strconv.Atoi(nameParts[len(nameParts)-1]) if err != nil { index = 0 From 3893edc22d9836fd6b2df88a483e38eafac92114 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Mon, 2 Sep 2024 20:56:57 +0200 Subject: [PATCH 4/8] chore: consistent wording --- internal/backend/remote-state/kubernetes/backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index 1b13d3d240fa..2d0eba45fbae 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -54,7 +54,7 @@ func New() backend.Backend { Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`.", ValidateFunc: func(v interface{}, s string) ([]string, []error) { value := v.(string) - // Split the suffix by '-' and check the last part + // Split the suffix by '-' and check the last segment. parts := strings.Split(value, "-") if len(parts) > 1 { lastPart := parts[len(parts)-1] From 732f0cd5f25a86cca97a26a57a4dda161947f401 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 20 Sep 2024 08:43:12 -0400 Subject: [PATCH 5/8] chore: use %q directive --- internal/backend/remote-state/kubernetes/backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index 9ef788c868f8..72d90576b0eb 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -68,7 +68,7 @@ func New() backend.Backend { // If the last segment is a number, it's considered invalid. // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. - return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got '%s'", value)} + return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got %q", value)} } } return nil, nil From 7efa5cdc082ed36fd6d2c531fec37cf28e8cf379 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 20 Sep 2024 08:48:40 -0400 Subject: [PATCH 6/8] chore: use strings.LastIndex --- internal/backend/remote-state/kubernetes/backend.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index 72d90576b0eb..66606a2dd5eb 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -60,10 +60,9 @@ func New() backend.Backend { Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`.", ValidateFunc: func(v interface{}, s string) ([]string, []error) { value := v.(string) - // Split the suffix by '-' and check the last segment. - parts := strings.Split(value, "-") - if len(parts) > 1 { - lastPart := parts[len(parts)-1] + // Find the last occurrence of '-' and get the part after it. + if idx := strings.LastIndex(value, "-"); idx != -1 { + lastPart := value[idx+1:] if _, err := strconv.Atoi(lastPart); err == nil { // If the last segment is a number, it's considered invalid. // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. From 981cf3968e520c99a10246c9cda1ed5b42c6b83b Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 20 Sep 2024 09:09:26 -0400 Subject: [PATCH 7/8] chore: refactor to offload logic to a separate function --- .../remote-state/kubernetes/backend.go | 28 +++++++++++++------ .../remote-state/kubernetes/backend_test.go | 21 ++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index 66606a2dd5eb..dabf66fbf206 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -60,15 +60,12 @@ func New() backend.Backend { Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`.", ValidateFunc: func(v interface{}, s string) ([]string, []error) { value := v.(string) - // Find the last occurrence of '-' and get the part after it. - if idx := strings.LastIndex(value, "-"); idx != -1 { - lastPart := value[idx+1:] - if _, err := strconv.Atoi(lastPart); err == nil { - // If the last segment is a number, it's considered invalid. - // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. - // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. - return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got %q", value)} - } + // Check if the last segment is a number + if hasNumericSuffix(value, "-") { + // If the last segment is a number, it's considered invalid. + // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. + // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. + return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got %q", value)} } return nil, nil }, @@ -481,3 +478,16 @@ func decodeListOfString(v cty.Value) []string { } return ret } + +func hasNumericSuffix(value, substr string) bool { + // Find the last occurrence of '-' and get the part after it + if idx := strings.LastIndex(value, substr); idx != -1 { + lastPart := value[idx+1:] + // Try to convert the last part to an integer. + if _, err := strconv.Atoi(lastPart); err == nil { + return true + } + } + // Return false if no '-' is found or if the last part isn't numeric + return false +} diff --git a/internal/backend/remote-state/kubernetes/backend_test.go b/internal/backend/remote-state/kubernetes/backend_test.go index b86125aa53ea..699dc1609893 100644 --- a/internal/backend/remote-state/kubernetes/backend_test.go +++ b/internal/backend/remote-state/kubernetes/backend_test.go @@ -197,3 +197,24 @@ func cleanupK8sResources(t *testing.T) { t.Fatal(errs) } } + +func Test_hasNumericSuffix(t *testing.T) { + tests := []struct { + input string + expected bool + }{ + {"my-secret-123", true}, + {"my-secret-abcd", false}, + {"nodashhere", false}, + {"another-secret-45abc", false}, + {"some-thing-1", true}, + {"some-thing-1-23", true}, + } + + for _, tt := range tests { + isNumeric := hasNumericSuffix(tt.input, "-") + if isNumeric != tt.expected { + t.Errorf("expected %v, got %v for input %s", tt.expected, isNumeric, tt.input) + } + } +} From 8044f69e94cc1daa728faeaa77134f27047553f6 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Fri, 20 Sep 2024 09:51:27 -0400 Subject: [PATCH 8/8] chore: move validation to configure --- .../remote-state/kubernetes/backend.go | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/internal/backend/remote-state/kubernetes/backend.go b/internal/backend/remote-state/kubernetes/backend.go index dabf66fbf206..cd2f61ee44d6 100644 --- a/internal/backend/remote-state/kubernetes/backend.go +++ b/internal/backend/remote-state/kubernetes/backend.go @@ -22,7 +22,6 @@ import ( "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/backend/backendbase" "github.com/hashicorp/terraform/internal/configs/configschema" @@ -55,20 +54,9 @@ func New() backend.Backend { Schema: &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "secret_suffix": { - Type: schema.TypeString, + Type: cty.String, Required: true, Description: "Suffix used when creating the secret. The secret will be named in the format: `tfstate-{workspace}-{secret_suffix}`. Note that the backend may append its own numeric index to the secret name when chunking large state files into multiple secrets. In this case, there will be multiple secrets named in the format: `tfstate-{workspace}-{secret_suffix}-{index}`.", - ValidateFunc: func(v interface{}, s string) ([]string, []error) { - value := v.(string) - // Check if the last segment is a number - if hasNumericSuffix(value, "-") { - // If the last segment is a number, it's considered invalid. - // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. - // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. - return nil, []error{fmt.Errorf("secret_suffix must not end with '-', got %q", value)} - } - return nil, nil - }, }, "labels": { Type: cty.Map(cty.String), @@ -336,7 +324,17 @@ func (b *Backend) Configure(configVal cty.Value) tfdiags.Diagnostics { ns := data.String("namespace") b.namespace = ns + b.nameSuffix = data.String("secret_suffix") + if hasNumericSuffix(b.nameSuffix, "-") { + // If the last segment is a number, it's considered invalid. + // The backend automatically appends its own numeric suffix when chunking large state files into multiple secrets. + // Allowing a user-defined numeric suffix could cause conflicts with this mechanism. + return backendbase.ErrorAsDiagnostics( + fmt.Errorf("secret_suffix must not end with '-', got %q", b.nameSuffix), + ) + } + b.config = cfg return nil