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

support browser authentication method #284

Merged
merged 3 commits into from Aug 28, 2022

Conversation

IDoneShaveIt
Copy link
Collaborator

Our dbt_runner expect the log messages of dbt run-operation to be in json format.
However, browser authentication scenario returns log messages as string.
So instead of failing on json parsing, I log the failure and the unsupported log message to our edr file.
This is OK because run-operation log message should be in json format as expected, so if anything is returning as string it is currently OK to ignore it.
And if something will break in the future, we will have the edr log to understand what happened and what we should change.

@Maayan-s Maayan-s linked an issue Aug 28, 2022 that may be closed by this pull request
@elongl
Copy link
Member

elongl commented Aug 28, 2022

I think this is a too broad way to handle this specific case, I really don't want to keep the except Exception trend in our code. Could you share the returned output when using browser authentication?

@IDoneShaveIt
Copy link
Collaborator Author

IDoneShaveIt commented Aug 28, 2022

@elongl can't reply your comment so starting a thread here.
I don't tend to agree in this case.
Starting to parse a specific log that might change from platform to platform is not a good solution IMO.
This method parse dbt run-operation messages which should be in json format.
Skipping those who aren't and logging them to the edr will:

  1. do the trick
  2. not throw away useful information
  3. not implement a specific support for an odd bug from dbt
  4. break in the rest of the code if something will change the assumption of json format responses

I can be more specific and except JSONDecodeError to make sure I only catch those kind of errors.
WDYT?

Anyhow this is the message:
Initiating login request with your identity provider. A browser window should have opened for you to complete the login. If you can't see it, check existing browser windows, or your OS settings. Press CTRL+C to abort and try again...

We can have a specific handle for this message, I am just not sure how stable it is..

@elongl
Copy link
Member

elongl commented Aug 28, 2022

  1. Why would it change from platform to platform? It's a dbt thing. EDIT: I now see that it depends on the warehouse.
  2. Why not handle it before doing the json.loads()?
if log.startswith('Initiating login request with your identity provider.'):
    logger.debug('Initiating authentication')
    continue

We're already seeing that these type of things don't actually do the trick, they just make it very hard to debug when a user has an actual problem. There's a reason all modern IDEs warn when you except Exception. Here's my attempt to do the inverse of that.

@elongl
Copy link
Member

elongl commented Aug 28, 2022

How does it work? Does the dbt command just hold until the browser auth is complete?

@IDoneShaveIt
Copy link
Collaborator Author

IDoneShaveIt commented Aug 28, 2022

I will shed some light here:

  1. This message created in snowflakedb/snowflake-connector-python, making it very specific to snowflake (and I guess this is why they don't return it in the regular format)
  2. The dbt command is waiting for the authentication to be complete before continue to run.
  3. Trying to parse this message specific is possible, but as I said in point 1 it is very specific to snowflake.
  4. Now when I except only JSONDecodeError, is it better, or are you still against any try except in this case?

If that a big no no for you, I can go with the specific handling and fix it later in the worst case scenario

@elongl
Copy link
Member

elongl commented Aug 28, 2022

I understand the problem better now. Thanks for elaborating. Approved.

@IDoneShaveIt IDoneShaveIt merged commit 67cad17 into master Aug 28, 2022
@IDoneShaveIt IDoneShaveIt deleted the support-browser-authentication-method branch August 28, 2022 17:49
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.

Unsupported authentication method in Snowflake
2 participants