Resolve configuration before performing verification (#890)

## 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.
This commit is contained in:
Pieter Noordhuis 2023-10-20 15:10:31 +02:00 committed by GitHub
parent ab05f8e6e7
commit d4be40520c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 163 additions and 22 deletions

View File

@ -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,
@ -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() {

View File

@ -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")
})
}

View File

@ -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()
})
}

View File

@ -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
}

View File

@ -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,
@ -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{
@ -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) {
@ -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) {

View File

@ -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"
)
@ -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
}