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.

Sign up for GitHub

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

Validate Elasticsearch resource names #1647

Merged
merged 7 commits into from
Sep 4, 2019
Merged

Validate Elasticsearch resource names #1647

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8185d24
Validate resource names
charith-elastic Aug 27, 2019
de6f51e
Avoid panics and truncate long names
charith-elastic Aug 28, 2019
aea00b3
Fix sset ordinal suffix length calculation
charith-elastic Aug 29, 2019
ac6b022
Account for dash in sset ordinal suffix length
charith-elastic Aug 29, 2019
8d13c6b
Update limit and add E2E test
charith-elastic Sep 2, 2019
2f672ab
Explain length restrictions
charith-elastic Sep 3, 2019
c3b53f7
Fix test failure
charith-elastic Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/controller/elasticsearch/name/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ func Validate(es v1alpha1.Elasticsearch) error {

// length of the ordinal suffix that will be added to the pods of this sset
podOrdinalSuffixLen := len(strconv.FormatInt(int64(nodeSpec.NodeCount), 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be nodeSpec.NodeCount - 1? We start numbering from 0, so for 100 nodes the last ordinal will be 99.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was intentional to account for the dash before the ordinal but now that I think about it, it doesn't work for node counts below 10.

if nodeSpec.NodeCount < 10 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I follow why this is needed. I'd think you always need to account for dash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I made the mistake of thinking in round numbers only.

podOrdinalSuffixLen++ // account for the dash before the ordinal
}
// there should be enough space for the ordinal suffix
if validation.DNS1123SubdomainMaxLength-len(ssetName) < podOrdinalSuffixLen {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this account for dash before the ordinal?

return fmt.Errorf("generated StatefulSet name '%s' exceeds allowed length of %d",
Expand Down