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

Jump to bottom

job, task group, and task IDs should not allow null characters #9020

Merged
merged 4 commits into from
Oct 5, 2020
Merged

job, task group, and task IDs should not allow null characters #9020

merged 4 commits into from
Oct 5, 2020

Conversation

cgbaker
Copy link
Contributor

@cgbaker cgbaker commented Oct 3, 2020 •
edited
Loading

resolves #8796

null characters in region, datacenter, job ID/name, task group name, and task name can break a lot of things, including:

  • memdb compound indexing
  • default task environment variables

changelog summary:

  • d4ef12e is a documenting test
  • 39c73f1 introduces the additional validation, but fails, because other tests actually rely on the ability to use null characters 😦 . also, updates CHANGELOG and upgrade guide.
  • 5062e74 relaxes the tests which protect this backwards compatibility
  • b0c2e51 adds additional tests for agent Region and Datacenter, as well as Job.Name

@cgbaker cgbaker added this to the 0.13 milestone Oct 3, 2020
@cgbaker cgbaker force-pushed the b-8796-i-do-not-want-no-null-chars branch 2 times, most recently from 717e5a8 to 61119f1 Compare October 3, 2020 22:14
@cgbaker cgbaker requested review from notnoop and tgross and removed request for notnoop October 4, 2020 00:48
tgross
tgross approved these changes Oct 5, 2020
View reviewed changes
nomad/structs/structs_test.go

// task name should not allow null characters
job.TaskGroups[0].Name = "so_much_better"
job.TaskGroups[0].Tasks[0].Name = "ive_had_it_with_these_\000_chars_in_these_names"
Copy link
Member

Choose a reason for hiding this comment

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

😆

nomad/structs/structs.go
@@ -3943,6 +3943,8 @@ func (j *Job) Validate() error {
mErr.Errors = append(mErr.Errors, errors.New("Missing job ID"))
} else if strings.Contains(j.ID, " ") {
mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a space"))
} else if strings.Contains(j.ID, "\000") {
mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a null chararacter"))
}
if j.Name == "" {
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't come up except when using the API, but Jobs have both a name and an ID. Should we be validating that Job.Name also doesn't contain a null char?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good point. i don't see any indexing there, but a quick test showed that null in Name put a docker job into a failing state:

2020-10-05T12:55:36Z  Driver Failure  Failed to start container cc77616db08d0701b20b438af789855e56eb343f7ce9c0636c779e399e052177: API error (500): OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:449: container init caused \"setenv: invalid argument\"": unknown

will update PR to include Job.Name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, in addition, the task drivers set the NOMAD_DC environment variable, which will fail if the node datacenter includes a null character 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and region 🤦++

@cgbaker cgbaker force-pushed the b-8796-i-do-not-want-no-null-chars branch from a048500 to fa9d0e7 Compare October 5, 2020 14:58
@cgbaker cgbaker requested a review from tgross October 5, 2020 16:31
tgross
tgross requested changes Oct 5, 2020
View reviewed changes
command/agent/command_test.go Outdated Show resolved Hide resolved
@cgbaker cgbaker force-pushed the b-8796-i-do-not-want-no-null-chars branch from fa9d0e7 to b0c2e51 Compare October 5, 2020 18:02
@cgbaker cgbaker requested a review from tgross October 5, 2020 18:02
tgross
tgross approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

@cgbaker cgbaker merged commit c195f93 into master Oct 5, 2020
@cgbaker cgbaker deleted the b-8796-i-do-not-want-no-null-chars branch October 5, 2020 19:54
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tgross tgross tgross approved these changes

@notnoop notnoop Awaiting requested review from notnoop

Assignees
No one assigned
Labels
theme/breaking-change
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

job ID allows null character
2 participants
@cgbaker @tgross