Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
36 changes: 36 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/url"
"reflect"
"sort"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -63,6 +64,8 @@ const (
InvalidConfig ConfigType = "INVALID_CONFIG"
)

var defaultScopes = []string{"all-apis"}

// Config represents configuration for Databricks Connectivity
type Config struct {
// Credentials holds an instance of Credentials Strategy to authenticate with Databricks REST APIs.
Expand Down Expand Up @@ -135,6 +138,27 @@ type Config struct {
ClientID string `name:"client_id" env:"DATABRICKS_CLIENT_ID" auth:"oauth" auth_types:"oauth-m2m"`
ClientSecret string `name:"client_secret" env:"DATABRICKS_CLIENT_SECRET" auth:"oauth,sensitive" auth_types:"oauth-m2m"`

// WARNING: This feature is still in development and may not work as expected.
//
// WARNING: Scopes support is EXPERIMENTAL and we don't guarantee backward
// compatibility for this feature. Don't use this in production workloads.
//
// Scopes is a list of OAuth scopes to request when authenticating.
// If not specified, defaults to ["all-apis"] for backwards compatibility.
// For U2M authentication, "offline_access" is automatically appended to
// include a refresh token (disable via DisableOAuthRefreshToken).
//
// Note: Setting scopes via environment variables is not supported.
// Note: The slice is sorted in-place during config resolution.
// Note: U2M flow's token cache does not support scopes yet.
Scopes []string `name:"scopes" auth:"-"`

// DisableOAuthRefreshToken controls whether the offline_access scope is requested
// during U2M OAuth authentication.
// offline_access is requested by default, causing a refresh token to be included
// in the OAuth token.
DisableOAuthRefreshToken bool `name:"disable_oauth_refresh_token" env:"DATABRICKS_DISABLE_OAUTH_REFRESH_TOKEN" auth:"-"`

// Path to the Databricks CLI (version >= 0.100.0).
DatabricksCliPath string `name:"databricks_cli_path" env:"DATABRICKS_CLI_PATH" auth_types:"databricks-cli"`

Expand Down Expand Up @@ -445,6 +469,11 @@ func (c *Config) EnsureResolved() error {
},
}
}
// Sort scopes in-place for better de-duplication in the refresh token cache,
// once scopes are supported in its cache key.
if len(c.Scopes) > 0 {
sort.Strings(c.Scopes)
}
c.resolved = true
return nil
}
Expand All @@ -460,6 +489,13 @@ func (c *Config) CanonicalHostName() string {
return c.Host
}

func (c *Config) GetScopes() []string {
if len(c.Scopes) == 0 {
return defaultScopes
}
return c.Scopes
}

func (c *Config) wrapDebug(err error) error {
debug := ConfigAttributes.DebugString(c)
if debug == "" {
Expand Down
13 changes: 13 additions & 0 deletions config/config_attribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"reflect"
"strconv"
"strings"
)

type Source struct {
Expand Down Expand Up @@ -69,6 +70,16 @@ func (a *ConfigAttribute) SetS(cfg *Config, v string) error {
return err
}
return a.Set(cfg, vv)
case reflect.Slice:
rawParts := strings.Split(v, ",")
var parts []string
for _, part := range rawParts {
trimmed := strings.TrimSpace(part)
if trimmed != "" {
parts = append(parts, trimmed)
}
}
return a.Set(cfg, parts)
default:
return fmt.Errorf("cannot set %s of unknown type %s",
a.Name, reflectKind(a.Kind))
Expand All @@ -85,6 +96,8 @@ func (a *ConfigAttribute) Set(cfg *Config, i interface{}) error {
field.SetBool(i.(bool))
case reflect.Int:
field.SetInt(int64(i.(int)))
case reflect.Slice:
field.Set(reflect.ValueOf(i.([]string)))
default:
// must extensively test with providerFixture to avoid this one
return fmt.Errorf("cannot set %s of unknown type %s", a.Name, reflectKind(a.Kind))
Expand Down
42 changes: 42 additions & 0 deletions config/config_file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package config
import (
"testing"

"github.com/databricks/databricks-sdk-go/internal/env"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -22,3 +24,43 @@ func TestConfigFileLoad(t *testing.T) {
assert.Equal(t, "%Y#X$Z", section.Key("password").String())
}
}

func TestConfigFile_Scopes(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this test? It looks redundant with the two tests we have added. If you decide to keep it, please update:

			withMockEnv(t, map[string]string{})
			t.Setenv("HOME", "testdata/scopes")

to

			withMockEnv(t, map[string]string{"HOME": "testdata/scopes"})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer keeping it as it acts as an integration test and ensures backwards compatibility (the first case checks that "all-apis" is used if no scopes are provided in the config file).

Made the change.

tests := []struct {
name string
profile string
expected []string
}{
{
name: "empty defaults to all-apis",
profile: "scope-empty",
expected: []string{"all-apis"},
},
{
name: "single scope",
profile: "scope-single",
expected: []string{"clusters"},
},
{
name: "multiple scopes sorted",
profile: "scope-multiple",
expected: []string{"clusters", "files:read", "iam:read", "jobs", "mlflow", "model-serving", "pipelines"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
env.CleanupEnvironment(t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I believe t.Setenv automatically undoes the environment setting when the test completes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed undo environment settings but its seems there are other environment variables set in the test environment that interfere with the test. I tried removing the line and the test started failing because it could not find the config file anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you investigate what is causing this? You can use os.Environ() to list all env variables set before calling env.CleanupEnvironment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I get (from here):
DATABRICKS_* env vars set: [DATABRICKS_TOKEN DATABRICKS_USERNAME DATABRICKS_PASSWORD DATABRICKS_AUTH_TYPE DATABRICKS_CONFIG_PROFILE DATABRICKS_CONFIG_FILE DATABRICKS_HOST]

DATABRICKS_CONFIG_FILE being set causes the test to fail.
Some other authentication method being configured using environment variables can cause config file loading to be skipped, so I'll continue using CleanupEnvironment.

Copy link
Contributor

@renaudhartert-db renaudhartert-db Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional, feel free to ignore] I'm OK with what you have in this PR as you're only building on the code that is already there. Though, I would prefer if to have truly isolated tests by design rather than having to rely on CleanupEnvironment. At least, I believe we could better separate the logic that we are testing so that the use of CleanupEnvironment is restricted to the part that actually reads from the environment.

For example, we could separate testing the Loader from testing how the config resolution uses it (sort + dedup). That is, we would have something like the following tests:

// config_attributes_test.go

func TestConfigAttributes_Configure(t *testing.T) {
  // Verify that slices attributes are properly parsed. This code either requires
  // using CleanupEnvironment or refactoring ConfigureAttribute to use a mock
  // environment (e.g. via a package level function that defaults to os.Getenv but
  // can be overwritten in tests. 
}
// config_test.go

type mockLoader func(cfg *Config) error

func (m mockLoader) Name() string {
  return "mockLoader"
} 

func (m mockLoader) Configure(cfg *Config) error {
  return m(cfg)
}

func TestConfig_EnsureResolved_scopeNormalization(t *testing.T) {
  testCases := []strunct{
    desc string
    scopes []string
    want []string
  }{
    // ...
  }

  for _, tc := range testCases {
    t.Run(tc.desc, func (t *testingT) {
      cfg := Config{
        Loaders: []Loader{mockLoader(func (cfg *Config) error {
          cfg.Scopes = tc.scopes
          return nil
        }))}
      }

      cfg.EnsureResolved()

      if !cmp.Equal(tc.want, cfg.Scopes) {
        t.Errorf("EnsureResolved(): got scopes %v, want %v", cfg.Scopes, tc.want) 
      }
    })
  }
}

This structure allows to test the EnsureResolved normalization no matter where the attributes are coming from (env, profile, something else...). It also removes the need for the test data file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. I've made the changes (added a package level function for mocking the environment, tested the sorting and de-duplication logic separately, and tested the config file list parsing logic separately). I have left the original config_file_test tests in because they test the actual loader and ensure it is backwards compatible (uses all-apis if scopes is empty).

t.Setenv("HOME", "testdata")

cfg := &Config{Profile: tt.profile}
err := cfg.EnsureResolved()
if err != nil {
t.Fatalf("EnsureResolved failed: %v", err)
}
if diff := cmp.Diff(tt.expected, cfg.GetScopes()); diff != "" {
t.Errorf("GetScopes mismatch (-expected +actual):\n%s", diff)
}
})
}
}
11 changes: 11 additions & 0 deletions config/testdata/.databrickscfg
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,14 @@ password = '%Y#X$Z' # comment
host = https://dbc-XXXXXXXX-YYYY.cloud.databricks.com/
username = abc
password = %Y#X$Z # comment

[scope-empty]
host = https://example.cloud.databricks.com

[scope-single]
host = https://example.cloud.databricks.com
scopes = clusters

[scope-multiple]
host = https://example.cloud.databricks.com
scopes = clusters, jobs, pipelines, iam:read, files:read, mlflow, model-serving
Loading