fix: application of sigv4a #1186
Conversation
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
...en/src/main/kotlin/aws/sdk/kotlin/codegen/customization/SigV4AsymmetricTraitCustomization.kt
Outdated
Show resolved
Hide resolved
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
| @@ -24,28 +24,34 @@ class SigV4AsymmetricTraitCustomization : KotlinIntegration { | |||
| // Needs to happen before the `SigV4AsymmetricAuthSchemeIntegration` & `SigV4AuthSchemeIntegration` (-50 & -50) | |||
| override val order: Byte = -60 | |||
|
|
|||
| // services which support SigV4A but don't model it | |||
| private val unmodeledSigV4aServices = listOf("s3", "eventbridge", "cloudfront keyvaluestore") | |||
There was a problem hiding this comment.
correctness: I don't think cloudfront KVS needs to be in this integration anymore does it?
There was a problem hiding this comment.
It is needed, this integration also applies the SigV4A trait. Do you want the new integration to replicate that and then remove cloudfront KVS from this integration?
There was a problem hiding this comment.
I'm not sure I'm following, the new CFKVS integration is applying the sigv4a trait as well so it's redundant here no?
There was a problem hiding this comment.
It wasn't when you first reviewed it, I just added that application and removed cloudfront keyvaluestore from the SigV4AsymmetricTraitCustomization
| } else { | ||
| AuthTrait(it.valueSet + mutableSetOf(SigV4ATrait.ID)) | ||
| } | ||
| } ?: AuthTrait(mutableSetOf(SigV4Trait.ID, SigV4ATrait.ID)) |
There was a problem hiding this comment.
suggestion: Probably want a remark in here about why this is ordered this way.
(It's because existing services (cloudfront KVS excluded) that use sigv4a do so through endpoint rules and it's not on the model. We don't know which operations it should apply for in those cases and so the safest thing to do is add it at the end and let endpoint rules change the priority as needed. Adding it this way registers support for the trait to let the rest of codegen Just Work (TM)).
| */ | ||
| class CloudFrontKeyValueStoreSigV4APrioritizationCustomization : KotlinIntegration { | ||
| // Runs after SigV4AsymmetricTraitCustomization (-60) and before `SigV4AsymmetricAuthSchemeIntegration`(-50) and `SigV4AuthSchemeIntegration` (-50) | ||
| override val order: Byte = -55 |
There was a problem hiding this comment.
I don't think you'll even need to order this if you remove it from the other integration
| * Modify the [AuthTrait] of the CloudFront KeyValueStore service, placing SigV4A before SigV4 in the | ||
| * prioritized list. | ||
| */ | ||
| class CloudFrontKeyValueStoreSigV4APrioritizationCustomization : KotlinIntegration { |
There was a problem hiding this comment.
fix: Other service specific customization's are in their own package directory.
suggestion/nit: This is more of a backfill, I'd consider renaming to BackfillSigv4A or something to that affect.
| builder.removeTrait(AuthTrait.ID) // remove existing auth trait | ||
|
|
||
| // add a new auth trait with SigV4A in front | ||
| val authTrait = AuthTrait(mutableSetOf(SigV4ATrait.ID, SigV4Trait.ID)) |
There was a problem hiding this comment.
fix: We should retain the existing auth trait in case it has other auth schemes over time (unlikely but no need to bake it in this way). I'd also only add sigv4a if the existing service model doesn't have it on the service shape.
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
|
A new generated diff is ready to view. |
Addresses an issue with applying the SigV4A trait to services which don't model it
Issue #
#1184
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.