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

Don't attempt auth in auth profiles --skip-validate #1282

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

ilia-db
Copy link
Contributor

@ilia-db ilia-db commented Mar 13, 2024

This makes the command almost instant, no matter how many profiles cfg file has. One downside is that we don't set AuthType for profiles that don't have it defined.

We can technically infer AuthType based on ConfigAttributes tags, but their names are different from the names of actual auth providers (and some tags cover multiple providers at the same time).

This makes the command almost instant, no matter how many profiles cfg file has.
One downside is that we don't set AuthType for profiles that don't have it defined.

We can technically infer AuthType based on ConfigAttributes tags, but their
names are different from the names of actual auth providers (and some tags cover
multiple providers at the same time)
@pietern
Copy link
Contributor

pietern commented Mar 13, 2024

Do you need just the names of the profiles?

In case of Azure, they might specify only the resource ID of the workspace, and performing the Authenticate call will resolve the workspace host. If we don't, the canonical host name will be empty.

@ilia-db
Copy link
Contributor Author

ilia-db commented Mar 13, 2024

Hmm, there are couple of places we rely on profile hosts. The most problematic place might be when we attempt to login automatically based on the host from the dabs config - if no profile will match the host we won't login automatically. But auth describe will actually fix that, after it lands and we switch to using it.

We will also need to update profile selection in our login wizard, as currently we only list profiles that match the host. After this PR lands we will add "hostless" profiles to the selection (otherwise users won't be able to select them).

I think the benefits of the instant profile loading outweigh the downsides (when some profiles might not be relevant for the host, but we will still show them just in case).

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

I'm OK to optimize for latency now.

But I would prefer a mode where we can pull all the details without having to make API calls or shell out to other commands. That will need work on the SDK side to facilitate. For example to decouple auth selection from actually getting a token.

return
}
c.Host = cfg.Host
c.Host = cfg.CanonicalHostName()
c.AuthType = cfg.AuthType
Copy link
Contributor

Choose a reason for hiding this comment

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

The AuthType will only be set if explicitly set (environment of in .databrickscfg profile).

It is guaranteed to be set only if we run Authenticate to pull a token.

@pietern pietern requested a review from mgyucht March 14, 2024 11:39
@pietern
Copy link
Contributor

pietern commented Mar 14, 2024

@mgyucht Thoughts on this?

@mgyucht
Copy link
Contributor

mgyucht commented Mar 14, 2024

This seems fine to me, but I don't think we can reliably determine auth type purely based on config. There are a number of checks that happen at runtime as well (e.g. CLI is present on path) that require some requests to ensure that the profile is actually valid.

One other idea: could we use goroutines to parallelize this? Seems silly to make these requests one-by-one, they should be trivially parallelizable.

@pietern
Copy link
Contributor

pietern commented Mar 18, 2024

@mgyucht This is already parallelized. The latency that this fixes is the latency for a single profile to call az (for ex).

@ilia-db
Copy link
Contributor Author

ilia-db commented Mar 18, 2024

Blocked by the #1244

Currently the PR will break the automatic login flow of the Databricks VSCode extension (but only for the edge case when there are profiles without hosts). We first need to wait for auth describe and switch to using it on the extension side.

@ilia-db ilia-db added this pull request to the merge queue Apr 5, 2024
Merged via the queue into main with commit 338fe1f Apr 5, 2024
4 checks passed
@ilia-db ilia-db deleted the profiles-skip-validate branch April 5, 2024 10:25
andrewnester added a commit that referenced this pull request Apr 10, 2024
CLI:
 * Don't attempt auth in `auth profiles --skip-validate` ([#1282](#1282)).
 * Fixed typo in error template for auth describe ([#1341](#1341)).

Bundles:
 * Correctly transform libraries in for_each_task block ([#1340](#1340)).
 * Do not emit warning on YAML anchor blocks ([#1354](#1354)).
 * Fixed pre-init script order ([#1348](#1348)).
 * Execute preinit after entry point to make sure scripts are loaded ([#1351](#1351)).

Dependency updates:
 * Bump internal terraform provider version to `1.39` ([#1339](#1339)).
 * Bump golang.org/x/term from 0.18.0 to 0.19.0 ([#1343](#1343)).
 * Bump github.com/hashicorp/hc-install from 0.6.3 to 0.6.4 ([#1344](#1344)).
 * Bump golang.org/x/mod from 0.16.0 to 0.17.0 ([#1345](#1345)).
 * Bump golang.org/x/oauth2 from 0.18.0 to 0.19.0 ([#1347](#1347)).
 * Bump golang.org/x/sync from 0.6.0 to 0.7.0 ([#1346](#1346)).
@andrewnester andrewnester mentioned this pull request Apr 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2024
CLI:
* Don't attempt auth in `auth profiles --skip-validate`
([#1282](#1282)).
* Fixed typo in error template for auth describe
([#1341](#1341)).

Bundles:
* Correctly transform libraries in for_each_task block
([#1340](#1340)).
* Do not emit warning on YAML anchor blocks
([#1354](#1354)).
* Fixed pre-init script order
([#1348](#1348)).
* Execute preinit after entry point to make sure scripts are loaded
([#1351](#1351)).


Dependency updates:
* Bump internal terraform provider version to `1.39`
([#1339](#1339)).
* Bump golang.org/x/term from 0.18.0 to 0.19.0
([#1343](#1343)).
* Bump github.com/hashicorp/hc-install from 0.6.3 to 0.6.4
([#1344](#1344)).
* Bump golang.org/x/mod from 0.16.0 to 0.17.0
([#1345](#1345)).
* Bump golang.org/x/oauth2 from 0.18.0 to 0.19.0
([#1347](#1347)).
* Bump golang.org/x/sync from 0.6.0 to 0.7.0
([#1346](#1346)).
@andrewnester andrewnester mentioned this pull request Apr 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2024
CLI:
* Don't attempt auth in `auth profiles --skip-validate`
([#1282](#1282)).
* Fixed typo in error template for auth describe
([#1341](#1341)).

Bundles:
* Correctly transform libraries in for_each_task block
([#1340](#1340)).
* Do not emit warning on YAML anchor blocks
([#1354](#1354)).
* Fixed pre-init script order
([#1348](#1348)).
* Execute preinit after entry point to make sure scripts are loaded
([#1351](#1351)).

Dependency updates:
* Bump internal terraform provider version to `1.39`
([#1339](#1339)).
* Bump golang.org/x/term from 0.18.0 to 0.19.0
([#1343](#1343)).
* Bump github.com/hashicorp/hc-install from 0.6.3 to 0.6.4
([#1344](#1344)).
* Bump golang.org/x/mod from 0.16.0 to 0.17.0
([#1345](#1345)).
* Bump golang.org/x/oauth2 from 0.18.0 to 0.19.0
([#1347](#1347)).
* Bump golang.org/x/sync from 0.6.0 to 0.7.0
([#1346](#1346)).

## Changes
<!-- Summary of your changes that are easy to understand -->

## Tests
<!-- How is this tested? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants