Skip to content

fix: Fallback to eu-west-1 if a given buckets LocationConstraint is EU#14476

Merged
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
AshCorr:aa/EU-Region-Buckets
Oct 11, 2023
Merged

fix: Fallback to eu-west-1 if a given buckets LocationConstraint is EU#14476
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
AshCorr:aa/EU-Region-Buckets

Conversation

@AshCorr
Copy link
Copy Markdown
Contributor

@AshCorr AshCorr commented Oct 9, 2023

Summary

When using the AWS Plugin we noticed that some of our buckets weren't being indexed by Cloudquery. After some investigation we noticed that those buckets seem to have a LocationConstraint of EU instead of an expected region such as eu-west-1

➜  aws git:(aa/EU-Region-Buckets) ✗ aws s3api get-bucket-location --bucket (redacted working bucket) --no-paginate

{
    "LocationConstraint": "eu-west-1"
}
➜  aws git:(aa/EU-Region-Buckets) ✗ aws s3api get-bucket-location --bucket (redacted broken bucket) --no-paginate

{
    "LocationConstraint": "EU"
}

This PR changes how we decide which region to use fetching s3 bucket data, falling back to eu-west-1 when a buckets LocationConstraint is set to EU.

Why?!

It looks like this fallback to eu-west-1 was a built in behaviour of the V1 AWS SDK https://github.com/aws/aws-sdk-go/blob/592707ce45b91664d30c270ad0057de033ce4dfe/service/s3/bucket_location.go#L23C6-L32 but was not ported to the V2 API which is used for Cloudquery aws/aws-sdk-go-v2#1473 (comment)

In the long run we should switch to the HeadBucket instead, which has this behaviour built in to the AWS API response.

Co-authored-by: akash1810 <akash1810@users.noreply.github.com>
@AshCorr
Copy link
Copy Markdown
Contributor Author

AshCorr commented Oct 9, 2023

Actually doesn't look too bad to switch to HeadBucket instead. DNM.

@jsonpr
Copy link
Copy Markdown
Contributor

jsonpr commented Oct 9, 2023

Hey @AshCorr - interesting find and thanks for the PR! Could you switch this PR to Draft for now or closing this until you're ready with the HeadBucket change or whichever change you think is appropriate at the time?

@jsonpr jsonpr requested a review from bbernays October 9, 2023 17:24
@AshCorr AshCorr marked this pull request as draft October 9, 2023 17:44
@bbernays
Copy link
Copy Markdown
Collaborator

bbernays commented Oct 9, 2023

@AshCorr - I think that switching to HeadBucket while is the recommended solution, it would require us to release a breaking version of the AWS plugin because users would be forced to change their permissions.

While I am not opposed to making that change, I think that we should immediately make the change you have proposed to correctly handle the EU value.

@AshCorr
Copy link
Copy Markdown
Contributor Author

AshCorr commented Oct 10, 2023

I think that switching to HeadBucket while is the recommended solution, it would require us to release a breaking version of the AWS plugin because users would be forced to change their permissions.

Ahh good point. I've re-opened the PR.

@AshCorr AshCorr marked this pull request as ready for review October 10, 2023 09:54
@AshCorr AshCorr changed the title fix: Fallback to eu-west-1 if Buckets RegionConstraint is EU fix: Fallback to eu-west-1 if a given buckets LocationConstraint is EU Oct 10, 2023
@bbernays bbernays added the automerge Automatically merge once required checks pass label Oct 10, 2023
@kodiakhq kodiakhq bot merged commit f6433e7 into cloudquery:main Oct 11, 2023
erezrokah added a commit that referenced this pull request Oct 13, 2023
🤖 I have created a release *beep* *boop*
---


##
[22.15.0](plugins-source-aws-v22.14.0...plugins-source-aws-v22.15.0)
(2023-10-13)


### Features

* Deleted query origin_access_identity_enabled.sql
([#13921](#13921))
([ec77ff8](ec77ff8))
* Introduce spec JSON schema
([#14296](#14296))
([c35f473](c35f473))
* **services:** Support newly added regions
([#14481](#14481))
([672772c](672772c))
* Updated query api_gw_cache_encrypted.sql
([#13860](#13860))
([dca3fe0](dca3fe0))
* Updated query of security_groups_with_access_to_unauthorized_ports
([#13855](#13855))
([efa9e34](efa9e34))
* Updated query of security_groups_with_open_critical_ports
([#13854](#13854))
([6834ee9](6834ee9))
* Updated query rds_databases_and_clusters_should_not_use_a_datab…
([#13936](#13936))
([810078f](810078f))
* Updated query secrets_configured_with_automatic_rotation_should…
([#13934](#13934))
([8b1293c](8b1293c))
* Updated query unused_acls.sql
([#13859](#13859))
([f47df0c](f47df0c))


### Bug Fixes

* Added check for empty string health_status
([#13861](#13861))
([37d8875](37d8875))
* Changed ssm.2 query to reduce redundent rows
([#13933](#13933))
([cee1fab](cee1fab))
* **deps:** Update github.com/cloudquery/arrow/go/v14 digest to d401686
([#14459](#14459))
([7ce40f8](7ce40f8))
* **deps:** Update module github.com/cloudquery/cloudquery-api-go to
v1.2.6 ([#14475](#14475))
([83fe7ca](83fe7ca))
* **deps:** Update module github.com/cloudquery/cloudquery-api-go to
v1.2.8 ([#14503](#14503))
([4056593](4056593))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.2
([#14378](#14378))
([a2e0c46](a2e0c46))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.3
([#14436](#14436))
([d529e2d](d529e2d))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.4
([#14489](#14489))
([9bb45dc](9bb45dc))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.5
([#14516](#14516))
([2d905bf](2d905bf))
* **deps:** Update module golang.org/x/net to v0.17.0 [SECURITY]
([#14500](#14500))
([9e603d5](9e603d5))
* Fallback to `eu-west-1` if a given buckets LocationConstraint is `EU`
([#14476](#14476))
([f6433e7](f6433e7))
* Lowercase policy statement in query for KMS.1
([#13858](#13858))
([b161fe1](b161fe1))
* Proper schema for `CustomECSListTasksInput.MaxResults`
([#14502](#14502))
([cdaaa99](cdaaa99))
* Redact Code Location and Fall back to other APIs to fully resolve
`aws_lambda_functions`
([#14381](#14381))
([bf402f4](bf402f4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
hydratim pushed a commit to hydratim/cloudquery that referenced this pull request Oct 20, 2023
… `EU` (cloudquery#14476)



#### Summary

When using the AWS Plugin we noticed that some of our buckets weren't being indexed by Cloudquery. After some investigation we noticed that those buckets seem to have a LocationConstraint of `EU` instead of an expected region such as `eu-west-1`

```bash
➜  aws git:(aa/EU-Region-Buckets) ✗ aws s3api get-bucket-location --bucket (redacted working bucket) --no-paginate

{
    "LocationConstraint": "eu-west-1"
}
```

```bash
➜  aws git:(aa/EU-Region-Buckets) ✗ aws s3api get-bucket-location --bucket (redacted broken bucket) --no-paginate

{
    "LocationConstraint": "EU"
}
```

This PR changes how we decide which region to use fetching s3 bucket data, falling back to `eu-west-1` when a buckets LocationConstraint is set to `EU`.

#### Why?!

It looks like this fallback to `eu-west-1` was a built in behaviour of the V1 AWS SDK https://github.com/aws/aws-sdk-go/blob/592707ce45b91664d30c270ad0057de033ce4dfe/service/s3/bucket_location.go#L23C6-L32 but was not ported to the V2 API which is used for Cloudquery aws/aws-sdk-go-v2#1473 (comment)

In the long run we should switch to the [HeadBucket](https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html) instead, which has this behaviour built in to the AWS API response.


<!--
Explain what problem this PR addresses
-->

<!--
hydratim pushed a commit to hydratim/cloudquery that referenced this pull request Oct 20, 2023
🤖 I have created a release *beep* *boop*
---


##
[22.15.0](cloudquery/cloudquery@plugins-source-aws-v22.14.0...plugins-source-aws-v22.15.0)
(2023-10-13)


### Features

* Deleted query origin_access_identity_enabled.sql
([cloudquery#13921](cloudquery#13921))
([ec77ff8](cloudquery@ec77ff8))
* Introduce spec JSON schema
([cloudquery#14296](cloudquery#14296))
([c35f473](cloudquery@c35f473))
* **services:** Support newly added regions
([cloudquery#14481](cloudquery#14481))
([672772c](cloudquery@672772c))
* Updated query api_gw_cache_encrypted.sql
([cloudquery#13860](cloudquery#13860))
([dca3fe0](cloudquery@dca3fe0))
* Updated query of security_groups_with_access_to_unauthorized_ports
([cloudquery#13855](cloudquery#13855))
([efa9e34](cloudquery@efa9e34))
* Updated query of security_groups_with_open_critical_ports
([cloudquery#13854](cloudquery#13854))
([6834ee9](cloudquery@6834ee9))
* Updated query rds_databases_and_clusters_should_not_use_a_datab…
([cloudquery#13936](cloudquery#13936))
([810078f](cloudquery@810078f))
* Updated query secrets_configured_with_automatic_rotation_should…
([cloudquery#13934](cloudquery#13934))
([8b1293c](cloudquery@8b1293c))
* Updated query unused_acls.sql
([cloudquery#13859](cloudquery#13859))
([f47df0c](cloudquery@f47df0c))


### Bug Fixes

* Added check for empty string health_status
([cloudquery#13861](cloudquery#13861))
([37d8875](cloudquery@37d8875))
* Changed ssm.2 query to reduce redundent rows
([cloudquery#13933](cloudquery#13933))
([cee1fab](cloudquery@cee1fab))
* **deps:** Update github.com/cloudquery/arrow/go/v14 digest to d401686
([cloudquery#14459](cloudquery#14459))
([7ce40f8](cloudquery@7ce40f8))
* **deps:** Update module github.com/cloudquery/cloudquery-api-go to
v1.2.6 ([cloudquery#14475](cloudquery#14475))
([83fe7ca](cloudquery@83fe7ca))
* **deps:** Update module github.com/cloudquery/cloudquery-api-go to
v1.2.8 ([cloudquery#14503](cloudquery#14503))
([4056593](cloudquery@4056593))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.2
([cloudquery#14378](cloudquery#14378))
([a2e0c46](cloudquery@a2e0c46))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.3
([cloudquery#14436](cloudquery#14436))
([d529e2d](cloudquery@d529e2d))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.4
([cloudquery#14489](cloudquery#14489))
([9bb45dc](cloudquery@9bb45dc))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.12.5
([cloudquery#14516](cloudquery#14516))
([2d905bf](cloudquery@2d905bf))
* **deps:** Update module golang.org/x/net to v0.17.0 [SECURITY]
([cloudquery#14500](cloudquery#14500))
([9e603d5](cloudquery@9e603d5))
* Fallback to `eu-west-1` if a given buckets LocationConstraint is `EU`
([cloudquery#14476](cloudquery#14476))
([f6433e7](cloudquery@f6433e7))
* Lowercase policy statement in query for KMS.1
([cloudquery#13858](cloudquery#13858))
([b161fe1](cloudquery@b161fe1))
* Proper schema for `CustomECSListTasksInput.MaxResults`
([cloudquery#14502](cloudquery#14502))
([cdaaa99](cloudquery@cdaaa99))
* Redact Code Location and Fall back to other APIs to fully resolve
`aws_lambda_functions`
([cloudquery#14381](cloudquery#14381))
([bf402f4](cloudquery@bf402f4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants