-
Notifications
You must be signed in to change notification settings - Fork 224
Add NoAuthRuntimePluginV2 and deprecate NoAuthRuntimePlugin
#4415
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
base: main
Are you sure you want to change the base?
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aajtodd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If NoAuthRuntimePlugin is already registered by default and makes noAuth an available/configured auth scheme, why not just make it easier to register a custom auth scheme (option) resolver? Is it because ResolveAuthScheme is specific to each generated crate?
I feel like the user experience ought to be either of the following depending on whether they want it for specific operations or all operations:
client.operation()
.foo(...)
.bar(...)
.customize()
.config_override(config_with_no_auth_override)
.send()
.awaitor at the client level configuring auth_scheme_resolver to return the no auth option.
I see a few use cases:
- Users want to disable auth completely for all users -> model it at the service level
- Users want to disable auth for certain operations for all users -> model it at the operation level
- User's want to offer optionalAuth operations/service
- User's don't/can't model this for reasons and they want to dictate the behavior at runtime to disable auth
Ideally people are moving to (1) and (2) but for (3) and (4) I'd want it to be very explicit about what's going and (IMO) force them to setup the custom auth resolver/config and customize/override operations.
I can disagree and commit to this though if this is the desired solution for the identified use cases.
Yes, that's for one; While we can code generate a custom no-auth resolver for them, it can only wrap the default auth scheme resolver, not the one dynamically configured in runtime components. For two, we can narrow the usage of |
|
Really? I thought we enabled runtime plugins for the SDK... |
case in point, though it's configurable at codegen time |
|
@rcoh, do you still feel strongly about original recommendations or do you consider us doc-hiding FYI, we kind of have a NoAuthSchemeResolver, but its usage is restricted to protocol tests. |
|
I don't really care the exact mechanism. But:
Having a well documented cookbook about "here's how you disable auth" would probably do the trick that is linked from that error message. The If we wanted to, you could consider adding a method to |
|
Sounds good. Taking both @aajtodd and @rcoh's input into account, I'll proceed with:
|
|
Actually one more consideration—we need to make sure that a runtime plugin can enable this or there is another ay for a service to vend a "working" client to customers which often involves doing some amount of custom settings. That's one thing that's nice about the existing RuntimePlugin approach — its easy for someone to provide a wrapper around the generated client that adds the runtime plugin (because a runtime plugin is always addable). |
OK, I was thinking to remove |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
...ient/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/auth/AuthDecorator.kt
Outdated
Show resolved
Hide resolved
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
#4413
Description
NoAuthRuntimePlugin was broken after aligning the auth implementation with SRA.
After the SRA alignment, the code generator follows models more faithfully; it injects
noAuthinto the default auth scheme resolver when an operation is annotated with@optionalAuthor@auth([]). However, this brokeNoAuthRuntimePluginsince it wasn't actually configuring the auth scheme option resolver.While fixing
noAuthissues in the originating Smithy model is ideal (by adding appropriate auth traits), that may not always be possible, e.g. when a Smithy-convertible model doesn't support it natively.This PR deprecates the existing
NoAuthRuntimePluginand introducesNoAuthRuntimePluginV2that configures the auth scheme option resolver fornoAuth.Note:
NoAuthRuntimePluginis registered as a default client runtime plugin inFluentClientGenerator.kt. This was a hack from before the auth implementation was aligned with SRA. The plugin continues to be registered by default for backward compatibility, but it's broken due to the lack of an appropriate auth scheme option resolver.NoAuthRuntimePluginV2is not added by default and should only be used as an escape hatch. The ideal fix is to annotate operations withthe appropriate auth trait in the Smithy model whenever possible.
While users could implement the functionality of
NoAuthRuntimePluginV2themselves, the multiple attempts have been seen in the wild (with the confusion), and some customers cannot update their models for various reasons. It's reasonable to provide this utility to avoid repeated issues.Testing
Added integration test in
AuthDecoratorTest.ktIgnore Semver compliance check since
NoAuthRuntimePluginhas been#[doc(hidden)], and the check couldn't get the information out of it.Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.