versatile-data-kit icon indicating copy to clipboard operation
versatile-data-kit copied to clipboard

vdk-control-cli: Added a generic exception to address token expiry and re-login

Open vivekdf opened this issue 1 year ago • 2 comments

Fix for: https://github.com/vmware/versatile-data-kit/issues/3030

Added check for handing exception and logging in information, in scenarios where token is expired or corrupted.

vivekdf avatar Feb 22 '24 04:02 vivekdf

I took a closer look at the issue.

I've not written good enough steps to reproduce for which I apologize. Those steps are more clear: To reproduce the issue you need to set the wrong or expired API TOKEN

export VDK_API_TOKEN=wrong-token 
export VDK_API_TOKEN_AUTHORIZATION_SERVER_URL=https://console.cloud.vmware.com/csp/gateway/am/api/auth/api-tokens/authorize

And run some vdk commands. This would reproduce the issue.

antoniivanov avatar Feb 23 '24 16:02 antoniivanov

Dear @*antoniivanov: When the issue was reproduced : the call is entering the decorated method : and the following debug sentence does get logged : *

Call function vdk.internal.control.command_groups.job.secrets.update_secrets

Printing of this log is very evident of the API throwing an error: Predominantly an authentication error (as in this case). given that the refresh token is passed to the rest API : Hence having a generic Exception becomes fairly pertinent.

Thanks again !

Vivek -

On Fri, Feb 23, 2024 at 7:58 AM Antoni Ivanov @.***> wrote:

@.**** commented on this pull request.

In projects/vdk-control-cli/src/vdk/internal/control/rest_lib/rest_client_errors.py https://github.com/vmware/versatile-data-kit/pull/3154#discussion_r1500865998 :

@@ -87,5 +87,18 @@ def decorated(*args, **kwargs): "and points to the correct host.", ) raise vdk_ex from ex

  •        except Exception as ex:
    
  •            log.debug(
    
  •                f"An gemeral Exception occurred in {fn.__module__}.{fn.__name__}",
    
  •                f"The Exception class was: {ex}",
    
  •            )
    
  •            vdk_ex = VDKException(
    

We are only wrapping the original exception. Is there any benefit to do that?

We want to avoid wrapping exceptions unless needed because that leads to a bigger logs (since now we have the original and he previous expcetion), user cannot handle it as easily (catch it).

Also assuming all exceptions are authentication-related is certainly not correct. There might be other exceptions - bugs or the command may fail for other reason. Also they may already be hanled and formatted appropriately

This decorator was created to catch rest API-specific exceptions and make the errors more user friendly to the user.

— Reply to this email directly, view it on GitHub https://github.com/vmware/versatile-data-kit/pull/3154#pullrequestreview-1898440456, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASOHGYH4YFPZSWUT7BO5DTDYVC4C5AVCNFSM6AAAAABDUHJIBGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJYGQ2DANBVGY . You are receiving this because you authored the thread.Message ID: @.***>

vivekdf avatar Feb 24 '24 04:02 vivekdf

Closing due to inactivity

DeltaMichael avatar Mar 26 '24 11:03 DeltaMichael

the code has been provided and needs to be reviewed and closed : @antoniivanov .

lordluvu avatar Mar 30 '24 18:03 lordluvu

Kindly approve the MR !

lordluvu avatar Mar 31 '24 14:03 lordluvu

@antoniivanov : Can you kindly review the code please! thanks !!!

lordluvu avatar Apr 05 '24 17:04 lordluvu

Addred the request as suggested by @DeltaMichael

lordluvu avatar Apr 10 '24 16:04 lordluvu

Please provide more information about the exact problem you're trying to solve. Your commit message and PR description should follow this template. https://github.com/vmware/versatile-data-kit/blob/main/support/git-commit-template.txt

Please check our contributing guide for more information. https://github.com/vmware/versatile-data-kit/blob/main/CONTRIBUTING.md#submit-a-pull-request

Please don't leak VMWare internals (ticket numbers, internal names of products, etc.) to the public repo. Your PR title should follow

<artifact/project>: <put a good 1-line overview here, no more than 50 characters!>

So in this case, it should be something like

vdk-control-cli: Added a generic exception to address token expiry and re-login

HAve addressed the review !

lordluvu avatar Apr 10 '24 16:04 lordluvu