Service sdk sigv4 #1381
Service sdk sigv4 #1381
Conversation
This comment has been minimized.
This comment has been minimized.
luigi617
added
the
no-changelog
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/service/ktor/Authentication.kt
Outdated
Show resolved
Hide resolved
...codegen/service-codegen-tests/src/main/kotlin/com/test/AuthenticationServiceTestGenerator.kt
Show resolved
Hide resolved
tests/codegen/service-codegen-tests/src/test/kotlin/com/test/AuthenticationServiceTest.kt
Outdated
Show resolved
Hide resolved
| private lateinit var proc: Process | ||
|
|
||
| @BeforeAll | ||
| fun boot() { |
There was a problem hiding this comment.
If possible the boot and shutdown function should be shared between authentication and cbor tests
There was a problem hiding this comment.
They shouldn't share these functions because the Smithy models used are different, meaning they rely on different services. However, this is something I’ve thought about before. Different tests could share the boot and shutdown functions if we created a large Smithy model that included everything. This would make the tests faster since fewer services would need to be started. On the other hand, it would make the code harder to read, maintain, and extend.
| val accessKey = "AKIAIOSFODNN7EXAMPLE" | ||
| val secretKey = "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" |
There was a problem hiding this comment.
Can you extract the accessKey and secretKey prefixes and make them constants? For readability, or if it doesn't break the tests, could you remove the prefix all together.
There was a problem hiding this comment.
I didn't understand this comment, I know the prefix of access key for sigv4 is AKIA, but what is the prefix of secret key? and what do you mean by removing the prefix all together?
There was a problem hiding this comment.
I mean the wJalrXUtnFEMI/K7MDENG+bPxRfiCY that is repeated for a lot of the secret keys and AKIAIOSFODNN7 in the access keys. Is it necessary for the SIGV4 signing to work? It's not a big deal if you leave it as is but I noticed it made the tests harder to follow. I meant just using EXAMPLEKEY, WRONGKEY, etc.
There was a problem hiding this comment.
I change the access key and secret key to make it easier
| withBlock("sigV4(name = #S) {", "}", "aws-sigv4") { | ||
| write("region = #T.region", ServiceTypes(pkgName).serviceFrameworkConfig) | ||
| val serviceSigV4AuthTrait = serviceShape.getTrait<SigV4Trait>() | ||
| if (serviceSigV4AuthTrait != null) { |
There was a problem hiding this comment.
Should we be rendering the
withBlock("sigV4(name = #S) {", "}", "aws-sigv4") {block from above if the service doesn't have the SigV4 trait?
There was a problem hiding this comment.
I think yes, because once generated, you should allow user to use sigv4 if they want to add it manually
| writer.withBlock("internal object SigV4CredentialStore {", "}") { | ||
| write("private val table: Map<String, #T> = mapOf()", RuntimeTypes.Auth.Credentials.AwsCredentials.Credentials) | ||
| withBlock("internal fun get(accessKeyId: String): #T? {", "}", RuntimeTypes.Auth.Credentials.AwsCredentials.Credentials) { | ||
| write("// TODO: implement me: return Credentials(accessKeyId = ..., secretAccessKey = ...)") |
There was a problem hiding this comment.
I like this comment, wouldn't the implementer just need to populate the table val ? Also can you add a similar comment for SigV4a
There was a problem hiding this comment.
I want to let user have the flexibility to choose between populating the table or using other methods to get the Credentials (maybe calling another api or accessing database)
...egen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/service/ktor/AuthenticationAWS.kt
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| val accessKey = "AKIAIOSFODNN7EXAMPLE" | ||
| val secretKey = "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY" |
There was a problem hiding this comment.
I mean the wJalrXUtnFEMI/K7MDENG+bPxRfiCY that is repeated for a lot of the secret keys and AKIAIOSFODNN7 in the access keys. Is it necessary for the SIGV4 signing to work? It's not a big deal if you leave it as is but I noticed it made the tests harder to follow. I meant just using EXAMPLEKEY, WRONGKEY, etc.
Affected ArtifactsChanged in size
|
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.