refactor(rt)!: split auth and signing packages; expose a method to sign requests #318
Conversation
kggilmer
left a comment
kggilmer
left a comment
There was a problem hiding this comment.
Overall LGTM but would like to think a bit more about the modeling of AwsSigningConfig.region
| /** | ||
| * Container for signed output and signature | ||
| */ | ||
| public data class SigningResult<T>(val output: T, val signature: ByteArray) { |
There was a problem hiding this comment.
question
why is equals/hashCode impl required for this type?
There was a problem hiding this comment.
Data classes don't handle arrays as expected. What you want usually is to compare the contents of the array not whether they are the same instance (which is what it will do by default). Intellij even warns you of this and generates the more appropriate one for you.
| import aws.smithy.kotlin.runtime.http.request.HttpRequest | ||
| import aws.smithy.kotlin.runtime.http.request.toBuilder | ||
|
|
||
| /** |
There was a problem hiding this comment.
suggestion
I assume the intent is that customers may use this type since there is no internal markers. the KDoc should probably be a bit more thorough in this case, specifying details for parameters for example.
| * Sign [HttpRequest] using the given signing [config] | ||
| * @return signing result | ||
| */ | ||
| public suspend fun sign(request: HttpRequest, config: AwsSigningConfig): SigningResult<HttpRequest> { |
There was a problem hiding this comment.
same as above regarding kdoc
| @@ -1,10 +1,17 @@ | |||
| package aws.sdk.kotlin.runtime.auth | |||
| /* | |||
| // region is required when using a standalone signing config but the middleware takes the signing region | ||
| // (and other settings) from the operation execution attributes and combines them with the middleware | ||
| // defaults | ||
| if (builder.region == null) { builder.region = "" } |
There was a problem hiding this comment.
question
It's not clear to me that this mapping from null to empty-string is better than allowing region to be a nullable property of AwsSigningConfig and deal with handling null based on context at points in code where the property is accessed. From my understanding, here "" means null, is this correct?
There was a problem hiding this comment.
Yeah basically. I changed this such that the middleware could just re-use the signing config rather than re-declaring an equivalent config type.
The issue I ran into is we (rightfully so) designed AwsSigningConfig to be general purpose and throw errors if not configured correctly.
It didn't seem necessarily the right thing to do to remove the validation since doing so will move the need to validate into each use of it. I opted to just abuse the type rather than remove the validation but if you see a cleaner way I'm all ears.
There was a problem hiding this comment.
IMO it would be cleaner to model the difference between presigning and signer middleware via different types which accurately capture their required inputs. So based on your breakdown the middleware would have a type in which region is nullable and the presigner would have a region member that was non-null.
There was a problem hiding this comment.
That's how it was. I changed it because the signing config has a lot of knobs and it was a lot of duplication. If anything I would just remove the requireNotNull() on the signing config and enforce it elsewhere. The only reason I left is to try and give customers an early indication that their signing config isn't valid.
There was a problem hiding this comment.
I would prefer that the types used in signing accurately modeled the required inputs, because to me that is more customer obsessed. If you and @ianbotsf prefer this approach however I'm fine with that.
There was a problem hiding this comment.
I agree with @kggilmer on this one. Having separate config classes which use different types for region seems more correct and more expected to me.
There was a problem hiding this comment.
Reverted to using a dedicated type for middleware config
3 participants
Issue #
N/A
support PR(s):
Description of changes
signingsubpackage ofauthcredentialssubpackage ofauthsign()method that can be used to sign HttpRequests independent of middlewareAwsSigningConfigfor it's feature configurationScope
This is a breaking change for users that import a specific credentials provider. They will need to update their code to the new import location.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.