cni: allow users to set CNI args in job spec #23538
Conversation
martisah
added
backport/ent/1.8.x+ent
| newMap := make(map[string]string) | ||
| for k, v := range d.Args { | ||
| newMap[k] = v | ||
| } |
There was a problem hiding this comment.
There's a maps.Clone helper for this.
| if strings.Contains(v, ";") { | ||
| err := fmt.Errorf("invalid ';' character in CNI arg value %q", v) | ||
| mErr.Errors = append(mErr.Errors, err) | ||
| } |
There was a problem hiding this comment.
This illegal character check feels ad hoc; are we certain that this is an actual attack vector for CNI plugins? Also we're interpolating the values on the client (although not the keys). If I were an evil job author, I could have a value that includes a client attribute with a ; and bypass the check.
There was a problem hiding this comment.
my concern was more about ergonomics than security -- I wanted to protect job authors from doing something confusingly silly. the semicolon here isn't dangerous like it might be when parsing something like a shell command or sql query, but if a user put a ; in a key or value, then it could confuse plugins that read the args, since CNI_ARGS in the end is a ;-separated string of key=vals.
e.g. given the key "bad-test-key;is-bad" the (common) library that the bridge plugin uses hits this error and the Nomad client logs
[ERROR] client.alloc_runner: prerun failed: alloc_id=29822c8b-7192-150b-cc00-9a4e827384b0 error="pre-run hook "network" failed: failed to configure networking for alloc: failed to configure network: plugin type="bridge" failed (add): ARGS: invalid pair "bad-test-key""
a similar bad;value produces similar results.
any other code (plugin, library) that parses CNI_ARGS would have to do its own error handling like that, and could potentially produce dangerous results (we can't control the entire CNI ecosystem), but the goal here is just to error way earlier, before the job is accepted, to save the user some headache and troubleshooting.
the idea of a client attribute with a ; is interesting, but if you (the user) are considering client stuff like that, I suppose I'm more comfortable with you needing to hunt down client logs to track down the error?
all that said, to your point in slack, we'll put something like this comment in the code, too, to clarify the intent and danger level (low).
This PR allows for users to set up CNI args in a job specification through the network block by adding a cni block and specifying an args field.
Resolves #16197