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

Added Config.GetAuthDetails #838

Merged
merged 5 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions config/auth_permutations_test.go
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func TestConfig_HostEnv(t *testing.T) {
Env: map[string]string{
"DATABRICKS_HOST": "x",
},
AssertError: fmt.Sprintf("%s. Config: host=https://x. Env: DATABRICKS_HOST", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. host=https://x\nConfiguration:\n- host=https://x (from DATABRICKS_HOST environment variable)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand All @@ -148,7 +148,7 @@ func TestConfig_TokenEnv(t *testing.T) {
Env: map[string]string{
"DATABRICKS_TOKEN": "x",
},
AssertError: fmt.Sprintf("%s. Config: token=***. Env: DATABRICKS_TOKEN", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- token=******** (from DATABRICKS_TOKEN environment variable)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand Down Expand Up @@ -180,7 +180,7 @@ func TestConfig_UserPasswordEnv(t *testing.T) {
"DATABRICKS_USERNAME": "x",
"DATABRICKS_PASSWORD": "x",
},
AssertError: fmt.Sprintf("%s. Config: username=x, password=***. Env: DATABRICKS_USERNAME, DATABRICKS_PASSWORD", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- password=******** (from DATABRICKS_PASSWORD environment variable)\n- username=x (from DATABRICKS_USERNAME environment variable)", defaultAuthBaseErrorMessage),
AssertHost: "https://x",
}.apply(t)
}
Expand Down Expand Up @@ -240,7 +240,7 @@ func TestConfig_ConflictingEnvs(t *testing.T) {
"DATABRICKS_USERNAME": "x",
"DATABRICKS_PASSWORD": "x",
},
AssertError: "validate: more than one authorization method configured: basic and pat. Config: host=x, token=***, username=x, password=***. Env: DATABRICKS_HOST, DATABRICKS_TOKEN, DATABRICKS_USERNAME, DATABRICKS_PASSWORD",
AssertError: "validate: more than one authorization method configured: basic and pat. host=x\nConfiguration:\n- host=x (from DATABRICKS_HOST environment variable)\n- password=******** (from DATABRICKS_PASSWORD environment variable)\n- token=******** (from DATABRICKS_TOKEN environment variable)\n- username=x (from DATABRICKS_USERNAME environment variable)",
}.apply(t)
}

Expand All @@ -263,7 +263,7 @@ func TestConfig_ConfigFile(t *testing.T) {
Env: map[string]string{
"DATABRICKS_CONFIG_FILE": "x",
},
AssertError: fmt.Sprintf("%s. Config: config_file=x. Env: DATABRICKS_CONFIG_FILE", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- config_file=x (from DATABRICKS_CONFIG_FILE environment variable)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand All @@ -274,7 +274,7 @@ func TestConfig_ConfigFileSkipDefaultProfileIfHostSpecified(t *testing.T) {
// This directory has a DEFAULT profile in databrickscfg
"HOME": "testdata",
},
AssertError: fmt.Sprintf("%s. Config: host=https://x", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. host=https://x\nConfiguration:\n- host=https://x (from dynamic configuration)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand Down Expand Up @@ -328,7 +328,7 @@ func TestConfig_PatFromDatabricksCfg_NohostProfile(t *testing.T) {
"HOME": "testdata",
"DATABRICKS_CONFIG_PROFILE": "nohost",
},
AssertError: fmt.Sprintf("%s. Config: token=***, profile=nohost. Env: DATABRICKS_CONFIG_PROFILE", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- profile=nohost (from DATABRICKS_CONFIG_PROFILE environment variable)\n- token=******** (from testdata/.databrickscfg config file)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand All @@ -339,7 +339,7 @@ func TestConfig_ConfigProfileAndToken(t *testing.T) {
"DATABRICKS_CONFIG_PROFILE": "nohost",
"HOME": "testdata",
},
AssertError: fmt.Sprintf("%s. Config: token=***, profile=nohost. Env: DATABRICKS_TOKEN, DATABRICKS_CONFIG_PROFILE", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- profile=nohost (from DATABRICKS_CONFIG_PROFILE environment variable)\n- token=******** (from DATABRICKS_TOKEN environment variable)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand All @@ -350,7 +350,7 @@ func TestConfig_ConfigProfileAndPassword(t *testing.T) {
"DATABRICKS_CONFIG_PROFILE": "nohost",
"HOME": "testdata",
},
AssertError: "validate: more than one authorization method configured: basic and pat. Config: token=***, username=x, profile=nohost. Env: DATABRICKS_USERNAME, DATABRICKS_CONFIG_PROFILE",
AssertError: "validate: more than one authorization method configured: basic and pat. \nConfiguration:\n- profile=nohost (from DATABRICKS_CONFIG_PROFILE environment variable)\n- token=******** (from testdata/.databrickscfg config file)\n- username=x (from DATABRICKS_USERNAME environment variable)",
}.apply(t)
}

Expand Down Expand Up @@ -389,7 +389,7 @@ func TestConfig_AzureCliHost_Fail(t *testing.T) {
"HOME": "testdata/azure",
"FAIL": "yes",
},
AssertError: "default auth: azure-cli: cannot get access token: This is just a failing script.\n. Config: azure_workspace_resource_id=/sub/rg/ws",
AssertError: "default auth: azure-cli: cannot get access token: This is just a failing script.\n. \nConfiguration:\n- azure_workspace_resource_id=/sub/rg/ws (from dynamic configuration)",
}.apply(t)
}

Expand All @@ -401,7 +401,7 @@ func TestConfig_AzureCliHost_AzNotInstalled(t *testing.T) {
"PATH": "whatever",
"HOME": "testdata/azure",
},
AssertError: fmt.Sprintf("%s. Config: azure_workspace_resource_id=/sub/rg/ws", defaultAuthBaseErrorMessage),
AssertError: fmt.Sprintf("%s. \nConfiguration:\n- azure_workspace_resource_id=/sub/rg/ws (from dynamic configuration)", defaultAuthBaseErrorMessage),
}.apply(t)
}

Expand All @@ -413,7 +413,7 @@ func TestConfig_AzureCliHost_PatConflict_WithConfigFilePresentWithoutDefaultProf
"PATH": testdataPath(),
"HOME": "testdata/azure",
},
AssertError: "validate: more than one authorization method configured: azure and pat. Config: token=***, azure_workspace_resource_id=/sub/rg/ws",
AssertError: "validate: more than one authorization method configured: azure and pat. \nConfiguration:\n- azure_workspace_resource_id=/sub/rg/ws (from dynamic configuration)\n- token=******** (from dynamic configuration)",
}.apply(t)
}

Expand Down Expand Up @@ -457,7 +457,7 @@ func TestConfig_AzureAndPasswordConflict(t *testing.T) { // TODO: this breaks
"HOME": "testdata/azure",
"DATABRICKS_USERNAME": "x",
},
AssertError: "validate: more than one authorization method configured: azure and basic. Config: host=x, username=x, azure_workspace_resource_id=/sub/rg/ws. Env: DATABRICKS_USERNAME",
AssertError: "validate: more than one authorization method configured: azure and basic. host=x\nConfiguration:\n- azure_workspace_resource_id=/sub/rg/ws (from dynamic configuration)\n- host=x (from dynamic configuration)\n- username=x (from DATABRICKS_USERNAME environment variable)",
}.apply(t)
}

Expand All @@ -467,7 +467,7 @@ func TestConfig_CorruptConfig(t *testing.T) {
"HOME": "testdata/corrupt",
"DATABRICKS_CONFIG_PROFILE": "DEFAULT",
},
AssertError: "resolve: testdata/corrupt/.databrickscfg has no DEFAULT profile configured. Config: profile=DEFAULT. Env: DATABRICKS_CONFIG_PROFILE",
AssertError: "resolve: testdata/corrupt/.databrickscfg has no DEFAULT profile configured. \nConfiguration:\n- profile=DEFAULT (from DATABRICKS_CONFIG_PROFILE environment variable)",
}.apply(t)
}

Expand Down
17 changes: 10 additions & 7 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ type Config struct {
// in ~/.databrickscfg.
ConfigFile string `name:"config_file" env:"DATABRICKS_CONFIG_FILE"`

GoogleServiceAccount string `name:"google_service_account" env:"DATABRICKS_GOOGLE_SERVICE_ACCOUNT" auth:"google"`
GoogleCredentials string `name:"google_credentials" env:"GOOGLE_CREDENTIALS" auth:"google,sensitive"`
GoogleServiceAccount string `name:"google_service_account" env:"DATABRICKS_GOOGLE_SERVICE_ACCOUNT" auth:"google" auth_types:"google-id"`
GoogleCredentials string `name:"google_credentials" env:"GOOGLE_CREDENTIALS" auth:"google,sensitive" auth_types:"google-credentials"`

// Azure Resource Manager ID for Azure Databricks workspace, which is exhanged for a Host
AzureResourceID string `name:"azure_workspace_resource_id" env:"DATABRICKS_AZURE_RESOURCE_ID" auth:"azure"`
AzureResourceID string `name:"azure_workspace_resource_id" env:"DATABRICKS_AZURE_RESOURCE_ID" auth:"azure" auth_types:"azure-cli,azure-msi"`

AzureUseMSI bool `name:"azure_use_msi" env:"ARM_USE_MSI" auth:"azure"`
AzureClientSecret string `name:"azure_client_secret" env:"ARM_CLIENT_SECRET" auth:"azure,sensitive"`
AzureClientID string `name:"azure_client_id" env:"ARM_CLIENT_ID" auth:"azure"`
AzureTenantID string `name:"azure_tenant_id" env:"ARM_TENANT_ID" auth:"azure"`
AzureUseMSI bool `name:"azure_use_msi" env:"ARM_USE_MSI" auth:"azure" auth_types:"azure-msi"`
AzureClientSecret string `name:"azure_client_secret" env:"ARM_CLIENT_SECRET" auth:"azure,sensitive" auth_types:"azure-client-secret"`
AzureClientID string `name:"azure_client_id" env:"ARM_CLIENT_ID" auth:"azure" auth_types:"azure-client-secret,azure-msi"`
AzureTenantID string `name:"azure_tenant_id" env:"ARM_TENANT_ID" auth:"azure" auth_types:"azure-cli,azure-client-secret"`

// AzureEnvironment (PUBLIC, USGOVERNMENT, CHINA) has specific set of API endpoints. Starting from v0.26.0,
// the environment is determined based on the workspace hostname, if it's specified.
Expand Down Expand Up @@ -147,6 +147,9 @@ type Config struct {

// HTTP request interceptor, that assigns Authorization header
auth func(r *http.Request) error

// Keep track of the source of each attribute
attrSource map[string]*Source
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}

// NewWithWorkspaceHost returns a new instance of the Config with the host set to
Expand Down
25 changes: 25 additions & 0 deletions config/config_attribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,26 @@ import (
"strconv"
)

type Source struct {
Type SourceType
Name string
}

func (s *Source) String() string {
if s.Name == "" {
return string(s.Type)
}
return fmt.Sprintf("%s %s", s.Name, s.Type)
}

type SourceType string

const (
SourceEnv SourceType = "environment variable"
SourceFile SourceType = "config file"
SourceDynamicConfig SourceType = "dynamic configuration"
)

// ConfigAttribute provides generic way to work with Config configuration
// attributes and parses `name`, `env`, and `auth` field tags.
//
Expand All @@ -16,6 +36,7 @@ type ConfigAttribute struct {
Kind reflect.Kind
EnvVars []string
Auth string
AuthTypes []string
Sensitive bool
Internal bool
num int
Expand Down Expand Up @@ -82,3 +103,7 @@ func (a *ConfigAttribute) GetString(cfg *Config) string {
field := rv.Elem().Field(a.num)
return fmt.Sprintf("%v", field.Interface())
}

func (a *ConfigAttribute) HasAuthAttribute() bool {
return a.Auth != ""
}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
76 changes: 40 additions & 36 deletions config/config_attributes.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,8 @@ var ConfigAttributes = loadAttrs()
type attributes []ConfigAttribute

func (a attributes) DebugString(cfg *Config) string {
buf := []string{}
attrsUsed := []string{}
envsUsed := []string{}
for _, attr := range a {
if attr.IsZero(cfg) {
continue
}
// Don't include internal fields in debug representation.
if attr.Internal {
continue
}
v := "***"
if !attr.Sensitive {
v = attr.GetString(cfg)
}
attrsUsed = append(attrsUsed, fmt.Sprintf("%s=%s", attr.Name, v))
for _, envName := range attr.EnvVars {
v := os.Getenv(envName)
if v == "" {
continue
}
envsUsed = append(envsUsed, envName)
}
}
if len(attrsUsed) > 0 {
buf = append(buf, fmt.Sprintf("Config: %s", strings.Join(attrsUsed, ", ")))
}
if len(envsUsed) > 0 {
buf = append(buf, fmt.Sprintf("Env: %s", strings.Join(envsUsed, ", ")))
}
return strings.Join(buf, ". ")
authDetails := cfg.GetAuthDetails(false)
return authDetails.String()
}

func (a attributes) Validate(cfg *Config) error {
Expand All @@ -53,7 +24,7 @@ func (a attributes) Validate(cfg *Config) error {
if attr.IsZero(cfg) {
continue
}
if attr.Auth == "" {
if !attr.HasAuthAttribute() {
continue
}
authsUsed[attr.Auth] = true
Expand Down Expand Up @@ -81,12 +52,21 @@ func (a attributes) Name() string {

// Configure implements Loader interface for environment variables
func (a attributes) Configure(cfg *Config) error {
for _, attr := range a {
for k := range a {
attr := &a[k]
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if !attr.IsZero(cfg) {
// don't overwtite a value previously set
continue
}
v := attr.ReadEnv()
var v string
for _, envName := range attr.EnvVars {
v = os.Getenv(envName)
if v == "" {
continue
}
cfg.SetAttrSource(attr, &Source{Type: SourceEnv, Name: envName})
break
}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if v == "" {
continue
}
Expand All @@ -99,7 +79,12 @@ func (a attributes) Configure(cfg *Config) error {
}

func (a attributes) ResolveFromStringMap(cfg *Config, m map[string]string) error {
for _, attr := range a {
return a.ResolveFromStringMapWithSource(cfg, m, &Source{Type: SourceDynamicConfig})
}

func (a attributes) ResolveFromStringMapWithSource(cfg *Config, m map[string]string, source *Source) error {
for k := range a {
attr := &a[k]
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if !attr.IsZero(cfg) {
// don't overwtite a value previously set
continue
Expand All @@ -112,12 +97,18 @@ func (a attributes) ResolveFromStringMap(cfg *Config, m map[string]string) error
if err != nil {
return err
}
cfg.SetAttrSource(attr, source)
}
return nil
}

func (a attributes) ResolveFromAnyMap(cfg *Config, m map[string]interface{}) error {
for _, attr := range a {
return a.ResolveFromAnyMapWithSource(cfg, m, &Source{Type: SourceDynamicConfig})
}

func (a attributes) ResolveFromAnyMapWithSource(cfg *Config, m map[string]interface{}, source *Source) error {
for k := range a {
attr := &a[k]
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
if !attr.IsZero(cfg) {
// don't overwtite a value previously set
continue
Expand All @@ -130,6 +121,7 @@ func (a attributes) ResolveFromAnyMap(cfg *Config, m map[string]interface{}) err
if err != nil {
return err
}
cfg.SetAttrSource(attr, source)
}
return nil
}
Expand All @@ -155,9 +147,21 @@ func loadAttrs() (attrs attributes) {
auth = ""
internal = true
}

var authTypes []string
authTypesTag := field.Tag.Get("auth_types")
if authTypesTag == "" {
if auth != "" {
authTypes = append(authTypes, auth)
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
authTypes = strings.Split(authTypesTag, ",")
}

attr := ConfigAttribute{
Name: nameTag,
Auth: auth,
AuthTypes: authTypes,
Kind: field.Type.Kind(),
Sensitive: sensitive,
Internal: internal,
Expand Down
Loading
Loading