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

provide a region hint of "us-west-2" in GetBucketRegion example #2716

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

kaovilai
Copy link
Contributor

@kaovilai kaovilai commented Jul 17, 2024

For changes to files under the /codegen/aws-models folder, and manual edits to autogenerated code (e.g. /service/s3/api.go) please create an Issue instead of a PR for those type of changes.

Related work: vmware-tanzu/velero-plugin-for-aws#210

If the PR addresses an existing bug or feature, please reference it here.

To help speed up the process and reduce the time to merge please ensure that Allow edits by maintainers is checked before submitting your PR. This will allow the project maintainers to make minor adjustments or improvements to the submitted PR, allow us to reduce the roundtrip time for merging your request.

@kaovilai kaovilai requested a review from a team as a code owner July 17, 2024 13:53
Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

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

Why the addition of AnonymousCredentials in the example? That's going to cause any requests against non-public buckets to fail here.

@kaovilai
Copy link
Contributor Author

kaovilai commented Jul 17, 2024

The anonymous credentials was added because if you don't have a specified ~/.aws/credentials, this call would fail with the current example prior to this PR.

Error only went away once a credential provider is defined when loading config. If you say it's needed for non-public buckets, I can make a note in the PR. WDYT?

Your feedback above will also help inform my patch elsewhere referenced above.

@kaovilai
Copy link
Contributor Author

// The request will not be signed, and will not use your AWS credentials.

That's going to cause any requests against non-public buckets to fail here.

So I guess creds could be used on a needed basis then?

@lucix-aws
Copy link
Contributor

I think it entirely depends on the bucket. According to HeadBucket docs, ("authentication and authorization" section) normal auth rules apply for HeadBucket, i.e. unauthorized operations will only work against non-public buckets.

I did notice the docs on our GetBucketRegion say "this will be anonymous, we won't sign the request" which is definitely wrong off the cuff. That should probably be removed.

What context are you using this in? I saw the PR downstream but don't really have any idea what that product is or what it's doing. I assume it's failing without anonymous config because the environment it's running in isn't configured with AWS credentials, but I haven't seen the underlying error. Not sure if that's intended or not. Are the buckets you're trying to query against public?

@kaovilai
Copy link
Contributor Author

kaovilai commented Jul 17, 2024

So I have two downstream PRs

I need to dynamically do config depending on env I guess. If in credentialess unit tests, anonymous.. otherwise do not put in anonymous credentials.

@lucix-aws
Copy link
Contributor

That makes sense to me.

So in terms of this PR:

  • adding region hint in the example is good
  • let's remove the example's use of Anonymous
  • you're welcome to remove the blurb in this API doc where it states // The request will not be signed, and will not use your AWS credentials. because that's definitely wrong - but you're equally welcome to just leave it for now

@kaovilai
Copy link
Contributor Author

@kaovilai kaovilai requested a review from lucix-aws July 17, 2024 17:15
@lucix-aws lucix-aws merged commit 17cc515 into aws:main Jul 24, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants