-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Added JWT authentication to the TableauHook #51756
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
Conversation
|
You probably need to run |
|
@eladkal The problem is between
It seems like this issue is not a problem with Python>=3.10 only with < 3.10 |
|
I think we can set the provider to be Python 3.10+ only. Python 3.9 is EOL very soon and anyway we don't introduce that many changes to the tableau provider. |
|
dependency issues seem to be resolved with python3.10 when testing locally. However |
|
Can you push the changes? Lets see |
|
@dominikhei try to run: |
Already tried it, still tries to run the checks with the 3.9 ci-image. maybe its hardcoded somewhere because I can not find a way to use the 3.10 image, even though I have configured breeze to use it. |
We have a case of excluded Python 3.9 from a provider. apply similar logic as shown here: airflow/providers/cloudant/pyproject.toml Lines 52 to 63 in a4a51a0
|
|
Apart from the failing doc checks, which i'll adjust, the CI still runs with 3.9 leading to tableauserverclient now not being included as a dependency ( |
|
Looks like we need also to make changes in airflow/dev/breeze/src/airflow_breeze/global_constants.py Lines 725 to 744 in 96c48a5
might also need change in airflow/.github/workflows/test-providers.yml Lines 129 to 132 in 3198aad
We just need to read the errors to get direction of what is the issue and then look up for |
|
Might be helpful for later once we made it work, to also adjust the Contributors Guide with a section on this. |
failure is because we are missing similar: airflow/providers/cloudant/provider.yaml Lines 57 to 62 in a4a51a0
|
You will be the best person for the job. Ideally, if we can find automations I think it's better. |
|
OK looks like all cutrent failures are docs/examples and tableau tests |
Seems like The failing tests are still linked to the python version: |
|
can you fix the other tests? Sometimes handling the other errors solves the doc problems |
|
Almost :) airflow/providers/cloudant/tests/unit/cloudant/hooks/test_cloudant.py Lines 30 to 37 in 4d5846f
|
|
@eladkal the only failing static check is Do you know why this is failing now? |
we reverted dependaboat bumps, please rebase it should work now? |
|
I started [DISCUSS] Dropping Python 3.9 support |
Should we just wait until we upgrade to 3.10, as the feedback was positive or add it for 3.9 with the workarounds? (Would need to rebase due to #51930) |
|
#52072 is merged. You can now proceed without the trouble of Python 3.9 |
|
Nice! |
|
Thank you for implementing JWT authentication. This opens the door to migrating from PAT to Connected Apps. I noticed the current implementation assumes the Tableau connection already contains a valid JWT token. Is there a reason you opted for this approach over generating the JWT within the Tableau hook? As far as I'm aware, it isn't possible to generate a long-lived JWT, so users would need to regenerate the JWT quite frequently (every couple of minutes). |
Hi, let me have a look at it and I'll get back to you. |
So from my perspective, as the JWT token comes from an external identity provider, tableauserverclient can only consume pre-signed JWT's. Fetching this is dependent on the identity provider, and I don't think logic to fetch this belongs in the tableau provider. If I am not mistaken you can sign the token yourself with a secret configured in Tableau Connected Apps? But I am unsure if logic for this belongs in here. What you could do for now is build a custom task using python that generates or fetches a token and then pass it. @potiuk what is your thought? |
Thank you for sharing your thoughts. You're correct, users would need to obtain a secret from a Tableau Connected App and pass it to the operator. This does add an extra step, but one could argue that generating a PAT follows a similar pattern. It also requires the user to create a token in Tableau and then pass both its name and value into the Tableau connection in Airflow. The main difference is that a Connected App must be created by a site administrator. |
In this case you would want to store the secret to sign the token in Airflow and then generate the token in Airflow and sign it using the secret? In my opinion, this is logic that should be handled outside of Airflow. I think it would be worthwhile to get input from someone with more experience before proceeding in any way. |
I added JWT Authentication to the TableauHook. If password and login are set, it is preferred over JWT Authentication.
There are two options for using a token: Passing it as a string to the connection extras or using a path to a location on disk to a file. If both are set, the file will be used.
closes: #51453