Skip to content
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

Determine validity of customizations marked with question label #344

Closed
kggilmer opened this issue May 12, 2021 · 10 comments
Closed

Determine validity of customizations marked with question label #344

kggilmer opened this issue May 12, 2021 · 10 comments

Comments

@kggilmer
Copy link
Contributor

kggilmer commented May 12, 2021

Note from planning meeting: (Ken) Setup time with Jasdel, zoe/dongie on open questions (after reconciling with SEPs)

Issues with questions: https://github.com/awslabs/smithy-kotlin/issues?q=is%3Aissue+is%3Aopen+label%3Aquestion

Criteria for evaluating customizations important for DP are:

  1. Does the lack of a customization cause a significant portion of a service to be unusable by customers?
  2. If we were to completely ignore a customization, would there be significant risk that our internals would need rework at the API level once we undertook that implementation?
  3. Is there some other reason why not implementing a customization would result in customers unable to evaluate our SDK after DP release?

Info Sources

@kggilmer
Copy link
Contributor Author

kggilmer commented May 18, 2021

Regarding https://github.com/awslabs/smithy-kotlin/issues/334:

Feature documentation: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.html

In terms of priorities this is an enabling feature (allows for IAM as access control to RDS service instead of other auth mechanisms). Based on this and not complete SDK support across languages, I would classify as necessary for GA possibly but not for DP.

I had correspondance with RDS PM Kahel Sinno. He was able to provide some usage information that supports scoping this feature in GA.

In review w/ team it was brought up that IAM access to RDS is an important security concern for internal teams. Thus, the low external usage of the feature does not represent the internal developer base. As not being able to use IAM authentication would prevent many internal teams from using RDS, based on the above heuristics we should scope this feature in DP.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 18, 2021

Regarding https://github.com/awslabs/smithy-kotlin/issues/174:

Where is this customization coming from? I don't see anything about this in the SEP repo. I don't see anything via search is.amazon.com, and I don't see anything about it in the Go v2 or Java SDKs.

I've searched the public docs of SQS for mention of being able to disable checksums and came up with nothing. This page references checksums only to say that they are calculated by the SDK. No mention of disabling or configuring the feature.

Based on this information I don't believe this is a customization we should implement for DP.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 18, 2021

Regarding #169:

There is no reference to the regex \/%2F\w+%2F in the Go v2 source code, nor do I see a equivalent customization in the Go v2 source tree. The Java v2 SDK does some custom handling with resource paths but they do not seem to be the same thing as referenced in the original Go code sample.

I'll need to follow up w/ Java team to ensure the link posted in the old Go SDK is unrelated to the Java customization. Verified w/ Zoe that this customization is not implemented in Java v2. Based on this I do not believe this customization is necessary at all.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 19, 2021

Regarding #162:

There is no reference to kinesis read timeouts in the SEPs. There is a wiki page which mentions timeouts for some services including Kinesis. The page is pretty old and unsure if its relevence now. Both the Go v2 SDK and the java v2 sdk specify a custom timeout for reads, and Java includes a timeout for writes. For go it's 5 seconds and for java it's 10. The wiki page mentions a suggested timeout of 5 and specifics that there is a server-side timeout of 6 seconds.

Based on this, I don't see this as important functionality for us to address in DP. This is based on my assumption that a client-side timeout is helpful for scalability in general and does not in itself provide functionality necessary to test service usage (as intended by DP release). I discussed the Java customization w Zoe and she confirmed my thoughts here.

Supplemental information from Zoe about Java v2 SDK issues relating to the Kinesis KCL.

@kggilmer
Copy link
Contributor Author

Regarding #160:

It appears that this is custom retry logic specific to EC2. In general, reasoning about retry logic would seem to be something that isn't high-priority for DP releases. I think this because these retry policies are designed in such a way to enable the best scaling configuration and the best client-side customer experience. But, the retry logic itself does not provide a feature or block a feature from working in a given service. Based on this I do not think EC2 custom retry logic is necessary for DP.

I've asked in the CRT slack if CRT handles this specific case from the Go SDK, or if there are any custom retry logic implementations we will need to handle in our SDK if we delegate to CRT generally for retries.

@kggilmer
Copy link
Contributor Author

Regarding #159:

From my reading of the Go SDK feature, this optionally computes the crc32 checksum on the response body, reads a computed value in the header, and compares them. It's unclear to me why this is necessary at all. I would assume the sum of OSI layers would be validating that the response has not been corrupted over the wire, but I must be missing something.

In any case I believe it is safe to designate this out of scope for DP due to the following reasons:

  1. it does not block or reduce client functionality.
  2. it does not present any architectural hurdles (our middleware abstraction allows us to implement specific logic to read response bodies and perform checks).

@kggilmer
Copy link
Contributor Author

kggilmer commented May 20, 2021

Regarding https://github.com/awslabs/smithy-kotlin/issues/155:

Looking at the S3 model in aws-models it does not appear that this has been fixed. I don't see any implementation of this change in Java or Go v2 SDKs. Will need to do some more digging before coming up w/ a decision. However looking at the docs for the impacted operations, this covers specifying a key for server side encryption for S3 objects. This change presumably would be necessary for the feature to work. If so then it comes down to the necessity of supporting custom keys for server side encryption for the DP release.

After discussing this with kotambka, it looks like the Go v2 sdk has yet to implement this. Given that and observations above I think this feature is out of scope for DP.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 20, 2021

regarding #154:

TIL: Some Go customizations are in cross-service source trees, such as for s3 and s3control.

The scoping of this feature is essentially the scoping of S3 Westeros and Outposts. If these S3 features are deemed important enough that customers should be able to use them in our DP SDK they should become in-scope for DP.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 20, 2021

regarding https://github.com/awslabs/smithy-kotlin/issues/152:

Checked w/ the Go team (origin of the issue) and they opted to not implement this feature for GA. As such I would guess this is not a critical feature for IoT support. I'm also unable to find specifics about what actually needs to change in the IoT SDK based on these 3 lines from the Go customization doc. If anyone has concerns I can keep digging but given the Go team's prioritization I would say that this feature is not in scope for DP.

@kggilmer
Copy link
Contributor Author

kggilmer commented May 20, 2021

regarding https://github.com/awslabs/smithy-kotlin/issues/151:

This I believe is referencing a line in the Go customization doc which describes a specific feature of the EC2 IMDS client (being able to retrieve host metadata). This is not a customization in particular but rather a feature of a general-purpose IMDS client.

In order for apps using our SDK to run with EC2 credentials, I presume IMDS client will be needed. As such, this (IMDS client) is an enabling feature for the EC2 service and should be scoped for DP.

In review discussion, the two known necessary features for running in ec2 is credential and region access. Other IMDS functionality is not known to be important (based on DP heuristics above) for DP. Thus specific bindings to CRT for region and credentials is necessary for DP but a general purpose IMDS client is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant