ajfabbri commented on code in PR #8628: URL: https://github.com/apache/hadoop/pull/8628#discussion_r3647361250
########## hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/qualifying_a_new_sdk.md: ########## @@ -0,0 +1,1899 @@ +<!--- + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. See accompanying LICENSE file. +--> + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL +NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and +"OPTIONAL" in this document are to be interpreted as described in +RFC 2119. + +# Qualifying an AWS SDK + + +> The AWS SDKs and CLI are designed for usage with official AWS services. +> We may introduce and enable new features by default, such as these new default integrity protections, +> prior to them being supported or otherwise handled by third-party service implementations. Review Comment: 👍 good intro for context. ########## hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/qualifying_a_new_sdk.md: ########## @@ -0,0 +1,1899 @@ +<!--- + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. See accompanying LICENSE file. +--> + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL +NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and +"OPTIONAL" in this document are to be interpreted as described in +RFC 2119. + +# Qualifying an AWS SDK + + +> The AWS SDKs and CLI are designed for usage with official AWS services. +> We may introduce and enable new features by default, such as these new default integrity protections, +> prior to them being supported or otherwise handled by third-party service implementations. + +That is a quote from an +[announcement of a somewhat incompatible change](https://github.com/aws/aws-sdk-java-v2/discussions/5802) +which shipped in v2.30.0 of the AWS SDK. + +It highlights the SDK team's point of view: their job is to work on the SDK to support AWS's own services. +Compatibility with third-party services is not their problem, and they do not test against such stores. + +This makes sense from their perspective: if someone implements their own S3 store, then it is +their task to make it compatible with AWS S3, even as that is a moving target with no public +formal API specification. + +The S3A connector is one of the most popular of S3 connectors used to connect +JVM-hosted big-data applications to AWS S3 *and to other S3-compatible stores*. +We do not have the luxury of saying "third-party stores are not our problem", so have to make +sure that our release works with all stores. + +And because of that broad adoption, we need to make sure that it works in +different deployment scenarios, with different configurations even within AWS. + +The task of qualifying an AWS SDK is a lot more than just incrementing a number in a maven POM file: +it is determining whether the SDK is safe to adopt, and, if safe, +identifying and making any changes in our code that are needed to migrate. + + +## Introduction + +The S3A connector is utterly dependent upon the AWS SDK; even a minor change can have serious consequences. +That is: changing a single number in a maven file can bring new features and needed bug fixes. +It can also cause a lot of damage, albeit unintentionally. + +Some example regressions encountered previously include: +* The SDK printing a warning message telling developers off every time a specific object in the SDK is instantiated + This breaks all tests which look for specific output strings and runs a risk of generating support calls asking "why is my application telling me off?" +* A change in the semantics of calling `abort()` on a stream. + This was a valid design decision — however, it was unexpected. + And again the warning message printed every time the stream was closed prematurely flooded application logs. +* Instabilities in the shading of third-party libraries (slf4j, etc.), with consequences such as the inability to enable any form of logging. +* The shaded library continuing to declare dependencies which are redundant due to the shading. Review Comment: Nice examples. This one has a special place in my heart: https://github.com/aws/aws-sdk-java/issues/1102 ########## hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/qualifying_a_new_sdk.md: ########## @@ -0,0 +1,1899 @@ +<!--- + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. See accompanying LICENSE file. +--> + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL +NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and +"OPTIONAL" in this document are to be interpreted as described in +RFC 2119. + +# Qualifying an AWS SDK + + +> The AWS SDKs and CLI are designed for usage with official AWS services. +> We may introduce and enable new features by default, such as these new default integrity protections, +> prior to them being supported or otherwise handled by third-party service implementations. + +That is a quote from an +[announcement of a somewhat incompatible change](https://github.com/aws/aws-sdk-java-v2/discussions/5802) +which shipped in v2.30.0 of the AWS SDK. + +It highlights the SDK team's point of view: their job is to work on the SDK to support AWS's own services. +Compatibility with third-party services is not their problem, and they do not test against such stores. + +This makes sense from their perspective: if someone implements their own S3 store, then it is +their task to make it compatible with AWS S3, even as that is a moving target with no public +formal API specification. + +The S3A connector is one of the most popular of S3 connectors used to connect +JVM-hosted big-data applications to AWS S3 *and to other S3-compatible stores*. +We do not have the luxury of saying "third-party stores are not our problem", so have to make +sure that our release works with all stores. + +And because of that broad adoption, we need to make sure that it works in +different deployment scenarios, with different configurations even within AWS. + +The task of qualifying an AWS SDK is a lot more than just incrementing a number in a maven POM file: +it is determining whether the SDK is safe to adopt, and, if safe, +identifying and making any changes in our code that are needed to migrate. + + +## Introduction + +The S3A connector is utterly dependent upon the AWS SDK; even a minor change can have serious consequences. +That is: changing a single number in a maven file can bring new features and needed bug fixes. +It can also cause a lot of damage, albeit unintentionally. + +Some example regressions encountered previously include: +* The SDK printing a warning message telling developers off every time a specific object in the SDK is instantiated + This breaks all tests which look for specific output strings and runs a risk of generating support calls asking "why is my application telling me off?" +* A change in the semantics of calling `abort()` on a stream. + This was a valid design decision — however, it was unexpected. + And again the warning message printed every time the stream was closed prematurely flooded application logs. +* Instabilities in the shading of third-party libraries (slf4j, etc.), with consequences such as the inability to enable any form of logging. +* The shaded library continuing to declare dependencies which are redundant due to the shading. + +Third-party store support can also be trouble as it is not something tested by the AWS SDK team themselves (why would they?). This means our code may be one of the first contact points between an update of the SDK and third-party stores. + +The core semantics of the S3A/SDK integration can be reasonably well tested simply by running the S3A integration test suite with all the optional features covered: +* KMS encryption +* Versioned bucket support +* AWS access points +* STS session tokens +* Third-party storage + + +The challenge when qualifying an SDK is to make sure that the following condition holds: + +> After an upgrade to the SDK, it is still possible to read and write data to all classes of AWS S3 store, +> and all third-party stores which were previously supported. +> This condition must hold for the command line and downstream applications; the hadoop-aws +> test suites are necessary but not sufficient. + +From the outset, assume that there is a regression and that your challenge is to find it. +That is, rather than the qualification being a process of "run some automated and manual tests to show that all is well", +the task has to be approached as one of "find out what has broken, where and why". +Then we can worry about how to fix. + +The test process then: + +1. Run the usual integration test with as many of the optional features covered. +Do not simply verify that everything appears to have worked: +you must also look through all the log output to make sure there are no new warning messages being printed +indicating a mismatch between how the S3A code is using the library and the library expects to be used. +2. Build a binary release and test through the command line. +3. Build and test as many downstream applications as you can. + +> What happens if a regression does surface and the qualification process that did not find it +> - and now the SDK upgrade has been applied? + +We revert. Immediately. Then the process for identifying and trying to remedy the issue surfaces. +If the library has already shipped, then this is harder. +Here as well as identifying the root cause we need to assess what is the impact of this in production deployment. +We may need to issue a new Hadoop release. +This is time-consuming, painful, and needless hard work. This is why it is so important that we get it right. + +> What happens if I absolutely need a new feature in the latest SDK? + +Congratulations! You have just taken on the task of qualifying the SDK release! + + +## Stop! Is This a Last-Minute Action Before a Release? + +If so: _it is too late_. + +We need at least two weeks of stabilization to see if other developers +encounter problems related to their own set-ups: endpoints, networks, +credentials - as well as applications built on top of it. + +If it is for a feature: postpone the release, or don't do the update. +Is it for a critical fix? Postpone the release. + +Either way: we need that time to find things before shipping. + +## Choosing an SDK + +Use an SDK which has been out for two or more weeks. + +* If we do need a specific release for a fix: go with that one or later. +* If it is a feature we need, always try for a slightly later build. + +Features always take time to stabilize, so let others find the problems and +AWS engineers the solutions. + +1. Look at [announcements](https://github.com/aws/aws-sdk-java-v2/discussions/categories/announcements) + to see if there is a recent announcement related to S3 or core authentication. +2. Look at [V2 SDK issues](https://github.com/aws/aws-sdk-java-v2/issues) to see what + recently reported issues are which may cause problems. + Look at the discussion, and if it is relevant, subscribe. + Consider also examining our code to see if there is any actual exposure. + Do not just look at the open issues: look at all recent issues as there may be + recently closed bugs whose fixes must be picked up; this + search identifies them. + +## Test Setup + +To be confident the upgraded SDK works in many deployment configurations, we +need to validate it with as many of the different configuration options and +store types we can. + +This is done through a combination of different S3 implementations, +and by having a complex test matrix of different configurations +for a very small set of S3 test buckets in AWS and elsewhere + +### Test Buckets + +Submitter MUST have the following buckets: +* `B1`: + - S3 standard + - SSE-KMS at bucket level + - SHOULD S3 server logging to B2. +* `B2`: + - S3 standard + - MUST: versioned (with versions configured to delete after 7 days) + - MUST: configured with path style access. + - MUST: configured to buffer writes into an array. + (Note, one `testMultiObjectDeleteLargeNumKeys` may OOM here, don't worry about it) + - SHOULD have an access point defined. + In this document`B2AP` is the bucket configuration + to access it via the AP. +* `B3`: S3 Express + - MAY: Using CSE-KMS +* `B4`: S3 standard (i.e. if you test in us-west-2, put this in us-east-1) + - S3 standard + - us-east-1 + - long-long distance link to the test system. + If you are testing remotely, this is implicit. + If you are testing within AWS infrastructure, it MUST be a different region. +* `B5`: third-party store. + - SHOULD: Use Google GCS as documented in [third-party stores](./third_party_stores.html). + - MAY: Any other third-party store you can access. Ideally one with bulk delete support, which + Google's S3 endpoint lacks. This is needed to verify regressions related to MD-5 signing + of bulk deletes haven't failed. + +Testing with at least one third-party store is critical, as is an S3 Express store. +Ideally, test with multiple third-party stores. + + +| Id | Class | Config | +|--------|-------------------|-----------------------------------------------------------| +| `B1` | S3 standard | SSE-KMS; Has S3 server logging to B2 | +| `B2` | S3 standard | Path style access, versioned, MUST BE same region as B1. | +| `B2AP` | Access Point | Access Point to B2 (TLS 1.3+ only) | +| `B3` | S3 Express | Default configurations | +| `B4` | S3 standard | Long haul link in US and access point access | +| `B5` | Third-party store | Google GCS or other third-party store | + +These are the core storage class/configurations which are used in production, +hence are part of the qualification process. + +One of the buckets B1-B4 MUST be in a region for which there is a FIPS endpoint, +so that it can be configured to use it for access. +That bucket must therefore be within a US region. + +A third-party store *MUST* be tested. + +*Note* in the XML below, replace `B1`, `B2`, etc. with the names of your test buckets. + +```xml +<property> + <name>fs.s3a.bucket.B4.endpoint.fips</name> + <value>true</value> +</property> +``` + +All buckets which support lifecycle policies SHOULD be set to abort all pending uploads +after 24h, delete all files after 7d. + +### Test Host + +We also need two test hosts to validate behaviour in the two key scenarios: in AWS and outside of it. + +You can start with a single host, but you will need to validate the behaviour of the SDK in both scenarios. + +Within AWS +1. EC2/kerberos deployment outside us-east-1 and within a VPC whose network rules can be configured to not allow access to us-east-1. + The build can be done without that rule (needed for the artifact download), but a test run must be one locked down. This is to validate local region resolution. +2. On a remote host, with any config for the AWS CLI (temporarily) renamed from `~/.aws/config` to something else. + This is needed to make sure the SDK isn't reading region/endpoint info from that file, as + it can do - and which can therefore accidentally hide regressions. + Note: renaming your config file before running CLI testing may be enough for this. + + +### Configuration and Extra Services + +Submitter MUST have the extra AWS setup for: +* KMS encryption +* Assumed role for session token tests. +* An access point. + +This may seem a lot of preparation but it is needed for full test coverage. + +These configuration SHOULD go into an XInclude-able configuration file which can be referenced absolutely, +for example in a directory `~/config/auth-keys.xml`, which can be +referenced both from hadoop-aws tests, and in full distributions you have +built up. + +Ideally, both `hadoop-tools/hadoop-aws/src/test/resources/auth-keys.xml` +and `etc/hadoop/core-site.xml` will look identical + +```xml +<configuration> + <include xmlns="http://www.w3.org/2001/XInclude"; + href="///users/alice/config/auth-keys.xml"> + </include> +</configuration> +``` + +Keeping these out of the hadoop source tree used to be to avoid accidentally committing secrets. +It is now critical as a way to stop AI tools scanning the files and including the secrets +when it generates code for the project. + +#### Recommendations + +Initialize that `~/config/` directory as a *local* github repo, it is easier to +see what you've broken. +Obviously you MUST NOT push it to any remote repo if it contains +your AWS secrets. + +Have a separate XInclude file for the test-related settings for each endpoint, to +make switching between them easier. Review Comment: good tip. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
