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

vdk-dag: fix unnecessary authorization failure #3096

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

antoniivanov
Copy link
Collaborator

If you have locally configured authorizaton settings (e.g client id, refresh token and so on) which might be expired or wrong, VDK dag still tries to authenticate and generate access token even if no authentication may be required by the Control Service API. This casues the DAG jobs to fail with authentication error which is unnecssary if the API didn't require auth.

Instead we now just post a warning about the failure to get acess token. We fail later if and only if the API requires authentication . If not everything suceeeds.

Testing Done: local tests pass (they failed before for above reason)

If you have locally configured authorizaton settings (e.g client id,
refresh token and so on) which might be expired or wrong, VDK dag still
tries to authenticate and generate access token even if no
authentication may be required by the Control Service API. This casues
the DAG jobs to fail with authentication error which is unnecssary if
the API didn't require auth.

Instead we now just post a warning about the failure to get acess token.
We fail later if and only if the API requires authentication . If not
everything suceeeds.

Testing Done: local tests pass (they failed before for above reason)

Signed-off-by: Antoni Ivanov <aivanov@vmware.com>
@antoniivanov antoniivanov merged commit 1402f3a into main Feb 9, 2024
13 checks passed
@antoniivanov antoniivanov deleted the person/aivanov/dag-auth branch February 9, 2024 15:16
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