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

Infer Azure tenant ID if not set #910

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented May 6, 2024

Changes

In order to use Azure U2M or M2M authentication with the Databricks SDK, users must request a token from the correct Entra ID instance, specifically, the same tenant as the one that the underlying workspace or account belongs to. Otherwise, Databricks will reject a user's requests. However, with Azure CLI auth, it is possible that a user is logged into multiple tenants at the same time. Currently, the SDK uses the subscription ID from the configured Azure Resource ID for the workspace when issuing the az account get-access-token command. However, when users don't specify the resource ID, the SDK simply fetches a token for the active subscription for the user. If the active subscription is in a different tenant than the workspace, users will see an error such as:

io.jsonwebtoken.IncorrectClaimException: Expected iss claim to be: https://sts.windows.net/72f988bf-86f1-41af-91ab-2d7cd011db47/, but was: https://sts.windows.net/e3fe3f22-4b98-4c04-82cc-d8817d1b17da/

This PR modifies Azure CLI and Azure SP credential providers to attempt to load the tenant ID of the workspace if not provided before authenticating. Currently, there are no unauthenticated endpoints that the tenant ID can be directly fetched from. However, the tenant ID is indirectly exposed via the redirect URL used when logging into a workspace. In this PR, we fetch the tenant ID from this endpoint and configure it if not already set.

Here, we lazily fetch the tenant ID only in the auth methods that need it. This prevents us from making any unnecessary requests if these Azure credential providers are not needed.

Tests

  • make test passing
  • make fmt applied
  • relevant integration tests applied

@mgyucht mgyucht requested a review from tanmay-db May 6, 2024 10:03

// Run a command and return the output.
func runCommand(ctx context.Context, cmd string, args []string) ([]byte, error) {
logger.Debugf(ctx, "Running command: %s %v", cmd, strings.Join(args, " "))
Copy link
Contributor

@tanmay-db tanmay-db May 8, 2024

Choose a reason for hiding this comment

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

Might be good to allow passing debug level defaulting to DEBUG.

    logMessage := fmt.Sprintf("Running command: %s %v", cmd, strings.Join(args, " "))

    // logLevel is enum for log levels, default to debug if not provided. 
    switch logLevel {
    case LogLevelDebug:
        logger.Debugf(ctx, logMessage)
    case LogLevelInfo:
        logger.Infof(ctx, logMessage)
    case LogLevelWarn:
        logger.Warnf(ctx, logMessage)
    case LogLevelError:
        logger.Errorf(ctx, logMessage)
    default:
        logger.Infof(ctx, "Unspecified log level for: %s", logMessage)
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants