destination-s3: add a test for listObjects permission on destination bucket #10856

grishick · 2022-03-04T16:52:46Z

What

The S3 destination connector occasionally needs s3:listObjects permissions on an S3 bucket. The connector currently does not actually verify that the input IAM user has listObjects permissions on the bucket which prevents running a RESET_SCHEMA job in Airbyte. This means that a user could setup the connector only to have it fail later due to this missing permissions.

How

add testIAMUserHasListObjectPermission method to S3Destination
and call this method from S3Destination::check. Method throws
an exception if IAM user does not have listObjects permission
on the destination bucket
add a unit test to S3DestinationTest to verify that S3Destination::check
fails if listObjects throws an exception
add a unit test to S3DestinationTest to verify that S3Destination::check
succeeds if listObjects succeeds
Add S3DestinationConfigFactory in order to be able to mock S3 client
used in S3Destination::check

🚨 User Impact 🚨

Before this change, when a user sets up an S3 destination with IAM credentials that do not have listObjects permission on the destination bucket, connection check succeeds. After this change, connection check fails if listObjects permissions are missing.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Tests

Unit

added S3DestinationTest::checksS3NoListObjectPermission

...ctors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3Destination.java

sherifnada · 2022-03-05T00:50:15Z

...s/destination-s3/src/test/java/io/airbyte/integrations/destination/s3/S3DestinationTest.java

+    assertEquals(Status.FAILED, status.getStatus(), "Connection check should have failed");
+    assertTrue(status.getMessage().indexOf("Access Denied") > 0, "Connection check returned wrong failure message");
+
+    // Test that check succeeds when IAM user has listObjects permission


ideally each unit test verifies a single well defined scenario, so we should split these into two unit tests

sherifnada · 2022-03-05T00:51:37Z

...s/destination-s3/src/test/java/io/airbyte/integrations/destination/s3/S3DestinationTest.java

@@ -20,19 +34,66 @@
 public class S3DestinationTest {

  private AmazonS3 s3;
+  private AmazonS3 s3NoAccess;


we can probably keep using a single s3 client, and instead add doThrow(..).when(s3) block in the test itself. This makes it clearer what mocks the test itself is setting up and keeps the global namespace light

sherifnada · 2022-03-05T00:51:55Z

...destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3DestinationConfig.java

@@ -127,6 +141,10 @@ public S3FormatConfig getFormatConfig() {
  }

  public AmazonS3 getS3Client() {
+    if(this.s3Client !=null) {


formatting: space between = and null

this should happen automatically if you run ./gradlew format . Also it should happen automatically in intellij if you follow these instructions https://docs.airbyte.com/contributing-to-airbyte/code-style

sherifnada

LGTM! should be good to go after following the checklist.

grishick · 2022-03-07T23:16:24Z

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1948492727
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1948492727
No Python unittests run

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

grishick · 2022-03-09T23:34:34Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1960262285
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1960262285

grishick · 2022-03-10T01:48:11Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1960689221
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1960689221

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

grishick · 2022-03-10T19:02:46Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1964977046
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1964977046

* add testIAMUserHasListObjectPermission method to S3Destination and call this method from S3Destination::check. Method throws an exception if IAM user does not have listObjects permission on the destination bucket * add a unit test to S3DestinationTest to verify that S3Destination::check fails if listObjects throws an exception * add a unit test to S3DestinationTest to verify that S3Destination::check succeeds if listObjects succeeds * Add S3DestinationConfigFactory in order to be able to mock S3 client used in S3Destination::check

- separate positive and negative unit tests - fix formatting - reuse s3 client for both positive and negative tests

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

grishick · 2022-03-11T21:56:23Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1971187036
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1971187036

…bucket (#10856) * Add a test for listObjects permission to destination-s3 connector * add testIAMUserHasListObjectPermission method to S3Destination and call this method from S3Destination::check. Method throws an exception if IAM user does not have listObjects permission on the destination bucket * add a unit test to S3DestinationTest to verify that S3Destination::check fails if listObjects throws an exception * add a unit test to S3DestinationTest to verify that S3Destination::check succeeds if listObjects succeeds * Add S3DestinationConfigFactory in order to be able to mock S3 client used in S3Destination::check * Addressing review comments: - separate positive and negative unit tests - fix formatting - reuse s3 client for both positive and negative tests * Add information about PR #10856 to the changelog * Prepare for publishing new version: * Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog * Update destination-s3 version in connector index * Update seed spec for destination-s3 connector

github-actions bot added the area/connectors Connector related issues label Mar 4, 2022

grishick temporarily deployed to more-secrets March 4, 2022 16:53 Inactive

grishick requested review from sherifnada and ChristopheDuong March 4, 2022 16:53

sherifnada reviewed Mar 4, 2022

View reviewed changes

...ctors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3Destination.java Outdated Show resolved Hide resolved

grishick force-pushed the greg/check-listObjects-10665 branch from cf608b3 to 2d04a71 Compare March 4, 2022 21:41

grishick temporarily deployed to more-secrets March 4, 2022 21:42 Inactive

sherifnada reviewed Mar 5, 2022

View reviewed changes

grishick force-pushed the greg/check-listObjects-10665 branch from 2d04a71 to 77a7101 Compare March 7, 2022 20:56

grishick temporarily deployed to more-secrets March 7, 2022 20:58 Inactive

sherifnada approved these changes Mar 7, 2022

View reviewed changes

octavia-squidington-iii temporarily deployed to more-secrets March 7, 2022 23:18 Inactive

grishick pushed a commit that referenced this pull request Mar 7, 2022

Add information about PR #10856 to the changelog

d2568f8

github-actions bot added the area/documentation Improvements or additions to documentation label Mar 7, 2022

grishick temporarily deployed to more-secrets March 7, 2022 23:27 Inactive

grishick pushed a commit that referenced this pull request Mar 9, 2022

Add information about PR #10856 to the changelog

c8bf2e5

grishick force-pushed the greg/check-listObjects-10665 branch from a2a76a9 to e5a0bc6 Compare March 9, 2022 22:13

github-actions bot referenced this pull request Mar 9, 2022

Prepare for publishing new version:

e5a0bc6

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

grishick pushed a commit that referenced this pull request Mar 10, 2022

Add information about PR #10856 to the changelog

06b9b69

grishick force-pushed the greg/check-listObjects-10665 branch from e5a0bc6 to 94af7d5 Compare March 10, 2022 18:17

grishick temporarily deployed to more-secrets March 10, 2022 18:18 Inactive

grishick temporarily deployed to more-secrets March 10, 2022 18:19 Inactive

github-actions bot referenced this pull request Mar 10, 2022

Prepare for publishing new version:

94af7d5

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

grishick pushed a commit that referenced this pull request Mar 10, 2022

Add information about PR #10856 to the changelog

3cb44f8

grishick force-pushed the greg/check-listObjects-10665 branch from 94af7d5 to 444ef03 Compare March 10, 2022 23:04

grishick temporarily deployed to more-secrets March 10, 2022 23:05 Inactive

github-actions bot referenced this pull request Mar 10, 2022

Update seed spec for destination-s3 connector

444ef03

grishick added 6 commits March 11, 2022 10:13

Addressing review comments:

7067ec9

- separate positive and negative unit tests - fix formatting - reuse s3 client for both positive and negative tests

Add information about PR #10856 to the changelog

2977276

Prepare for publishing new version:

8fbc66a

* Bump version to 0.2.10 in Dockerfile * Bump version to 0.2.10 in changelog

Update destination-s3 version in connector index

7ea8f7f

Update seed spec for destination-s3 connector

5444bd2

grishick force-pushed the greg/check-listObjects-10665 branch from 444ef03 to 5444bd2 Compare March 11, 2022 18:26

grishick temporarily deployed to more-secrets March 11, 2022 18:27 Inactive

grishick merged commit f48a6a0 into master Mar 11, 2022

grishick deleted the greg/check-listObjects-10665 branch March 11, 2022 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

destination-s3: add a test for listObjects permission on destination bucket #10856

destination-s3: add a test for listObjects permission on destination bucket #10856

grishick commented Mar 4, 2022 •

edited

Loading

sherifnada Mar 5, 2022

grishick Mar 7, 2022

sherifnada Mar 5, 2022

grishick Mar 7, 2022

sherifnada Mar 5, 2022

grishick Mar 7, 2022

sherifnada left a comment

grishick commented Mar 7, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 9, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 10, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 10, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 11, 2022 •

edited by github-actions bot

Loading

destination-s3: add a test for listObjects permission on destination bucket #10856

destination-s3: add a test for listObjects permission on destination bucket #10856

Conversation

grishick commented Mar 4, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Tests

sherifnada Mar 5, 2022

Choose a reason for hiding this comment

grishick Mar 7, 2022

Choose a reason for hiding this comment

sherifnada Mar 5, 2022

Choose a reason for hiding this comment

grishick Mar 7, 2022

Choose a reason for hiding this comment

sherifnada Mar 5, 2022

Choose a reason for hiding this comment

grishick Mar 7, 2022

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

grishick commented Mar 7, 2022 • edited by github-actions bot Loading

grishick commented Mar 9, 2022 • edited by github-actions bot Loading

grishick commented Mar 10, 2022 • edited by github-actions bot Loading

grishick commented Mar 10, 2022 • edited by github-actions bot Loading

grishick commented Mar 11, 2022 • edited by github-actions bot Loading

grishick commented Mar 4, 2022 •

edited

Loading

grishick commented Mar 7, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 9, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 10, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 10, 2022 •

edited by github-actions bot

Loading

grishick commented Mar 11, 2022 •

edited by github-actions bot

Loading