s3: websiteRoutingRule doesn't allow empty keyPrefixEquals

[dbartholomae]

Describe the bug

When setting up an S3 bucket with

        {
          condition: { keyPrefixEquals: "" },
          protocol: RedirectProtocol.HTTPS,
          hostName: "example.com",
          replaceKey: ReplaceKey.with("/"),
        },

it causes

Error: The condition property cannot be an empty object

Expected Behavior

It should work.

Current Behavior

Error: The condition property cannot be an empty object

Reproduction Steps

Set up a bucket with

        {
          condition: { keyPrefixEquals: "" },
          protocol: RedirectProtocol.HTTPS,
          hostName: "example.com",
          replaceKey: ReplaceKey.with("/"),
        },

Possible Solution

In this line:
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L2321

it should be

if (rule.condition && rule.condition.httpErrorCodeReturnedEquals != null && rule.condition.keyPrefixEquals != null) {

instead of

if (rule.condition && !rule.condition.httpErrorCodeReturnedEquals && !rule.condition.keyPrefixEquals) {

Additional Information/Context

No response

CDK CLI Version

2.72.1

Framework Version

No response

Node.js Version

16.14.12

OS

Mac

Language

Typescript

Language Version

No response

Other information

No response

[dbartholomae]

I've set up #26243 to fix this.

[peterwoodworth]

@dbartholomae I can see why this is running into the error and why your proposed fix would prevent the error from throwing, however I'm not sure if it would work at deployment since I've never tried this. I can't find any documentation on supplying an empty key prefix - do you know if this works in the API / CloudFormation? Are you sure it shouldn't be provided as /?

[dbartholomae]

@peterwoodworth good question! I know that I can set this manually. I'll try to set it via CloudFormation to confirm tomorrow.

[peterwoodworth]

We'll probably require an integration test for this regardless, maybe setting that up on your PR could be the fastest way to find out 🤔

[dbartholomae]

Hmm, I'll give it a look, but it seems like S3 doesn't have any integration tests yet, and I don't think I will be able to set up all fresh integrations tests for a one-line bug fix.

[peterwoodworth]

We've moved our integration tests here, hope this will make it easier!

[dbartholomae]

Thanks! I might be able to add a test that proves that the bucket can be created with an empty string value without value, but am unsure how to test that the redirect works correctly. Also, I'm still struggling to get integration tests to work locally on my end, so I might not be able to create the correct snapshots.

[peterwoodworth]

If you're struggling to run them, don't worry, we will be able to in that case.

Typically, our integ tests are just testing for successful deployment, without checking if specific things actually are configured correctly. It's not ideal, but it does help catch stuff. I suspect that if we cannot set up the template the way you propose, then the deployment will fail. You're right though that this won't guarantee that it actually works.

edit: For clarity, you won't have to write a check for the redirect in the integ test

[dbartholomae]

I've added an integration test based on your comment and added it to the PR :)

[peterwoodworth]

Yeah I tested on my own stack and it does appear this will work. The policy is accepted as valid! Though I didn't try the actual redirect mechanism since I don't have an easy way to set that up right now.

@dbartholomae we'll need a unit test on the PR as well, ping me again tomorrow and I can run the integ test for you

[dbartholomae]

I've just pushed a unit test

[mrgrain]

I'm cool with this change and the test coverage on the PR.

But could you provide some context on what the empty key would achieve here? I wouldn't even know where to start to manually verify this.

[dbartholomae]

Not sure if it is intended behaviour for S3, but the empty key matches with a redirect for the root domain while / doesn't seem to.

[mrgrain]

Okay, so the goal is that requests to the root of the bucket (i.e. that do not contain an object key) are redirected to a URL.
And everything else should return the regular S3 response?

[dbartholomae]

The goal in this case is that every single request is redirected to the same domain (not only the root).

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.