Skip to content

Restricts naming for repositories#41008

Merged
danhermann merged 3 commits intoelastic:masterfrom
danhermann:repo_name_40817
Apr 26, 2019
Merged

Restricts naming for repositories#41008
danhermann merged 3 commits intoelastic:masterfrom
danhermann:repo_name_40817

Conversation

@danhermann
Copy link
Copy Markdown
Contributor

@danhermann danhermann commented Apr 9, 2019

Applies the same naming restrictions to repositories as to snapshots except that leading underscores and uppercase characters are permitted.

Fixes #40817.

@original-brownbear original-brownbear added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >bug v7.2.0 v8.0.0 labels Apr 9, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed

@original-brownbear original-brownbear self-requested a review April 9, 2019 12:28
@danhermann danhermann force-pushed the repo_name_40817 branch 5 times, most recently from 71392e2 to d1861ab Compare April 15, 2019 13:54
Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

@danhermann thanks! I think this is fine, can you add a quick test that checks these conditions as well please?

@original-brownbear
Copy link
Copy Markdown
Contributor

@ywelsch maybe you can take a quick look too, to confirm that it's fine to restrict naming like this (looks like the same conditions as for snapshot naming are in here as well which makes perfect sense to me)?

Copy link
Copy Markdown
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've left some comments. Thank you for contributing this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is already covered by filename check

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is already covered by filename check

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is a method that is internally called both on repository creation, but also after a restart when a node rejoins a cluster with an existing repository. I think we should do the validation only for new repository creation requests, and not retroactively apply to old repositories (which will break them). This means that the validation should be done in registerRepository, not createRepository.

…except that leading underscores and uppercase characters are permitted.

Fixes elastic#40817.
Copy link
Copy Markdown
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM2

@danhermann danhermann merged commit d922d4d into elastic:master Apr 26, 2019
@danhermann danhermann deleted the repo_name_40817 branch April 26, 2019 13:55
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
Applies the same naming restrictions to repositories as to snapshots except that leading underscores and uppercase characters are permitted.

Fixes elastic#40817.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Applies the same naming restrictions to repositories as to snapshots except that leading underscores and uppercase characters are permitted.

Fixes elastic#40817.
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.2.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repository name with commas can't be retrieved

5 participants