Skip to content

Commit

Permalink
Resolve configuration before performing verification (#890)
Browse files Browse the repository at this point in the history
## Changes

If a bundle configuration specifies a workspace host, and the user
specifies a profile to use, we perform a check to confirm that the
workspace host in the bundle configuration and the workspace host from
the profile are identical. If they are not, we return an error. The
check was introduced in #571.

Previously, the code included an assumption that the client
configuration was already loaded from the environment prior to
performing the check. This was not the case, and as such if the user
intended to use a non-default path to `.databrickscfg`, this path was
not used when performing the check.

The fix does the following:
* Resolve the configuration prior to performing the check.
* Don't treat the configuration file not existing as an error.
* Add unit tests.

Fixes #884.

## Tests

Unit tests and manual confirmation.
  • Loading branch information
pietern authored Oct 20, 2023
1 parent ab05f8e commit d4be405
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 22 deletions.
15 changes: 12 additions & 3 deletions bundle/config/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (s User) MarshalJSON() ([]byte, error) {
}

func (w *Workspace) Client() (*databricks.WorkspaceClient, error) {
cfg := databricks.Config{
cfg := config.Config{
// Generic
Host: w.Host,
Profile: w.Profile,
Expand Down Expand Up @@ -114,14 +114,23 @@ func (w *Workspace) Client() (*databricks.WorkspaceClient, error) {
}
}

if w.Profile != "" && w.Host != "" {
// Resolve the configuration. This is done by [databricks.NewWorkspaceClient] as well, but here
// we need to verify that a profile, if loaded, matches the host configured in the bundle.
err := cfg.EnsureResolved()
if err != nil {
return nil, err
}

// Now that the configuration is resolved, we can verify that the host in the bundle configuration
// is identical to the host associated with the selected profile.
if w.Host != "" && w.Profile != "" {
err := databrickscfg.ValidateConfigAndProfileHost(&cfg, w.Profile)
if err != nil {
return nil, err
}
}

return databricks.NewWorkspaceClient(&cfg)
return databricks.NewWorkspaceClient((*databricks.Config)(&cfg))
}

func init() {
Expand Down
144 changes: 144 additions & 0 deletions bundle/config/workspace_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package config

import (
"context"
"io/fs"
"path/filepath"
"runtime"
"testing"

"github.com/databricks/cli/internal/testutil"
"github.com/databricks/cli/libs/databrickscfg"
"github.com/databricks/databricks-sdk-go/config"
"github.com/stretchr/testify/assert"
)

func setupWorkspaceTest(t *testing.T) string {
testutil.CleanupEnvironment(t)

home := t.TempDir()
t.Setenv("HOME", home)
if runtime.GOOS == "windows" {
t.Setenv("USERPROFILE", home)
}

return home
}

func TestWorkspaceResolveProfileFromHost(t *testing.T) {
// If only a workspace host is specified, try to find a profile that uses
// the same workspace host (unambiguously).
w := Workspace{
Host: "https://abc.cloud.databricks.com",
}

t.Run("no config file", func(t *testing.T) {
setupWorkspaceTest(t)
_, err := w.Client()
assert.NoError(t, err)
})

t.Run("default config file", func(t *testing.T) {
setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
Profile: "default",
Host: "https://abc.cloud.databricks.com",
Token: "123",
})

client, err := w.Client()
assert.NoError(t, err)
assert.Equal(t, "default", client.Config.Profile)
})

t.Run("custom config file", func(t *testing.T) {
home := setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
ConfigFile: filepath.Join(home, "customcfg"),
Profile: "custom",
Host: "https://abc.cloud.databricks.com",
Token: "123",
})

t.Setenv("DATABRICKS_CONFIG_FILE", filepath.Join(home, "customcfg"))
client, err := w.Client()
assert.NoError(t, err)
assert.Equal(t, "custom", client.Config.Profile)
})
}

func TestWorkspaceVerifyProfileForHost(t *testing.T) {
// If both a workspace host and a profile are specified,
// verify that the host configured in the profile matches
// the host configured in the bundle configuration.
w := Workspace{
Host: "https://abc.cloud.databricks.com",
Profile: "abc",
}

t.Run("no config file", func(t *testing.T) {
setupWorkspaceTest(t)
_, err := w.Client()
assert.ErrorIs(t, err, fs.ErrNotExist)
})

t.Run("default config file with match", func(t *testing.T) {
setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
Profile: "abc",
Host: "https://abc.cloud.databricks.com",
})

_, err := w.Client()
assert.NoError(t, err)
})

t.Run("default config file with mismatch", func(t *testing.T) {
setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
Profile: "abc",
Host: "https://def.cloud.databricks.com",
})

_, err := w.Client()
assert.ErrorContains(t, err, "config host mismatch")
})

t.Run("custom config file with match", func(t *testing.T) {
home := setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
ConfigFile: filepath.Join(home, "customcfg"),
Profile: "abc",
Host: "https://abc.cloud.databricks.com",
})

t.Setenv("DATABRICKS_CONFIG_FILE", filepath.Join(home, "customcfg"))
_, err := w.Client()
assert.NoError(t, err)
})

t.Run("custom config file with mismatch", func(t *testing.T) {
home := setupWorkspaceTest(t)

// This works if there is a config file with a matching profile.
databrickscfg.SaveToProfile(context.Background(), &config.Config{
ConfigFile: filepath.Join(home, "customcfg"),
Profile: "abc",
Host: "https://def.cloud.databricks.com",
})

t.Setenv("DATABRICKS_CONFIG_FILE", filepath.Join(home, "customcfg"))
_, err := w.Client()
assert.ErrorContains(t, err, "config host mismatch")
})
}
2 changes: 1 addition & 1 deletion cmd/root/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestBundleConfigureWithNonExistentProfileFlag(t *testing.T) {
cmd.Flag("profile").Value.Set("NOEXIST")

b := setup(t, cmd, "https://x.com")
assert.PanicsWithError(t, "no matching config profiles found", func() {
assert.Panics(t, func() {
b.WorkspaceClient()
})
}
Expand Down
1 change: 1 addition & 0 deletions libs/databrickscfg/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func (l profileFromHostLoader) Configure(cfg *config.Config) error {
return fmt.Errorf("%s %s profile: %w", configFile.Path(), match.Name(), err)
}

cfg.Profile = match.Name()
return nil
}

Expand Down
18 changes: 3 additions & 15 deletions libs/databrickscfg/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TestLoaderErrorsOnInvalidFile(t *testing.T) {
assert.ErrorContains(t, err, "unclosed section: ")
}

func TestLoaderSkipssNoMatchingHost(t *testing.T) {
func TestLoaderSkipsNoMatchingHost(t *testing.T) {
cfg := config.Config{
Loaders: []config.Loader{
ResolveProfileFromHost,
Expand All @@ -73,20 +73,6 @@ func TestLoaderSkipssNoMatchingHost(t *testing.T) {
assert.Empty(t, cfg.Token)
}

func TestLoaderConfiguresMatchingHost(t *testing.T) {
cfg := config.Config{
Loaders: []config.Loader{
ResolveProfileFromHost,
},
ConfigFile: "testdata/databrickscfg",
Host: "https://default/?foo=bar",
}

err := cfg.EnsureResolved()
assert.NoError(t, err)
assert.Equal(t, "default", cfg.Token)
}

func TestLoaderMatchingHost(t *testing.T) {
cfg := config.Config{
Loaders: []config.Loader{
Expand All @@ -99,6 +85,7 @@ func TestLoaderMatchingHost(t *testing.T) {
err := cfg.EnsureResolved()
assert.NoError(t, err)
assert.Equal(t, "default", cfg.Token)
assert.Equal(t, "DEFAULT", cfg.Profile)
}

func TestLoaderMatchingHostWithQuery(t *testing.T) {
Expand All @@ -113,6 +100,7 @@ func TestLoaderMatchingHostWithQuery(t *testing.T) {
err := cfg.EnsureResolved()
assert.NoError(t, err)
assert.Equal(t, "query", cfg.Token)
assert.Equal(t, "query", cfg.Profile)
}

func TestLoaderErrorsOnMultipleMatches(t *testing.T) {
Expand Down
5 changes: 2 additions & 3 deletions libs/databrickscfg/ops.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"strings"

"github.com/databricks/cli/libs/log"
"github.com/databricks/databricks-sdk-go"
"github.com/databricks/databricks-sdk-go/config"
"gopkg.in/ini.v1"
)
Expand Down Expand Up @@ -130,17 +129,17 @@ func SaveToProfile(ctx context.Context, cfg *config.Config) error {
return configFile.SaveTo(configFile.Path())
}

func ValidateConfigAndProfileHost(cfg *databricks.Config, profile string) error {
func ValidateConfigAndProfileHost(cfg *config.Config, profile string) error {
configFile, err := config.LoadFile(cfg.ConfigFile)
if err != nil {
return fmt.Errorf("cannot parse config file: %w", err)
}

// Normalized version of the configured host.
host := normalizeHost(cfg.Host)
match, err := findMatchingProfile(configFile, func(s *ini.Section) bool {
return profile == s.Name()
})

if err != nil {
return err
}
Expand Down

0 comments on commit d4be405

Please sign in to comment.