job: Sanitize instead of validate the job names #16
Conversation
| if mangled || len(runes) > maxNameLength { | ||
| // Name was mangled or is too long, truncate and append hash. | ||
| const hashLen = 10 | ||
| hash := fmt.Sprintf("%x", sha256.Sum256([]byte(name))) |
There was a problem hiding this comment.
Might want to avoid some "bad" words here, c. f. the cilium/cilium fix in cilium/cilium#35031 (comment)
There was a problem hiding this comment.
What are you referring to? What's a bad word?
There was a problem hiding this comment.
I was talking about https://github.com/cilium/cilium/pull/35031/files#diff-383201c3e15adcbd860745a3ce7762a5bcc54e5e94a9d7e57283abd20358f708R35-R55, but I'm not sure it's super relevant
| if mangled || len(runes) > maxNameLength { | ||
| // Name was mangled or is too long, truncate and append hash. | ||
| const hashLen = 10 | ||
| hash := fmt.Sprintf("%x", sha256.Sum256([]byte(name))) |
There was a problem hiding this comment.
I was talking about https://github.com/cilium/cilium/pull/35031/files#diff-383201c3e15adcbd860745a3ce7762a5bcc54e5e94a9d7e57283abd20358f708R35-R55, but I'm not sure it's super relevant
| { | ||
| name: "long name", | ||
| in: strings.Repeat("a", maxNameLength*2), | ||
| out: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-c2a908d98f", | ||
| }, |
There was a problem hiding this comment.
possibly worth adding some non-ascii unicode char tests
job/job.go
Outdated
| case r >= '0' && r <= '9': | ||
| case r == '-' || r == '_': | ||
| default: | ||
| // Replace unallowed characters with underscores. |
There was a problem hiding this comment.
What do you think about replacing only space with _ and simply ignore the other characters?
in: "X$%^&Y",
out: "X____Y-4af3ba2ac8",
I don't really see much value in just having lots of _ appended together.
There was a problem hiding this comment.
ah good point, I'll do that!
| runes := []rune(name) | ||
| mangled := false | ||
| for i, r := range runes[:min(maxNameLength, len(name))] { | ||
| switch { |
There was a problem hiding this comment.
Comparing with the previous regex, now we are allowing names to start with numbers and contain only numbers, though it's kind-a weird but acceptable. no action needed for this comment.
There was a problem hiding this comment.
Yeah, the implementation was much simpler this way so though that might as well allow these.
| hash := fmt.Sprintf("%x", sha256.Sum256([]byte(name))) | ||
| newLen := min(maxNameLength-hashLen, len(runes)) | ||
| runes = runes[:newLen] | ||
| return string(runes) + "-" + hash[:hashLen] |
There was a problem hiding this comment.
we could potentially prefix with something to highlight that this name is mangled?
There was a problem hiding this comment.
I think the suffix is enough.
There was a problem hiding this comment.
currently, the suffix is not highlighting that the name was mangled because the name is invalid, I was thinking to something like **-mangled-HASH
The job names may come from many different sources and it is too much of a foot-gun to reject the names by panicing. Instead to keep the names within a nice constraint set of characters and length, sanitize the names silently. Related PR that had to work around the panics: cilium/cilium#35031 Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/sanitize-job-name
branch
from
2a126de
to
416460f
Compare
No. We'll fix them as necessary. Right now we're not really caring about the job names anywhere. |
The job names may come from many different sources and it is too much of a foot-gun to reject the names by panicing.
Instead to keep the names within a nice constraint set of characters and length, sanitize the names silently.
Related PR that had to work around the panics:
cilium/cilium#35031