Allow the Docker driver to run containers with additional groups

[dnrce]

This option is equivalent to docker run --group-add.

---

This is my first attempt at contributing to Nomad, my first contribution in Go anywhere, and in fact my first shot at doing anything in Go, period, so please be gentle. 🙂

A few questions, assuming this change is even directionally correct:

• Have I covered all the areas that need changes?
• Are these tests remotely sane?
• Does this new option have the best name? I haven't thought extremely hard about alternatives but group_add didn't seem like the right fit. https://docs.docker.com/engine/reference/run/ describes the option as "Add additional groups to run as."
• Is master the correct branch to target to get this into a near release?

Thanks!

[hashicorp-cla]

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

[gogococo]

I'm not going to do a code review as I'm not familiar enough with our code base, tests or Go at the moment.
Just wanted to drop in to say congrats, great job on putting together such a big first! Keep it up :)

[tgross]

• Have I covered all the areas that need changes?
• Are these tests remotely sane?

Yup, this looks great! I really appreciate that you've also updated the docs (and that you even paid attention things like how the options were ordered in the docs!).

I've checked out this branch and run an example job with and without the config option and the resulting container has the expected GroupAdd config values. So from a technical standpoint this LGTM.

Does this new option have the best name? I haven't thought extremely hard about alternatives but group_add didn't seem like the right fit.

This is tough... my gut is to say that we want the config options for Docker to match the docker run flags as closely as possible, because then when folks see the option they can mentally map it easily. Which would suggest we should use group_add instead of additional_groups. But maybe @notnoop or @schmichael have a second opinion here though?

Is master the correct branch to target to get this into a near release?

Yes! We generally try to land everything on master, and then whomever merges this (probably me) will be responsible for cherry-picking into the next release branch as needed. We intentionally try to make that "not your problem" 😀

[tgross]

This whole if err != nil block can be reduced to require.NoError(t, err)

[dnrce]

Copypasta -- updated.

[tgross]

Oh, and make sure you sign the CLA -- we'll need that done before we can merge this!

[dnrce]

Thanks for the review!

This is tough... my gut is to say that we want the config options for Docker to match the docker run flags as closely as possible, because then when folks see the option they can mentally map it easily. Which would suggest we should use group_add instead of additional_groups.

I'm happy to update to whatever is decided. I'll also throw out added_groups as another possibility that splits the difference between the underlying Docker option name and the declarative format of the driver config.

Oh, and make sure you sign the CLA -- we'll need that done before we can merge this!

Noted -- I'm contributing as an Oracle employee here so I'm just checking internally to make sure all my ducks are in a row before I do.

[schmichael]

Which would suggest we should use group_add instead of additional_groups. But maybe @notnoop or @schmichael have a second opinion here though?

For better or worse we try to match docker run, so +1 for group_add

I can't think of any security implications, but I'd love confirmation from someone else I'm not forgetting something.

(Also: thanks for the contribution! Great job!)

[tgross]

It's been a little over two years without the CLA being signed on this, so I suspect it's safe to unfortunately close it out by now. If we want to implement this we may want to do it as a cleanroom implementation.

[github-actions]

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.