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

show error information from APEL token endpoint #1123

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

maxfischer2781
Copy link
Contributor

This PR improves the information provided when getting the security token from APEL fails. Since this usually ties into an issue with third-party configuration (namely missing GOCDB entry) just having a bare KeyError when the response contains no token is arcane.

The change tries to format APEL server response and failing that formats the bare HTTP response code/message.

The new error looks like this:

Traceback (most recent call last):
  File "/opt/auditor/venv/lib64/python3.11/site-packages/auditor_apel_plugin/core.py", line 469, in get_token
    return response.json()["token"]
           ~~~~~~~~~~~~~~~^^^^^^^^^
KeyError: 'token'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/opt/auditor/venv/bin/auditor-apel-publish", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/auditor/venv/lib64/python3.11/site-packages/auditor_apel_plugin/publish.py", line 164, in main
    run(logger, config, client)
  File "/opt/auditor/venv/lib64/python3.11/site-packages/auditor_apel_plugin/publish.py", line 45, in run
    token = get_token(config)
            ^^^^^^^^^^^^^^^^^
  File "/opt/auditor/venv/lib64/python3.11/site-packages/auditor_apel_plugin/core.py", line 472, in get_token
    raise RuntimeError(
RuntimeError: could not get authentication token: Binding was not found [404 Not Found]

The previous error was just the bit up to KeyError: 'token'.

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 63.23%. Comparing base (d864bb8) to head (56e098c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1123   +/-   ##
=======================================
  Coverage   63.22%   63.23%           
=======================================
  Files          55       55           
  Lines        7177     7175    -2     
  Branches       67       66    -1     
=======================================
- Hits         4538     4537    -1     
+ Misses       2639     2638    -1     
Components Coverage Δ
apel_plugin 79.04% <0.00%> (+0.10%) ⬆️
auditor 65.36% <ø> (ø)
auditor_client 92.14% <ø> (ø)
slurm_collector 71.96% <ø> (ø)
slurm_epilog_collector 0.00% <ø> (ø)
htcondor_collector ∅ <ø> (∅)
priority_plugin 36.31% <ø> (ø)

@dirksammel
Copy link
Member

Hey @maxfischer2781,
thanks for this PR!

When looking at your code I was wondering if it would be even nicer to merge your solution with the try-except in the previous lines. Something like

try:
   response = requests.get(
            auth_url, cert=(client_cert, client_key), verify=ca_path, timeout=10
   )
   response.raise_for_status()
   return response.json()["token"]
except requests.exceptions.RequestException as e:
   logger.critical(f"Could not get authentication token - {e}")
   raise

What do you think?

@maxfischer2781
Copy link
Contributor Author

This sacrifices extracting the detailed error message ("Binding was not found") for just the HTTP error ("404 Client Error: Not Found"), but truth be told they don't seem all that useful. I think the smaller code size is worth it.

@dirksammel dirksammel merged commit 4a653fc into ALU-Schumacher:main Jan 30, 2025
57 checks passed
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.

3 participants