feat: account id routing #1111
feat: account id routing #1111
Conversation
…mp-all-the-versions
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
| @@ -121,6 +121,7 @@ internal const val WEB_IDENTITY_TOKEN_FILE = "web_identity_token_file" | |||
| internal const val AWS_ACCESS_KEY_ID = "aws_access_key_id" | |||
| internal const val AWS_SECRET_ACCESS_KEY = "aws_secret_access_key" | |||
| internal const val AWS_SESSION_TOKEN = "aws_session_token" | |||
| internal const val AWS_ACCOUNT_ID = "aws_account_id" | |||
There was a problem hiding this comment.
Is this standardized somewhere? I've never seen it before
There was a problem hiding this comment.
No but it's in the SEP, I too was wondering about this.
There was a problem hiding this comment.
what happened here with the sorting?
There was a problem hiding this comment.
I mentioned in the PR description. The order matters when we have multiple section writer bindings for the same section (though we can control this more explicitly with the order parameter in KotlinIntegration). I noticed there wasn't much thought given to the order though in terms of grouping customizations and what not so I re-arranged it.
| { | ||
| "id": "08768631-1402-4492-96da-c7f246a2eec5", | ||
| "type": "feature", | ||
| "description": "Enable account ID based endpoint routing for services that use it" |
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (other !is Arn) return false | ||
| if (partition != other.partition) return false | ||
| if (service != other.service) return false | ||
| if (region != other.region) return false | ||
| if (accountId != other.accountId) return false | ||
| return resource == other.resource | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| var result = partition.hashCode() | ||
| result = 31 * result + service.hashCode() | ||
| result = 31 * result + (region?.hashCode() ?: 0) | ||
| result = 31 * result + (accountId?.hashCode() ?: 0) | ||
| result = 31 * result + resource.hashCode() | ||
| return result | ||
| } |
There was a problem hiding this comment.
Nit: If this is an internal class, it could be a data class to avoid writing/maintaining these.
There was a problem hiding this comment.
It could be, I didn't refactor this when moving from public to internal though and didn't see a reason to.
| @PublishedApi | ||
| internal fun build(): Arn { | ||
| require(!partition.isNullOrBlank()) { "ARN partition must not be null or blank" } | ||
| require(!service.isNullOrBlank()) { "ARN service must not be null or blank" } | ||
| requireNotNull(resource) { "ARN resource must not be null" } | ||
| return Arn(this) | ||
| } |
There was a problem hiding this comment.
Question: Why validate some of the fields in the builder and the other fields in the Arn class itself?
There was a problem hiding this comment.
because it has a constructor that can be used independent of the builder. The builder itself has nullable fields but the constructor doesn't. We only validate the things the builder needs to be responsible for here and elsewhere.
| @PublishedApi | ||
| internal fun build(): Arn { |
There was a problem hiding this comment.
Question: Does @PublishedApi have any effect on members of inner classes of internal classes?
There was a problem hiding this comment.
N, this was originally all public so I left it as is.
| return result | ||
| } | ||
|
|
||
| public class Builder { |
There was a problem hiding this comment.
It was originally public. My understanding is that any client who sees the declaring class sees its public members. Because the outer class visibility is internal though this is only visible wherever the outer class is. The binary diff tooling seems to agree.
| public fun fromValue(value: String): ArnResourceTypeSeparator = when (value) { | ||
| "/" -> SLASH | ||
| ":" -> COLON | ||
| else -> error("unknown ARN resource type separator `$value`, expected one of ${values().map { it.separator }}") | ||
| } |
There was a problem hiding this comment.
Style:
public fun fromValue(value: String): ArnResourceTypeSeparator =
checkNotNull(entries.find { it.separator = value }) {
"unknown ARN resource type separator `$value`, expected one of ${entries.map { it.separator }}"
}| /** | ||
| * Endpoint parameters are populated on a best effort basis. This is the default when not | ||
| * explicitly set. | ||
| */ | ||
| PREFERRED, |
There was a problem hiding this comment.
Nit: Defaults are at the usage site (e.g., AccountIdEndpointModeProp) not at the declaration site. The sentence about default probably belongs in other KDocs.
| tests.forEach { test -> | ||
| val parsed = Arn.parse(test.first) | ||
| assertEquals(test.second, parsed) | ||
| // test round trip | ||
| assertEquals(test.first, parsed.toString()) | ||
| } |
There was a problem hiding this comment.
Style: Consider using destructuring to make the test loop clearer:
tests.forEach { (originalString, expected) ->
val parsed = Arn.parse(originalString)
assertEquals(expected, parsed)
// test round trip
assertEquals(originalString, parsed.toString())
}| private fun ecsResponse(accountId: String? = null): HttpResponse { | ||
| val kvp = buildMap { | ||
| put("Code", "Success") | ||
| put("LastUpdated", "2021-09-17T20to57to08Z") | ||
| put("Type", "AWS-HMAC") | ||
| put("AccessKeyId", "AKID") | ||
| put("SecretAccessKey", "test-secret") | ||
| put("Token", "test-token") | ||
| put("Expiration", expectedExpiration.format(TimestampFormat.ISO_8601)) | ||
| if (accountId != null) { | ||
| put("AccountId", accountId) | ||
| } | ||
| } | ||
| """.encodeToByteArray() | ||
|
|
||
| val payload = kvp.entries.joinToString(prefix = "{", postfix = "}", separator = ",") { | ||
| "\"${it.key}\":\"${it.value}\"" | ||
| }.encodeToByteArray() | ||
|
|
||
| return HttpResponse(HttpStatusCode.OK, Headers.Empty, HttpBody.fromBytes(payload)) | ||
| } |
There was a problem hiding this comment.
Suggestion: Manually constructing a JSON string of this complexity is brittle and prone to break. Consider using kotlinx.serialization for clarity and extra safety:
val payload = buildJsonObject {
put("Code", "Success")
put("LastUpdated", "2021-09-17T20to57to08Z")
put("Type", "AWS-HMAC")
put("AccessKeyId", "AKID")
put("SecretAccessKey", "test-secret")
put("Token", "test-token")
put("Expiration", expectedExpiration.format(TimestampFormat.ISO_8601))
if (accountId != null) {
put("AccountId", accountId)
}
}.toString().encodeToByteArray()| private fun ecsResponse(accountId: String? = null): HttpResponse { | ||
| val kvp = buildMap { | ||
| put("Code", "Success") | ||
| put("LastUpdated", "2021-09-17T20to57to08Z") |
There was a problem hiding this comment.
Question: Why is "to" used instead of ":" in this timestamp?
|
|
||
| override fun enabledForService(model: Model, settings: KotlinSettings): Boolean { | ||
| val rules = model.expectShape<ServiceShape>(settings.service).getEndpointRules() | ||
| return rules?.parameters?.find { it.isBuiltIn && it.builtIn.get() == "AWS::Auth::AccountId" } != null |
There was a problem hiding this comment.
Nit: Feels like "AWS::Auth::AccountId" should be a constant somewhere.
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
… accountid-builtin
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
upstream: smithy-lang/smithy-kotlin#998
Description of changes
Arntype + parser. We discussed as a team and decided to leave thisinternalfor now. As such I've moved it toaws-configartifact. If we ever choose to make it public and implement AWS ARN Parser #215 we should move it back toaws-core.finalizeConfigsection writers. These all need to append rather than overwrite each other. I also re-ordered the integrations registration to something with more thought given to it (core integrations first, service specific ones grouped and after core).Testing
This isn't easy to test since there are no services making use of this builtin yet. I added the parameter to the S3 endpoint rules and inspected the generated code is correct and compiles.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.