fix(s3): allow wildcard in imported buckets

[mina-asham]

• Allow using wildcards in imported buckets, this makes it easy to grant permissions using the helper methods
• Previous PR enforced a validation on bucket names, this stops the usecase of importing bucket names with wildcards to make granting permissions easier and more idiomatic: fix(s3): enforce that fromBucketAttributes supplies a valid bucket name #16915

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[otaviomacedo]

What happens when the string matches more than one bucket? And what if, instead of importing an existing bucket, the user tries to create a new Bucket() with a wildcard in the name?

[mina-asham]

What happens when the string matches more than one bucket? And what if, instead of importing an existing bucket, the user tries to create a new Bucket() with a wildcard in the name?

That's a good callout, I missed all the usages of that validation method, I think a safer approach here is to add an optional parameter to it to allow wildcard and only allow that for imports, how does that sound?

[otaviomacedo]

It's still unclear what to expect from something like:

Bucket.fromBucketAttributes(this, 'bucket', {
  bucketName: 'foo*'
});

What am I trying to convey here? "Get me whatever bucket whose name starts with 'foo'"? I don't think that makes sense.

[mina-asham]

It's still unclear what to expect from something like:

Bucket.fromBucketAttributes(this, 'bucket', {
  bucketName: 'foo*'
});

What am I trying to convey here? "Get me whatever bucket whose name starts with 'foo'"? I don't think that makes sense.

Yup, that's how I would read it exactly, get me all buckets that look like this and then I am going to reference that in permissions, it's also important to say that this "used" to work and s3 is a stable module according to the docs so the previous PR broke compatability for us, but I am happy with re-introducing that in a different way and updating if you have an idea of how to achieve this?

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: ff448aa
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mina-asham]

@otaviomacedo Any thoughts on the above?

[gshpychka]

Yup, that's how I would read it exactly, get me all buckets that look like this

But the Bucket resource represents a single bucket. I don't really understand the use case.

[mina-asham]

Yup, that's how I would read it exactly, get me all buckets that look like this

But the Bucket resource represents a single bucket. I don't really understand the use case.

My specific use case here is granting permissions without hand crafting the policy statements, maybe it wasn't the proper use or the use intended, but it was a functionality that was available in a "stable" module that is not working anymore (and btw still works in other modules, so you can do the same in KMS Key as well).

Beside the backward incompatibility this introduced, which I understand there is a strong reasoning for anyway because this was not an intended use case, maybe we should rework this as having some static helpers to grant read/write permissions for resources, hand crafting policy statements is against the spirit of CDK imo and should be avoided where possible.

[gshpychka]

FWIW, iam-floyd is great for generating policies.

[otaviomacedo]

Closing, as this can lead to non-deterministic behavior. If the community really thinks we need this behavior, we should create a new API, that returns an array of Buckets. But that would probably require the implementation of a new context provider, so I'm not sure it's worth it.

If someone feels this is an important feature to have, please open a feature-request issue and we will prioritize it accordingly.