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

s3 workspace_key_prefix #20432

Merged
merged 2 commits into from
Feb 23, 2019
Merged

s3 workspace_key_prefix #20432

merged 2 commits into from
Feb 23, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 22, 2019

The handling of slashes was broken around listing workspaces in
workspace_key_prefix. While it worked in most places by splitting an
extra time around the spurious slashes, it failed in the case that the
prefix ended with a slash of its own.

A test was temporarily added to verify that the backend now works with the
unusual keys, but rather than risking silent breakage around prefixes
with trailing slashes, we also add validation to prevent users from
entering keys with trailing slashes at all.

--- PASS: TestBackend_impl (0.00s)
--- PASS: TestBackendConfig (1.08s)
--- PASS: TestBackendConfig_invalidKey (0.00s)
--- PASS: TestBackend (18.43s)
--- PASS: TestBackendLocked (27.05s)
--- PASS: TestBackendExtraPaths (16.82s)
--- PASS: TestBackendPrefixInWorkspace (6.02s)
--- PASS: TestKeyEnv (58.52s)
--- PASS: TestRemoteClient_impl (0.00s)
--- PASS: TestRemoteClient (6.58s)
--- PASS: TestRemoteClientLocks (19.80s)
--- PASS: TestForceUnlock (18.84s)
--- PASS: TestRemoteClient_clientMD5 (14.00s)
--- PASS: TestRemoteClient_stateChecksum (21.14s)

related to #17508

The 0.12 update left this test broken. Make sure we're working with the
correct objects here.
The handling of slashes was broken around listing workspaces in
workspace_key_prefix. While it worked in most places by splitting an
extra time around the spurious slashes, it failed in the case that the
prefix ended with a slash of its own.

A test was temporarily added to verify that the backend works with the
unusual keys, but rather than risking silent breakage around prefixes
with trailing slashes, we also add validation to prevent users from
entering keys with trailing slashes at all.
@jbardin jbardin requested review from bflad, radeksimko and a team February 22, 2019 15:40
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

It seems like technically this a breaking change for anyone who was using leading/trailing slashes in a way that managed to work before in spite of being incorrect? I think that's okay if so -- we're about to do a major release anyway -- but would warrant a "Backward Incompatibilities / Notes" entry in the changelog, perhaps.

@jbardin
Copy link
Member Author

jbardin commented Feb 23, 2019

Yup. I purposely made the validation change in order to force it to show up for users, in case there was yet still missing some edge case around the slashes and splits that wasn't covered yet.

@jbardin jbardin merged commit 9c0e3cc into master Feb 23, 2019
@jbardin jbardin deleted the jbardin/s3-prefix-key branch February 23, 2019 02:00
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants