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

[Bug] Silent SSO token expiration #1072

Merged
merged 16 commits into from
Jun 24, 2024
Merged

Conversation

mikealfare
Copy link
Contributor

resolves #851

Problem

Expired SSO tokens fail silently.

Solution

Catch the specific exception for an expired SSO token and raise it so the user is aware of why the connection is failing.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@mikealfare mikealfare self-assigned this Jun 5, 2024
@cla-bot cla-bot bot added the cla:yes label Jun 5, 2024
@mikealfare mikealfare assigned McKnight-42 and unassigned mikealfare Jun 5, 2024
@McKnight-42
Copy link
Contributor

McKnight-42 commented Jun 11, 2024

looking into the further up swap to a Database Error

looking at open method in connections.py we get to

retryable_exceptions = [
            InternalError,
            InternalServerError,
            ServiceUnavailableError,
            GatewayTimeoutError,
            RequestTimeoutError,
            BadGatewayError,
            OtherHTTPRetryableError,
            BindUploadError,
        ]
        # these two options are for backwards compatibility
        if creds.retry_all:
            retryable_exceptions = [Error]
        elif creds.retry_on_database_errors:
            retryable_exceptions.insert(0, DatabaseError)

        return cls.retry_connection(
            connection,
            connect=connect,
            logger=logger,
            retry_limit=creds.connect_retries,
            retry_timeout=timeout if timeout is not None else exponential_backoff,
            retryable_exceptions=retryable_exceptions,
        )

falling back to dbt-adapters and looking at the method defined there as part of BaseConnectionManager failure seems to happen here

except Exception as e:
            connection.handle = None
            connection.state = ConnectionState.FAIL  # type: ignore
            raise FailedToConnectError(str(e))

applying breakpoint here and checking type and str of e shows:

type(e)
<class 'dbt.adapters.exceptions.connection.FailedToConnectError'>
str: str(e)
'Database Error\n  This error occurs when authentication has expired. Please reauth with your auth provider.'

I'm looking into where the Database Error is getting added on.

the current idea is class DbtDatabaseError which has a MESSAGE = Database Error field and is the direct parent of FailedToConnectError.

@@ -88,3 +90,28 @@ def models(self):
def test_snowflake_basic(self, project):
run_dbt()
check_relations_equal(project.adapter, ["MODEL_3", "MODEL_4"])


class TestSnowflakeOAuthExpiration:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the test run logs I think the issue here is that as part of test setup we are creating a connection with the database so it's failing in test setup and not in the test_token_expiration method

Copy link
Contributor

Choose a reason for hiding this comment

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

updated the test, it hits the error then for some reason continues on into a RunTimeError as well

@McKnight-42 McKnight-42 marked this pull request as ready for review June 24, 2024 15:42
@McKnight-42 McKnight-42 requested a review from a team as a code owner June 24, 2024 15:42
@McKnight-42 McKnight-42 merged commit 2576a08 into main Jun 24, 2024
18 checks passed
@McKnight-42 McKnight-42 deleted the bug/851-silent-sso-token-expiration branch June 24, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-1049] [Bug] Snowflake SSO Token expiration not surfaced in logs
3 participants