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.

Sign up for GitHub

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

destination-s3 use instanceprofile if credentials are not provided #9399

Merged
merged 18 commits into from
Jan 14, 2022
Merged

destination-s3 use instanceprofile if credentials are not provided #9399

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2e3586c
use instanceprofile to auth if id is not provided
alvaroqueiroz Jan 10, 2022
0278279
restore support for using endpoint
alvaroqueiroz Jan 10, 2022
61413d8
update readme
alvaroqueiroz Jan 10, 2022
7678abe
update changelog
alvaroqueiroz Jan 10, 2022
03b71d1
update documentation, add setup guide
alvaroqueiroz Jan 10, 2022
4167e2a
Update docs/integrations/destinations/s3.md
alvaroqueiroz Jan 11, 2022
0f8adb2
minor fixes
alvaroqueiroz Jan 11, 2022
0faf5d3
add error message
alvaroqueiroz Jan 11, 2022
4ec2af3
now using RuntimeException
alvaroqueiroz Jan 11, 2022
9c56539
Update airbyte-integrations/connectors/destination-s3/src/main/java/i…
alvaroqueiroz Jan 11, 2022
f002c68
Merge branch 'master' into master
alvaroqueiroz Jan 12, 2022
4d50500
Merge branch 'master' of github.com:alvaroqueiroz/airbyte into marcos…
marcosmarxm Jan 12, 2022
7bc31f9
bump connector version
marcosmarxm Jan 12, 2022
03f98c3
Merge branch 'master' into master
alvaroqueiroz Jan 13, 2022
b067991
bump connector version and merge master
marcosmarxm Jan 13, 2022
e11d69e
Merge branch 'master' into marcos/test-pr-9399
marcosmarxm Jan 14, 2022
fdbbaf6
update seed file
marcosmarxm Jan 14, 2022
091bc14
solve conflicts
marcosmarxm Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions airbyte-integrations/connectors/destination-s3/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ As a community contributor, you will need access to AWS to run the integration t

- Create an S3 bucket for testing.
- Get your `access_key_id` and `secret_access_key` that can read and write to the above bucket.
- if you leave `access_key_id` and `secret_access_key` in blank, the authentication will rely on the instance profile authentication
- Paste the bucket and key information into the config files under [`./sample_secrets`](./sample_secrets).
- Rename the directory from `sample_secrets` to `secrets`.
- Feel free to modify the config files with different settings in the acceptance test file (e.g. `S3CsvDestinationAcceptanceTest.java`, method `getFormatConfig`), as long as they follow the schema defined in [spec.json](src/main/resources/spec.json).
Expand Down
13 changes: 10 additions & 3 deletions ...tination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3DestinationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package io.airbyte.integrations.destination.s3;

import com.amazonaws.auth.InstanceProfileCredentialsProvider;
import com.amazonaws.ClientConfiguration;
import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.AWSStaticCredentialsProvider;
Expand Down Expand Up @@ -87,8 +88,8 @@ public static S3DestinationConfig getS3DestinationConfig(final JsonNode config)
config.get("s3_bucket_name").asText(),
bucketPath,
config.get("s3_bucket_region").asText(),
config.get("access_key_id").asText(),
config.get("secret_access_key").asText(),
config.get("access_key_id") == null ? "" : config.get("access_key_id").asText(),
edgao marked this conversation as resolved.
Show resolved Hide resolved
config.get("secret_access_key") == null ? "" : config.get("secret_access_key").asText(),
partSize,
format);
}
Expand Down Expand Up @@ -128,7 +129,13 @@ public S3FormatConfig getFormatConfig() {
public AmazonS3 getS3Client() {
final AWSCredentials awsCreds = new BasicAWSCredentials(accessKeyId, secretAccessKey);

if (endpoint == null || endpoint.isEmpty()) {
if (accessKeyId == null && secretAccessKey == null || accessKeyId.isEmpty() && secretAccessKey.isEmpty()) {
return AmazonS3ClientBuilder.standard()
.withCredentials(new InstanceProfileCredentialsProvider(false))
alvaroqueiroz marked this conversation as resolved.
Show resolved Hide resolved
.build();
}

else if (endpoint == null || endpoint.isEmpty()) {
return AmazonS3ClientBuilder.standard()
.withCredentials(new AWSStaticCredentialsProvider(awsCreds))
.withRegion(bucketRegion)
Expand Down
4 changes: 1 addition & 3 deletions airbyte-integrations/connectors/destination-s3/src/main/resources/spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
"s3_bucket_name",
"s3_bucket_path",
"s3_bucket_region",
"access_key_id",
"secret_access_key",
Comment on lines -15 to -16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherifnada can you check this PR adding the option to use S3 destination with InstanceProfile. No additional change in UI, but how users can experience errors from the front-end. What do you think? Should add an option to select the connection method => (credentials or instance profile)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the user-experience will not suffer,

if access_key_id AND secret_access_key are not provided -> instanceprofile auth
if access_key_id OR secret_access_key are not provided -> standard authentication error
the rest will stay the same.

"format"
],
"additionalProperties": false,
Expand Down Expand Up @@ -72,7 +70,7 @@
},
"access_key_id": {
"type": "string",
"description": "The access key id to access the S3 bucket. Airbyte requires Read and Write permissions to the given bucket.",
"description": "The access key id to access the S3 bucket. Airbyte requires Read and Write permissions to the given bucket, if not set, Airbyte will rely on Instance Profile",
alvaroqueiroz marked this conversation as resolved.
Show resolved Hide resolved
"title": "S3 Key Id",
"airbyte_secret": true,
"examples": ["A012345678910EXAMPLE"]
Expand Down
1 change: 1 addition & 0 deletions docs/integrations/destinations/s3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ Under the hood, an Airbyte data stream in Json schema is first converted to an A

| Version | Date | Pull Request | Subject |
| :--- | :--- | :--- | :--- |
| 0.2.3 | 2022-01-10 | [\#9399](https://github.com/airbytehq/airbyte/pull/9399) | Use instance profile authentication if credentials are not provided |
alvaroqueiroz marked this conversation as resolved.
Show resolved Hide resolved
| 0.2.2 | 2021-12-21 | [\#8574](https://github.com/airbytehq/airbyte/pull/8574) | Added namespace to Avro and Parquet record types |
| 0.2.1 | 2021-12-20 | [\#8974](https://github.com/airbytehq/airbyte/pull/8974) | Release a new version to ensure there is no excessive logging. |
| 0.2.0 | 2021-12-15 | [\#8607](https://github.com/airbytehq/airbyte/pull/8607) | Change the output filename for CSV files - it's now `bucketPath/namespace/streamName/timestamp_epochMillis_randomUuid.csv` |
Expand Down