🐛Destination-gcs\destination-bigquery(gcs) - updated check() method to handle that user has both storage.objects.create and storage.multipartUploads.create roles #9121

etsybaev · 2021-12-27T12:57:23Z

What

On write the S3StreamTransferManagerHelper is performing a multipart upload.
On GCS storage.multipartUploads.create permission is required to write an object to the bucket, the check only requires storage.objects.create permission.
If the service account has storage.objects.create permission but not storage.multipartUploads.create the check will pass but the sync will fail.

How

Updated check method to check both roles.

!!!!!! TODO once approved !!!!!!!!:

perform final test for user with missed roles for Multipart upload
pass PR checklists
update versions
update changelog
publish destination-gcs, destination-bigquery and destination-bigquery-denormalized

Also tested locally on UI:
here is the result for user without multipart upload role (fails with AccessDenied and printed expected roles):

Here is the result for user with all expected roles (verification passed):

User Impact

No breaks backward compatibility expected

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

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 connector is published, connector added to connector index 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

Updating a connector

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

…thod to handle that user has both storage.objects.create and storage.multipartUploads.create roles

...rs/destination-gcs/src/main/java/io/airbyte/integrations/destination/gcs/GcsDestination.java

alexandertsukanov · 2021-12-28T16:29:54Z

Finished with review, left comments.

alexandertsukanov · 2021-12-28T16:30:03Z

CC: @etsybaev

alexandr-shegeda

LGTM. we can proceed with publishing and merging without airbyte review

tuliren · 2022-01-04T16:03:04Z

airbyte-integrations/connectors/destination-gcs/build.gradle

@@ -12,6 +12,7 @@ application {
 dependencies {
    implementation project(':airbyte-config:models')
    implementation project(':airbyte-protocol:models')
+    implementation project(':airbyte-workers')


It looks like the worker is not needed?

Hi @tuliren.
I've added a new test for insufficient roles verification that calls runCheck method. If fails like this if airbyte-workers haven't been added.

But you're right, that this is not required for the whole project, moved it to integrationTestJavaImplementation dependency.
Thanks

…-roles

etsybaev · 2022-01-10T17:31:42Z

/test connector=connectors/destination-gcs

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

…-roles

etsybaev · 2022-01-10T18:00:30Z

/test connector=connectors/destination-gcs

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

etsybaev · 2022-01-10T18:18:17Z

/publish connector=connectors/destination-gcs

🕑 connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1678835314
✅ connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1678835314

etsybaev · 2022-01-10T18:38:22Z

/test connector=connectors/destination-bigquery

🕑 connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/1678911770
✅ connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/1678911770
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                              Stmts   Miss  Cover
	 -------------------------------------------------------------------------------------
	 main_dev_transform_catalog.py                                         3      3     0%
	 main_dev_transform_config.py                                          3      3     0%
	 normalization/__init__.py                                             4      0   100%
	 normalization/destination_type.py                                    13      0   100%
	 normalization/transform_catalog/__init__.py                           2      0   100%
	 normalization/transform_catalog/catalog_processor.py                143     77    46%
	 normalization/transform_catalog/destination_name_transformer.py     155      8    95%
	 normalization/transform_catalog/reserved_keywords.py                 13      0   100%
	 normalization/transform_catalog/stream_processor.py                 520    333    36%
	 normalization/transform_catalog/table_name_registry.py              174     34    80%
	 normalization/transform_catalog/transform.py                         45     26    42%
	 normalization/transform_catalog/utils.py                             33      7    79%
	 normalization/transform_config/__init__.py                            2      0   100%
	 normalization/transform_config/transform.py                         148     34    77%
	 -------------------------------------------------------------------------------------
	 TOTAL                                                              1258    525    58%

etsybaev · 2022-01-10T18:38:41Z

/test connector=connectors/destination-bigquery-denormalized

🕑 connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
❌ connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
🐛

🕑 connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
❌ connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
🐛

🕑 connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
❌ connectors/destination-bigquery-denormalized https://github.com/airbytehq/airbyte/actions/runs/1678913040
🐛

etsybaev · 2022-01-10T19:07:16Z

/publish connector=connectors/destination-bigquery

🕑 connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/1679024050
✅ connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/1679024050

etsybaev · 2022-01-10T19:59:29Z

The bigquery-denormalized connector currently seems to have broken integration tests after recent merging another PR. So the version will be bumped as part of its fixing

mjaggard · 2022-01-19T21:32:12Z

@etsybaev This change seems to require either storage.multipartUploads.listParts or storage.multipartUploads.list or both. Without them but with the listed permissions I got Access Denied when trying to add a destination.

tuliren · 2022-01-20T01:45:19Z

@etsybaev This change seems to require either storage.multipartUploads.listParts or storage.multipartUploads.list or both. Without them but with the listed permissions I got Access Denied when trying to add a destination.

@mjaggard, the listParts and list should not be necessary. Only the following permissions are required:

Here is the doc:

This user or account will require the following permissions for the bucket:
storage.multipartUploads.abort
storage.multipartUploads.create
storage.objects.create
storage.objects.delete
storage.objects.get
storage.objects.list

tuliren · 2022-01-20T01:57:57Z

Actually let me double check that our integration test is using a service account that only has these six permissions.

tuliren · 2022-01-20T03:57:15Z

The integration test passed locally with the six permissions, but failed on CI. I will look into it. #9631

tuliren · 2022-01-24T04:16:32Z

I did not have time to fully investigate this last week. Will do it on Monday.

[9044] Destination-gcs\destination-bigquery(gcs) - updated check() me…

530829f

…thod to handle that user has both storage.objects.create and storage.multipartUploads.create roles

etsybaev requested a review from alexandr-shegeda December 27, 2021 12:57

etsybaev linked an issue Dec 27, 2021 that may be closed by this pull request

🐛 Destination GCS: Multi-part upload permissions are not validated by the connection check step #9044

Closed

github-actions bot added the area/connectors Connector related issues label Dec 27, 2021

etsybaev requested review from VitaliiMaltsev, andriikorotkov, alexandertsukanov and alexandr-shegeda and removed request for VitaliiMaltsev, alexandr-shegeda and andriikorotkov December 28, 2021 15:46

alexandertsukanov reviewed Dec 28, 2021

View reviewed changes

...rs/destination-gcs/src/main/java/io/airbyte/integrations/destination/gcs/GcsDestination.java Outdated Show resolved Hide resolved

alexandertsukanov reviewed Dec 28, 2021

View reviewed changes

...rs/destination-gcs/src/main/java/io/airbyte/integrations/destination/gcs/GcsDestination.java Outdated Show resolved Hide resolved

fixed code as per comments in PR

e9fd2da

etsybaev requested a review from alexandertsukanov December 30, 2021 17:08

etsybaev temporarily deployed to more-secrets December 30, 2021 17:09 Inactive

alexandr-shegeda approved these changes Jan 4, 2022

View reviewed changes

alexandr-shegeda marked this pull request as ready for review January 4, 2022 11:48

tuliren reviewed Jan 4, 2022

View reviewed changes

karinakuz added connectors/destination/bigquery connectors/destinations-api labels Jan 6, 2022

etsybaev added 3 commits January 8, 2022 11:59

Merge branch 'master' into etsybaev/9044-destination-gcs-update-check…

0860e2a

…-roles

Fixed check method

275a3ef

Fixed code style by running "./gradlew --no-daemon format --scan"

9675c5d

github-actions bot added the CDK Connector Development Kit label Jan 8, 2022

etsybaev temporarily deployed to more-secrets January 8, 2022 16:09 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 17:34 Inactive

etsybaev added 2 commits January 10, 2022 19:46

Merge branch 'master' into etsybaev/9044-destination-gcs-update-check…

1a2d744

…-roles

Bumped versions

a929b64

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

etsybaev temporarily deployed to more-secrets January 10, 2022 18:00 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 18:03 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 18:20 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 18:40 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 19:09 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2022 19:38 Inactive

reverted bumping version for bigquery-denormalized

d17d00d

etsybaev temporarily deployed to more-secrets January 10, 2022 19:59 Inactive

etsybaev merged commit 44cb30a into master Jan 10, 2022

etsybaev deleted the etsybaev/9044-destination-gcs-update-check-roles branch January 10, 2022 20:00

octavia-squidington-iii mentioned this pull request Jan 11, 2022

Bump Airbyte version from 0.35.4-alpha to 0.35.5-alpha #9421

Merged

karinakuz added connectors/destinations-warehouse and removed connectors/destinations-api labels Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛Destination-gcs\destination-bigquery(gcs) - updated check() method to handle that user has both storage.objects.create and storage.multipartUploads.create roles #9121

🐛Destination-gcs\destination-bigquery(gcs) - updated check() method to handle that user has both storage.objects.create and storage.multipartUploads.create roles #9121

etsybaev commented Dec 27, 2021 •

edited

Loading

alexandertsukanov commented Dec 28, 2021

alexandertsukanov commented Dec 28, 2021

alexandr-shegeda left a comment

tuliren Jan 4, 2022

etsybaev Jan 8, 2022

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022

mjaggard commented Jan 19, 2022

tuliren commented Jan 20, 2022 •

edited

Loading

tuliren commented Jan 20, 2022

tuliren commented Jan 20, 2022

tuliren commented Jan 24, 2022

🐛Destination-gcs\destination-bigquery(gcs) - updated check() method to handle that user has both storage.objects.create and storage.multipartUploads.create roles #9121

🐛Destination-gcs\destination-bigquery(gcs) - updated check() method to handle that user has both storage.objects.create and storage.multipartUploads.create roles #9121

Conversation

etsybaev commented Dec 27, 2021 • edited Loading

What

Recommended reading order:

How

User Impact

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

alexandertsukanov commented Dec 28, 2021

alexandertsukanov commented Dec 28, 2021

alexandr-shegeda left a comment

Choose a reason for hiding this comment

tuliren Jan 4, 2022

Choose a reason for hiding this comment

etsybaev Jan 8, 2022

Choose a reason for hiding this comment

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022 • edited by github-actions bot Loading

etsybaev commented Jan 10, 2022

mjaggard commented Jan 19, 2022

tuliren commented Jan 20, 2022 • edited Loading

tuliren commented Jan 20, 2022

tuliren commented Jan 20, 2022

tuliren commented Jan 24, 2022

etsybaev commented Dec 27, 2021 •

edited

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

etsybaev commented Jan 10, 2022 •

edited by github-actions bot

Loading

tuliren commented Jan 20, 2022 •

edited

Loading