From bc299cafb8f17fe73b5fbba3e96fb98869d2bf8b Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Thu, 27 Feb 2025 21:28:48 +0530 Subject: [PATCH] Add warning when variable interpolation is used for auth fields (#2399) ## Changes This PR adds a warning which gives users clear guidance when they try to use variable interpolation for an auth field. ## Tests Modify existing acceptance test. --- acceptance/bundle/debug/out.stderr.txt | 1 + acceptance/bundle/variables/host/output.txt | 16 +++- .../validate/interpolation_in_auth_config.go | 77 +++++++++++++++++++ bundle/phases/initialize.go | 1 + libs/auth/env.go | 14 ++++ libs/auth/env_test.go | 39 ++++++++++ libs/dyn/dynvar/ref.go | 4 + 7 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 bundle/config/validate/interpolation_in_auth_config.go diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index e763cb6ff..147b33a45 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -17,6 +17,7 @@ 10:07:59 Debug: Apply pid=12345 mutator= 10:07:59 Info: Phase: initialize pid=12345 10:07:59 Debug: Apply pid=12345 mutator=validate:AllResourcesHaveValues +10:07:59 Debug: Apply pid=12345 mutator=validate:interpolation_in_auth_config 10:07:59 Debug: Apply pid=12345 mutator=RewriteSyncPaths 10:07:59 Debug: Apply pid=12345 mutator=SyncDefaultPath 10:07:59 Debug: Apply pid=12345 mutator=SyncInferRoot diff --git a/acceptance/bundle/variables/host/output.txt b/acceptance/bundle/variables/host/output.txt index df0a4527a..eef4a08bf 100644 --- a/acceptance/bundle/variables/host/output.txt +++ b/acceptance/bundle/variables/host/output.txt @@ -1,5 +1,12 @@ >>> errcode [CLI] bundle validate -o json +Warning: Variable interpolation is not supported for fields that configure authentication + at workspace.host + in databricks.yml:10:9 + +Interpolation is not supported for the field workspace.host. Please set +the DATABRICKS_HOST environment variable if you wish to configure this field at runtime. + Error: failed during request visitor: parse "https://${var.host}": invalid character "{" in host name { @@ -27,6 +34,13 @@ Error: failed during request visitor: parse "https://${var.host}": invalid chara Exit code: 1 >>> errcode [CLI] bundle validate +Warning: Variable interpolation is not supported for fields that configure authentication + at workspace.host + in databricks.yml:10:9 + +Interpolation is not supported for the field workspace.host. Please set +the DATABRICKS_HOST environment variable if you wish to configure this field at runtime. + Error: failed during request visitor: parse "https://${var.host}": invalid character "{" in host name Name: host @@ -34,6 +48,6 @@ Target: default Workspace: Host: ${var.host} -Found 1 error +Found 1 error and 1 warning Exit code: 1 diff --git a/bundle/config/validate/interpolation_in_auth_config.go b/bundle/config/validate/interpolation_in_auth_config.go new file mode 100644 index 000000000..83d6c6ae1 --- /dev/null +++ b/bundle/config/validate/interpolation_in_auth_config.go @@ -0,0 +1,77 @@ +package validate + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/auth" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" +) + +type noInterpolationInAuthConfig struct{} + +func NoInterpolationInAuthConfig() bundle.Mutator { + return &noInterpolationInAuthConfig{} +} + +func (f *noInterpolationInAuthConfig) Name() string { + return "validate:interpolation_in_auth_config" +} + +func (f *noInterpolationInAuthConfig) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + authFields := []string{ + // Generic attributes. + "host", "profile", "auth_type", "metadata_service_url", + + // OAuth specific attributes. + "client_id", + + // Google specific attributes. + "google_service_account", + + // Azure specific attributes. + "azure_resource_id", "azure_use_msi", "azure_client_id", "azure_tenant_id", + "azure_environment", "azure_login_app_id", + } + + diags := diag.Diagnostics{} + + for _, fieldName := range authFields { + p := dyn.NewPath(dyn.Key("workspace"), dyn.Key(fieldName)) + v, err := dyn.GetByPath(b.Config.Value(), p) + if dyn.IsNoSuchKeyError(err) { + continue + } + if err != nil { + return diag.FromErr(err) + } + + if v.Kind() == dyn.KindInvalid || v.Kind() == dyn.KindNil { + continue + } + + vv := v.MustString() + + // Check if the field contains interpolation. + if dynvar.ContainsVariableReference(vv) { + envVar, ok := auth.GetEnvFor(fieldName) + if !ok { + continue + } + + diags = append(diags, diag.Diagnostic{ + Severity: diag.Warning, + Summary: "Variable interpolation is not supported for fields that configure authentication", + Detail: fmt.Sprintf(`Interpolation is not supported for the field %s. Please set +the %s environment variable if you wish to configure this field at runtime.`, p.String(), envVar), + Locations: v.Locations(), + Paths: []dyn.Path{p}, + }) + } + } + + return diags +} diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index fef38bd28..1da5b61f4 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -26,6 +26,7 @@ func Initialize(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return bundle.ApplySeq(ctx, b, validate.AllResourcesHaveValues(), + validate.NoInterpolationInAuthConfig(), // Update all path fields in the sync block to be relative to the bundle root path. mutator.RewriteSyncPaths(), diff --git a/libs/auth/env.go b/libs/auth/env.go index c58cc53e3..5c0d21292 100644 --- a/libs/auth/env.go +++ b/libs/auth/env.go @@ -24,3 +24,17 @@ func Env(cfg *config.Config) map[string]string { return out } + +func GetEnvFor(name string) (string, bool) { + for _, attr := range config.ConfigAttributes { + if attr.Name != name { + continue + } + if len(attr.EnvVars) == 0 { + return "", false + } + return attr.EnvVars[0], true + } + + return "", false +} diff --git a/libs/auth/env_test.go b/libs/auth/env_test.go index be1cfc7ac..850110b60 100644 --- a/libs/auth/env_test.go +++ b/libs/auth/env_test.go @@ -40,3 +40,42 @@ func TestAuthEnv(t *testing.T) { out := Env(in) assert.Equal(t, expected, out) } + +func TestGetEnvFor(t *testing.T) { + tcases := []struct { + name string + expected string + }{ + // Generic attributes. + {"host", "DATABRICKS_HOST"}, + {"profile", "DATABRICKS_CONFIG_PROFILE"}, + {"auth_type", "DATABRICKS_AUTH_TYPE"}, + {"metadata_service_url", "DATABRICKS_METADATA_SERVICE_URL"}, + + // OAuth specific attributes. + {"client_id", "DATABRICKS_CLIENT_ID"}, + + // Google specific attributes. + {"google_service_account", "DATABRICKS_GOOGLE_SERVICE_ACCOUNT"}, + + // Azure specific attributes. + {"azure_workspace_resource_id", "DATABRICKS_AZURE_RESOURCE_ID"}, + {"azure_use_msi", "ARM_USE_MSI"}, + {"azure_client_id", "ARM_CLIENT_ID"}, + {"azure_tenant_id", "ARM_TENANT_ID"}, + {"azure_environment", "ARM_ENVIRONMENT"}, + {"azure_login_app_id", "DATABRICKS_AZURE_LOGIN_APP_ID"}, + } + + for _, tcase := range tcases { + t.Run(tcase.name, func(t *testing.T) { + out, ok := GetEnvFor(tcase.name) + assert.True(t, ok) + assert.Equal(t, tcase.expected, out) + }) + } + + out, ok := GetEnvFor("notfound") + assert.False(t, ok) + assert.Empty(t, out) +} diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index ba397267a..21ec00fda 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -76,6 +76,10 @@ func IsPureVariableReference(s string) bool { return len(s) > 0 && re.FindString(s) == s } +func ContainsVariableReference(s string) bool { + return re.MatchString(s) +} + // If s is a pure variable reference, this function returns the corresponding // dyn.Path. Otherwise, it returns false. func PureReferenceToPath(s string) (dyn.Path, bool) {