Content-Length: 21365 | pFad | http://github.com/coder/terraform-provider-coder/pull/421.patch

thub.com From 38bc617a63119b6ff45dd5bb6ce7b6ce28b8583f Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Mon, 21 Jul 2025 15:17:37 +0000 Subject: [PATCH 1/6] fix: correct URL validation and centralize logic --- provider/app.go | 14 +++------ provider/app_test.go | 57 ++++++++++++++++++++++++++++++++++ provider/helpers/validation.go | 19 ++++++++++++ provider/metadata.go | 15 +++------ provider/parameter.go | 26 +++++----------- provider/parameter_test.go | 36 ++++++++++++++++----- provider/provider.go | 11 ++----- 7 files changed, 123 insertions(+), 55 deletions(-) create mode 100644 provider/helpers/validation.go diff --git a/provider/app.go b/provider/app.go index adbbf0e7..38135c0b 100644 --- a/provider/app.go +++ b/provider/app.go @@ -2,7 +2,7 @@ package provider import ( "context" - "net/url" + "github.com/coder/terraform-provider-coder/v2/provider/helpers" "regexp" "github.com/google/uuid" @@ -93,15 +93,9 @@ func appResource() *schema.Resource { Description: "A URL to an icon that will display in the dashboard. View built-in " + "icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " + "built-in icon with `\"${data.coder_workspace.me.access_url}/icon/\"`.", - ForceNew: true, - Optional: true, - ValidateFunc: func(i any, s string) ([]string, []error) { - _, err := url.Parse(s) - if err != nil { - return nil, []error{err} - } - return nil, nil - }, + ForceNew: true, + Optional: true, + ValidateFunc: helpers.ValidateURL, }, "slug": { Type: schema.TypeString, diff --git a/provider/app_test.go b/provider/app_test.go index aeb42d08..2b9a5580 100644 --- a/provider/app_test.go +++ b/provider/app_test.go @@ -478,4 +478,61 @@ func TestApp(t *testing.T) { }) } }) + + t.Run("Icon", func(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + icon string + expectError *regexp.Regexp + }{ + { + name: "Empty", + icon: "", + }, + { + name: "ValidURL", + icon: "/icon/region.svg", + }, + { + name: "InvalidURL", + icon: "/icon%.svg", + expectError: regexp.MustCompile("invalid URL escape"), + }, + } + + for _, c := range cases { + c := c + + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + config := fmt.Sprintf(` + provider "coder" {} + resource "coder_agent" "dev" { + os = "linux" + arch = "amd64" + } + resource "coder_app" "code-server" { + agent_id = coder_agent.dev.id + slug = "code-server" + display_name = "Testing" + url = "http://localhost:13337" + open_in = "slim-window" + icon = "%s" + } + `, c.icon) + + resource.Test(t, resource.TestCase{ + ProviderFactories: coderFactory(), + IsUnitTest: true, + Steps: []resource.TestStep{{ + Config: config, + ExpectError: c.expectError, + }}, + }) + }) + } + }) } diff --git a/provider/helpers/validation.go b/provider/helpers/validation.go new file mode 100644 index 00000000..e532c701 --- /dev/null +++ b/provider/helpers/validation.go @@ -0,0 +1,19 @@ +package helpers + +import ( + "fmt" + "net/url" +) + +// ValidateURL checks that the given value is a valid URL string. +// Example: for `icon = "/icon/region.svg"`, value is `/icon/region.svg` and label is `icon`. +func ValidateURL(value interface{}, label string) ([]string, []error) { + val, ok := value.(string) + if !ok { + return nil, []error{fmt.Errorf("expected %q to be a string", label)} + } + if _, err := url.Parse(val); err != nil { + return nil, []error{err} + } + return nil, nil +} diff --git a/provider/metadata.go b/provider/metadata.go index 535c700c..b9e0162c 100644 --- a/provider/metadata.go +++ b/provider/metadata.go @@ -2,8 +2,7 @@ package provider import ( "context" - "net/url" - + "github.com/coder/terraform-provider-coder/v2/provider/helpers" "github.com/google/uuid" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -56,15 +55,9 @@ func metadataResource() *schema.Resource { Description: "A URL to an icon that will display in the dashboard. View built-in " + "icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " + "built-in icon with `\"${data.coder_workspace.me.access_url}/icon/\"`.", - ForceNew: true, - Optional: true, - ValidateFunc: func(i interface{}, s string) ([]string, []error) { - _, err := url.Parse(s) - if err != nil { - return nil, []error{err} - } - return nil, nil - }, + ForceNew: true, + Optional: true, + ValidateFunc: helpers.ValidateURL, }, "daily_cost": { Type: schema.TypeInt, diff --git a/provider/parameter.go b/provider/parameter.go index c8284da1..c70f4270 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -6,7 +6,7 @@ import ( "encoding/hex" "encoding/json" "fmt" - "net/url" + "github.com/coder/terraform-provider-coder/v2/provider/helpers" "os" "regexp" "strconv" @@ -223,15 +223,9 @@ func parameterDataSource() *schema.Resource { Description: "A URL to an icon that will display in the dashboard. View built-in " + "icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " + "built-in icon with `\"${data.coder_workspace.me.access_url}/icon/\"`.", - ForceNew: true, - Optional: true, - ValidateFunc: func(i interface{}, s string) ([]string, []error) { - _, err := url.Parse(s) - if err != nil { - return nil, []error{err} - } - return nil, nil - }, + ForceNew: true, + Optional: true, + ValidateFunc: helpers.ValidateURL, }, "option": { Type: schema.TypeList, @@ -263,15 +257,9 @@ func parameterDataSource() *schema.Resource { Description: "A URL to an icon that will display in the dashboard. View built-in " + "icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " + "built-in icon with `\"${data.coder_workspace.me.access_url}/icon/\"`.", - ForceNew: true, - Optional: true, - ValidateFunc: func(i interface{}, s string) ([]string, []error) { - _, err := url.Parse(s) - if err != nil { - return nil, []error{err} - } - return nil, nil - }, + ForceNew: true, + Optional: true, + ValidateFunc: helpers.ValidateURL, }, }, }, diff --git a/provider/parameter_test.go b/provider/parameter_test.go index 9b5e76f1..c96b70e5 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -227,23 +227,19 @@ func TestParameter(t *testing.T) { data "coder_parameter" "region" { name = "Region" type = "string" - default = "2" + default = "1" option { name = "1" value = "1" icon = "/icon/code.svg" description = "Something!" } - option { - name = "2" - value = "2" - } } `, Check: func(state *terraform.ResourceState) { for key, expected := range map[string]string{ "name": "Region", - "option.#": "2", + "option.#": "1", "option.0.name": "1", "option.0.value": "1", "option.0.icon": "/icon/code.svg", @@ -665,7 +661,33 @@ data "coder_parameter" "region" { } `, ExpectError: regexp.MustCompile("ephemeral parameter requires the default property"), - }} { + }, { + Name: "InvalidIconURL", + Config: ` + data "coder_parameter" "region" { + name = "Region" + type = "string" + icon = "/icon%.svg" + } + `, + ExpectError: regexp.MustCompile("invalid URL escape"), + }, { + Name: "OptionInvalidIconURL", + Config: ` + data "coder_parameter" "region" { + name = "Region" + type = "string" + option { + name = "1" + value = "1" + icon = "/icon%.svg" + description = "Something!" + } + } + `, + ExpectError: regexp.MustCompile("invalid URL escape"), + }, + } { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() diff --git a/provider/provider.go b/provider/provider.go index 43e3a6ac..6657de74 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -2,6 +2,7 @@ package provider import ( "context" + "github.com/coder/terraform-provider-coder/v2/provider/helpers" "net/url" "reflect" "strings" @@ -26,14 +27,8 @@ func New() *schema.Provider { Optional: true, // The "CODER_AGENT_URL" environment variable is used by default // as the Access URL when generating scripts. - DefaultFunc: schema.EnvDefaultFunc("CODER_AGENT_URL", "https://mydeployment.coder.com"), - ValidateFunc: func(i interface{}, s string) ([]string, []error) { - _, err := url.Parse(s) - if err != nil { - return nil, []error{err} - } - return nil, nil - }, + DefaultFunc: schema.EnvDefaultFunc("CODER_AGENT_URL", "https://mydeployment.coder.com"), + ValidateFunc: helpers.ValidateURL, }, }, ConfigureContextFunc: func(c context.Context, resourceData *schema.ResourceData) (interface{}, diag.Diagnostics) { From de3cf097faa71bf1512cfb325e9d4dd39d0999a2 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 22 Jul 2025 11:14:17 +0000 Subject: [PATCH 2/6] fix: go imports order --- provider/app.go | 3 ++- provider/metadata.go | 4 +++- provider/parameter.go | 3 ++- provider/provider.go | 3 ++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/provider/app.go b/provider/app.go index 38135c0b..52996a72 100644 --- a/provider/app.go +++ b/provider/app.go @@ -2,13 +2,14 @@ package provider import ( "context" - "github.com/coder/terraform-provider-coder/v2/provider/helpers" "regexp" "github.com/google/uuid" "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + + "github.com/coder/terraform-provider-coder/v2/provider/helpers" ) var ( diff --git a/provider/metadata.go b/provider/metadata.go index b9e0162c..5ed6d478 100644 --- a/provider/metadata.go +++ b/provider/metadata.go @@ -2,11 +2,13 @@ package provider import ( "context" - "github.com/coder/terraform-provider-coder/v2/provider/helpers" + "github.com/google/uuid" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "golang.org/x/xerrors" + + "github.com/coder/terraform-provider-coder/v2/provider/helpers" ) func metadataResource() *schema.Resource { diff --git a/provider/parameter.go b/provider/parameter.go index c70f4270..2d3ea413 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -6,7 +6,6 @@ import ( "encoding/hex" "encoding/json" "fmt" - "github.com/coder/terraform-provider-coder/v2/provider/helpers" "os" "regexp" "strconv" @@ -19,6 +18,8 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/mitchellh/mapstructure" "golang.org/x/xerrors" + + "github.com/coder/terraform-provider-coder/v2/provider/helpers" ) var ( diff --git a/provider/provider.go b/provider/provider.go index 6657de74..a0ef63f9 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -2,7 +2,6 @@ package provider import ( "context" - "github.com/coder/terraform-provider-coder/v2/provider/helpers" "net/url" "reflect" "strings" @@ -11,6 +10,8 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "golang.org/x/xerrors" + + "github.com/coder/terraform-provider-coder/v2/provider/helpers" ) type config struct { From e46a0a7427fda655858285dbaeaa7f649a829190 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 22 Jul 2025 11:39:30 +0000 Subject: [PATCH 3/6] tests: add validation URL tests --- provider/helpers/validation.go | 7 +- provider/helpers/validation_test.go | 163 ++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 provider/helpers/validation_test.go diff --git a/provider/helpers/validation.go b/provider/helpers/validation.go index e532c701..9cc21b89 100644 --- a/provider/helpers/validation.go +++ b/provider/helpers/validation.go @@ -5,15 +5,18 @@ import ( "net/url" ) -// ValidateURL checks that the given value is a valid URL string. +// ValidateURL validates that value is a valid URL string. +// Accepts empty strings, local file paths, file:// URLs, and http/https URLs. // Example: for `icon = "/icon/region.svg"`, value is `/icon/region.svg` and label is `icon`. -func ValidateURL(value interface{}, label string) ([]string, []error) { +func ValidateURL(value any, label string) ([]string, []error) { val, ok := value.(string) if !ok { return nil, []error{fmt.Errorf("expected %q to be a string", label)} } + if _, err := url.Parse(val); err != nil { return nil, []error{err} } + return nil, nil } diff --git a/provider/helpers/validation_test.go b/provider/helpers/validation_test.go new file mode 100644 index 00000000..6f9c310d --- /dev/null +++ b/provider/helpers/validation_test.go @@ -0,0 +1,163 @@ +package helpers + +import ( + "strings" + "testing" +) + +func TestValidateURL(t *testing.T) { + tests := []struct { + name string + value any + label string + expectError bool + errorContains string + }{ + // Valid cases + { + name: "empty string", + value: "", + label: "url", + expectError: false, + }, + { + name: "valid http URL", + value: "http://example.com", + label: "url", + expectError: false, + }, + { + name: "valid https URL", + value: "https://example.com/path", + label: "url", + expectError: false, + }, + { + name: "absolute file path", + value: "/path/to/file", + label: "url", + expectError: false, + }, + { + name: "relative file path", + value: "./file.txt", + label: "url", + expectError: false, + }, + { + name: "relative path up directory", + value: "../config.json", + label: "url", + expectError: false, + }, + { + name: "simple filename", + value: "file.txt", + label: "url", + expectError: false, + }, + { + name: "URL with query params", + value: "https://example.com/search?q=test", + label: "url", + expectError: false, + }, + { + name: "URL with fragment", + value: "https://example.com/page#section", + label: "url", + expectError: false, + }, + + // Various URL schemes that url.Parse accepts + { + name: "file URL scheme", + value: "file://github.com/path/to/file", + label: "url", + expectError: false, + }, + { + name: "ftp scheme", + value: "ftp://files.example.com/file.txt", + label: "url", + expectError: false, + }, + { + name: "mailto scheme", + value: "mailto:user@example.com", + label: "url", + expectError: false, + }, + { + name: "tel scheme", + value: "tel:+1234567890", + label: "url", + expectError: false, + }, + { + name: "data scheme", + value: "data:text/plain;base64,SGVsbG8=", + label: "url", + expectError: false, + }, + + // Invalid cases + { + name: "non-string type - int", + value: 123, + label: "url", + expectError: true, + errorContains: "expected \"url\" to be a string", + }, + { + name: "non-string type - nil", + value: nil, + label: "config_url", + expectError: true, + errorContains: "expected \"config_url\" to be a string", + }, + { + name: "invalid URL with spaces", + value: "http://example .com", + label: "url", + expectError: true, + errorContains: "invalid character", + }, + { + name: "malformed URL", + value: "http://[::1:80", + label: "endpoint", + expectError: true, + errorContains: "missing ']'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + warnings, errors := ValidateURL(tt.value, tt.label) + + if tt.expectError { + if len(errors) == 0 { + t.Errorf("expected an error but got none") + return + } + + if tt.errorContains != "" { + errorStr := errors[0].Error() + if !strings.Contains(errorStr, tt.errorContains) { + t.Errorf("expected error to contain %q, got %q", tt.errorContains, errorStr) + } + } + } else { + if len(errors) > 0 { + t.Errorf("expected no errors but got: %v", errors) + } + + // Should always return nil for warnings + if warnings != nil { + t.Errorf("expected warnings to be nil, got %v", warnings) + } + } + }) + } +} From 1907d5a3650c2e4c13fe8c282f4ed747dd1c5e53 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 22 Jul 2025 11:45:11 +0000 Subject: [PATCH 4/6] revert: parameter_test ValidDefaultWithOptions test update --- provider/parameter_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/provider/parameter_test.go b/provider/parameter_test.go index c96b70e5..35f045b9 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -227,19 +227,23 @@ func TestParameter(t *testing.T) { data "coder_parameter" "region" { name = "Region" type = "string" - default = "1" + default = "2" option { name = "1" value = "1" icon = "/icon/code.svg" description = "Something!" } + option { + name = "2" + value = "2" + } } `, Check: func(state *terraform.ResourceState) { for key, expected := range map[string]string{ "name": "Region", - "option.#": "1", + "option.#": "2", "option.0.name": "1", "option.0.value": "1", "option.0.icon": "/icon/code.svg", From 30ad47ccab43196cbeb6328a8574ee3df33f4358 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 22 Jul 2025 12:08:10 +0000 Subject: [PATCH 5/6] test: update validation_test to use require --- provider/helpers/validation_test.go | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/provider/helpers/validation_test.go b/provider/helpers/validation_test.go index 6f9c310d..9e926a68 100644 --- a/provider/helpers/validation_test.go +++ b/provider/helpers/validation_test.go @@ -1,8 +1,9 @@ package helpers import ( - "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestValidateURL(t *testing.T) { @@ -137,27 +138,17 @@ func TestValidateURL(t *testing.T) { warnings, errors := ValidateURL(tt.value, tt.label) if tt.expectError { - if len(errors) == 0 { - t.Errorf("expected an error but got none") - return - } + require.NotEmpty(t, errors, "expected an error but got none") if tt.errorContains != "" { - errorStr := errors[0].Error() - if !strings.Contains(errorStr, tt.errorContains) { - t.Errorf("expected error to contain %q, got %q", tt.errorContains, errorStr) - } + require.Contains(t, errors[0].Error(), tt.errorContains) } } else { - if len(errors) > 0 { - t.Errorf("expected no errors but got: %v", errors) - } - - // Should always return nil for warnings - if warnings != nil { - t.Errorf("expected warnings to be nil, got %v", warnings) - } + require.Empty(t, errors, "expected no errors but got: %v", errors) } + + // Should always return nil for warnings + require.Nil(t, warnings, "expected warnings to be nil, got %v", warnings) }) } } From 484b5b1c89437eb4acf8068c6037229067244bf9 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 22 Jul 2025 12:11:30 +0000 Subject: [PATCH 6/6] test: update validation_test tests --- provider/helpers/validation_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/provider/helpers/validation_test.go b/provider/helpers/validation_test.go index 9e926a68..557bae41 100644 --- a/provider/helpers/validation_test.go +++ b/provider/helpers/validation_test.go @@ -138,17 +138,14 @@ func TestValidateURL(t *testing.T) { warnings, errors := ValidateURL(tt.value, tt.label) if tt.expectError { - require.NotEmpty(t, errors, "expected an error but got none") - - if tt.errorContains != "" { - require.Contains(t, errors[0].Error(), tt.errorContains) - } + require.Len(t, errors, 1, "expected an error but got none") + require.Contains(t, errors[0].Error(), tt.errorContains) } else { require.Empty(t, errors, "expected no errors but got: %v", errors) } // Should always return nil for warnings - require.Nil(t, warnings, "expected warnings to be nil, got %v", warnings) + require.Nil(t, warnings, "expected warnings to be nil but got: %v", warnings) }) } }

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!