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.

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

Expanded docker driver options #390

Merged
merged 4 commits into from
Nov 6, 2015

Conversation

Sdedelbrock
Copy link
Contributor

This pull request adds additional task level options for the docker driver. Addresses #328

  • Added privileged option to task config to allow containers to run in
    privileged mode.
  • Added dns-servers option to task config to allow containers to use
    custom DNS servers
  • Added search-domains option to task config to allow containers to
    use custom dns search domains
  • Added authentication options (under key namespace auth.*) to allow
    authentication on a task level for docker remote.
  • Added docker.privileged.enabled to client config
  • Updated site docs to reflect changes

Example Task Config with new options

...
driver = "docker"
config{
    privileged = "true"
    dns-servers = "8.8.8.8,8.8.4.4"
    search-domains = "hashicorp.com,google.com"

    auth.username = "un"
    auth.password = "pw"
    auth.email = "dockerhubuser@quay.io"
    auth.server-address = "quay.io"
}
...

Example Client Config with new options

...
`docker.privileged.enabled = "true"
...

- Added `priviliged` option to task config to allow containers to run in
 priviliged mode.
- Added `dns-servers` option to task config to allow containers to use
  custom DNS servers
- Added `search-domains` option to task config to allow containers to
  use custom dns search domains
- Added authentication options (under key namespace `auth.*`) to allow
  authentication on a task level for docker remote.
- Updated site docs to reflect changes
…cker-driver-options

* 'master' of https://github.com/hashicorp/nomad: (59 commits)
  Move the executor and spawn package into driver
  Remove file watching
  Check if the PID is alive instead of heartbeating through modify time
  Update CHANGELOG.md
  nomad/watch: add a note about the Item struct
  go fmt this file
  Vet errors
  Search path
  Update website
  Make a basic executor that can be shared and fix some fingerprinting/tests
  Small improvements
  Use const value for AWS metadata URL
  Create Spawn pkg that handles IPC with the spawn-daemon and update exec_linux to use that
  Fixed the restart policy syntax
  Introducing vars to create default batch and service restart policies
  Fixed the tests
  Declaring Batch and Service default restart policies
  Fixing tests to not create a TG without restart policies
  This option only work -> This option only works
  leave -> leaving
  ...
@@ -166,6 +167,32 @@ func (d *DockerDriver) createContainer(ctx *ExecContext, task *structs.Task) (do
d.logger.Printf("[DEBUG] driver.docker: using %d cpu shares for %s", hostConfig.CPUShares, task.Config["image"])
d.logger.Printf("[DEBUG] driver.docker: binding directories %#v for %s", hostConfig.Binds, task.Config["image"])

// set privileged (fallback to false)
hostConfig.Privileged, _ = strconv.ParseBool(task.Config["privileged"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you handle the error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since privileged is a potentially dangerous flag, we want to give the Nomad operator control over whether this is allowed. So what should happen is there should be a name key/value to enable or disable the docker driver from allowing the privileged flag:

That would go here: https://github.com/hashicorp/nomad/blob/master/client/config/config.go#L55
Here is an example:

_, err = strconv.ParseBool(d.config.ReadDefault("docker.cleanup.container", "true"))

And the default should be that privileged is not enabled. The docker driver should also set a node attribute to indicate if it allows privileged. Let me know if you need clarification

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes perfect sense, I'll update the PR

- Added error checking on priviliged mode.
- Added `docker.privileged.enabled` to client config/fingerprint
@@ -121,6 +141,11 @@ The `docker` driver has the following configuration options:
* `docker.cleanup.image` Defaults to `true`. Changing this to `false` will
prevent Nomad from removing images from stopped tasks.

* `docker.privileged.enabled` Defaults to `false`. Changing this to `true` will
allow containers to use "privileged" mode, which gives the containers full access
to the host
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put a period after

@dadgar
Copy link
Contributor

dadgar commented Nov 6, 2015

There is also a some build errors but coming together nicely 👍

returned wrong value, and forot a ":" :/
dadgar added a commit that referenced this pull request Nov 6, 2015
@dadgar dadgar merged commit 638f78e into hashicorp:master Nov 6, 2015
@dadgar
Copy link
Contributor

dadgar commented Nov 6, 2015

Thanks @Sdedelbrock

@github-actions
Copy link

github-actions bot commented May 6, 2023

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 May 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants