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

Fix the section name in wait_for_oidc_server function #3415

Merged
merged 5 commits into from
May 10, 2023

Conversation

npalaska
Copy link
Member

@npalaska npalaska commented May 8, 2023

We missed this change earlier, our wait_for_oidc_server function wasn't working at all. We use [openid] as a section name in our config file but the wait_for_oidc_server function was still using [openid-connect] which results in openid not configured error and we were silently ignoring it.

@npalaska npalaska self-assigned this May 8, 2023
@npalaska npalaska added this to the v0.72 milestone May 8, 2023
@npalaska npalaska requested review from webbnh and dbutenhof May 8, 2023 21:10
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it seems that the test case you removed was flawed before, and breaks differently now that you added the missing return... do we have test coverage of the failing OIDC connect case?

Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have test coverage of the failing OIDC connect case?

I believe that Nikhil fixed the unit test so that it will now correctly detect the presence of the successful call (to the mock), and, likewise, it will detect the absence of such a call; however, apparently we don't have a test which exercises the case in which the wait fails.

It would also appear that we have no test coverage for wait_for_oidc_server().

So, @npalaska, are you up for adding some unit tests?

BTW, @npalaska, here are two more spots you might want to touch up: endpoint_configure.py and load_keycloak.sh

lib/pbench/cli/server/shell.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/test_shell_cli.py Show resolved Hide resolved
@npalaska
Copy link
Member Author

npalaska commented May 9, 2023

however, apparently we don't have a test which exercises the case in which the wait fails.

We do have a test called test_main_wait_for_oidc_server_exc that test for a generic exception raised from the function but yeah we don't have a specific test case for OpenIDClient.NotConfigured exception which is the only exception we catch and return 1.

It would also appear that we have no test coverage for wait_for_oidc_server().

do we have test coverage of the failing OIDC connect case?

I will add some test coverage for wait_for_oidc_server.

dbutenhof
dbutenhof previously approved these changes May 9, 2023
lib/pbench/cli/server/shell.py Outdated Show resolved Hide resolved
@npalaska npalaska requested a review from webbnh May 9, 2023 19:47
webbnh
webbnh previously approved these changes May 9, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, Nikhil! (💯 coverage on shell.py!!)

Remaining questions:

Or, should we just declare victory?

lib/pbench/test/unit/server/test_shell_cli.py Outdated Show resolved Hide resolved
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, Mr Jenkins has expressed their displeasure:

Traceback (most recent call last):
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/test/unit/server/auth/test_auth.py", line 456, in test_token_introspect_aud
assert str(exc.value.cause) == "Invalid audience", f"{exc.value.cause}"
AssertionError: Audience doesn't match
assert "Audience doesn't match" == 'Invalid audience'

  • Invalid audience
  • Audience doesn't match

lib/pbench/cli/server/shell.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
@dbutenhof
Copy link
Member

dbutenhof commented May 10, 2023

Unfortunately, Mr Jenkins has expressed their displeasure:

Traceback (most recent call last):
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/test/unit/server/auth/test_auth.py", line 456, in test_token_introspect_aud
assert str(exc.value.cause) == "Invalid audience", f"{exc.value.cause}"
AssertionError: Audience doesn't match
assert "Audience doesn't match" == 'Invalid audience'

  • Invalid audience

  • Audience doesn't match

For the record, though, this isn't "you" ... Siddardh's and Riya's builds failed the same way. We have a new latent bug or broken package dependency, apparently ... 😦

@npalaska npalaska requested a review from dbutenhof May 10, 2023 16:13
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a thing of beauty!

@npalaska npalaska merged commit ddc3a81 into distributed-system-analysis:main May 10, 2023
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.

3 participants