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

Provide keyringName configuration to OIDC CredentialsProvider lookup #41293

Merged

Conversation

ryandens
Copy link
Contributor

@ryandens ryandens commented Jun 18, 2024

Fixes #41265

  • Removes redundant null-check on value returned by CredentialsProviderFinder.find(String)
  • Modifies test to verify the appropriate keyring is provided to the CredentialsProvider test implementation
    • Took a slightly different strategy here than suggested in Quarkus OIDC CredenitalsProvider does not follow conventions of allowing a separate "bean name" and "keyring name" #41265, as concatenating the keyring name to the key would mean that the Map.get(String) call on this line would return null because the extension expects the map key to be the configured value for quarkus.oidc.credentials.client-secret.provider.key. Instead, we verify this functionality by only returning the stubbed secret in the Map if the keyring name matches our expectations. I think this is desirable as it more maps to how a production-like CredentialsProvider implementation would work.
  • Modifies the OidcCommonUtils logic to use the configured keyringName, or null to look up the OIDC client secret from the CredentialsProvider, as suggested by @sberyozkin in Quarkus OIDC CredenitalsProvider does not follow conventions of allowing a separate "bean name" and "keyring name" #41265. I wasn't sure if we should instead access this optional value by wrapping the codeblock in a Optional.isPresent check, like we do for the provider key config item, but happy to follow the suggestions of the Quarkus team.
  • Updates documentation to reflect the new config item required to use the OIDC extension with a CredentialsProvider

Let me know if there's anything else I can do!

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/oidc labels Jun 18, 2024

This comment has been minimized.

Copy link

github-actions bot commented Jun 18, 2024

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Thanks @ryandens, LGTM, can you please rebase to pick up the latest updates, like a Maven version bump to 3.9.8 ?
Please also squash, use the PR title if you don't mind as the single commit message
Thanks

This comment has been minimized.

@ryandens ryandens force-pushed the ryandens/41265/oidc-keyring-name branch from 4da9545 to e2b829e Compare June 18, 2024 18:35

This comment has been minimized.

This comment has been minimized.

@ryandens ryandens requested a review from sberyozkin June 18, 2024 19:40
@ryandens
Copy link
Contributor Author

Thank you @sberyozkin! I believe I incorporated all your suggestions, I appreciate the help!

@sberyozkin
Copy link
Member

Hi @ryandens A few more tiny suggestions are proposed and it will be ready to go, thanks

@sberyozkin sberyozkin self-requested a review June 18, 2024 21:29
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

LGTM, we can merge once a few more minor doc updates are applied

@ryandens ryandens force-pushed the ryandens/41265/oidc-keyring-name branch from 5eb96a5 to 114fd29 Compare June 18, 2024 21:32
@sberyozkin sberyozkin self-requested a review June 18, 2024 21:49
@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 18, 2024
Copy link

quarkus-bot bot commented Jun 18, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 114fd29.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Jun 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 114fd29.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit 112c7c3 into quarkusio:main Jun 19, 2024
26 checks passed
@ryandens ryandens deleted the ryandens/41265/oidc-keyring-name branch July 17, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation area/oidc triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
2 participants