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

Include check on s3:listBucket permission in check_connection of destination-s3 #10665

Closed
ChristopheDuong opened this issue Feb 25, 2022 · 5 comments
Assignees
Labels
area/connectors Connector related issues connectors/destination/s3 type/enhancement New feature or request

Comments

@ChristopheDuong
Copy link
Contributor

ChristopheDuong commented Feb 25, 2022

Tell us about the problem you're trying to solve

The S3 destination connector occasionally needs s3:listBucket permissions on an S3 bucket. This is in the case we run the connector with the OVERWRITE sync mode. To perform the overwrite, it lists & deletes all objects in the configured bucket directory, then writes the new data.

The connector currently does not actually verify that the input IAM user has list 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.

Following up on:
#10627

which came from this oncall issue: airbytehq/alpha-beta-issues#7

Describe the solution you’d like

Since the reset operation does a call to final List<S3ObjectSummary> objects = s3Client.listObjects(bucket, outputPrefix), we need to verify the input user has list bucket permissions.

Implementation hints

  1. Create a method in S3Destination.java called testIAMUserHasListObjectPermission which returns nothing if it succeeds and throws an exception if it fails. Add it to the check method.
  2. Write a unit test which verifies the check method returns the correct value if the listObject call fails to s3. You may need to add as a constructor param for the class a factory which returns an S3DestinationConfig. This would allow you to mock the S3Client returned.
  3. Create a PR and follow the steps in the PR checklist for updating a connector to push the change to the finish line
  4. Congrats!
@ChristopheDuong ChristopheDuong changed the title Include check on s3:listObject permission in check_connection of destination-s3 Include check on s3:listBucket permission in check_connection of destination-s3 Feb 25, 2022
@sherifnada sherifnada added this to the ConnCore March 2, 2022 milestone Mar 1, 2022
@sherifnada
Copy link
Contributor

We should also make sure the user configured for staging in AWS has the proper permissions to do reset jobs (not just syncs)

We're going to bump this outside the scope of this ticket, it should be done as part of a separate ticket.

@grishick could you create a separate issue to address this piece?

@ChristopheDuong
Copy link
Contributor Author

We're going to bump this outside the scope of this ticket, it should be done as part of a separate ticket.

@grishick could you create a separate issue to address this piece?

The s3:listBucket permission is needed:

  • for overwrite sync modes
  • for append sync modes, only when reset jobs with this connector, I don't think we need a separate issue?

@sherifnada
Copy link
Contributor

@ChristopheDuong maybe I misunderstood- when you said staging do you mean for other destinations like redshift or did you mean for the s3 destination directly?

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 2, 2022

Oh ok, I guess I am the one that misunderstood you.

This ticket was only for the destination-s3.
For staging options from other destinations (using destination-s3 as underlying implementation), we may indeed need an extra ticket yes.

@sherifnada
Copy link
Contributor

Ok then we are just agreeing very vigorously then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues connectors/destination/s3 type/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants