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

Set user for rkt tasks #3612

Merged
merged 3 commits into from
Dec 5, 2017
Merged

Set user for rkt tasks #3612

merged 3 commits into from
Dec 5, 2017

Conversation

chelseakomlo
Copy link
Contributor

Further testing is necessary for each case

@@ -201,6 +201,7 @@ type DockerDriverConfig struct {
MacAddress string `mapstructure:"mac_address"` // Pin mac address to container
SecurityOpt []string `mapstructure:"security_opt"` // Flags to pass directly to security-opt
Devices []DockerDevice `mapstructure:"devices"` // To allow mounting USB or other serial control devices
User string `json:"User,omitempty" yaml:"User,omitempty" toml:"User,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

whats up with the yaml and toml here?

@chelseakomlo chelseakomlo changed the title WIP: Set user for Docker and rkt tasks Set user for Docker and rkt tasks Dec 4, 2017
@chelseakomlo
Copy link
Contributor Author

Testing suggestions are more than welcome.

Copy link
Contributor

@dadgar dadgar left a comment

Choose a reason for hiding this comment

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

You can test by specifying a non-existent user:

docker run --user alice redis:3.2
docker: Error response from daemon: linux spec user: unable to find user alice: no matching entries in passwd file.

@@ -201,6 +201,7 @@ type DockerDriverConfig struct {
MacAddress string `mapstructure:"mac_address"` // Pin mac address to container
SecurityOpt []string `mapstructure:"security_opt"` // Flags to pass directly to security-opt
Devices []DockerDevice `mapstructure:"devices"` // To allow mounting USB or other serial control devices
User string `mapstructure:user`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add a new config field instead of using the task.User?

@chelseakomlo
Copy link
Contributor Author

Changed this as we already expose this functionality for Docker, see https://www.nomadproject.io/docs/job-specification/task.html#user.

Added the change to pass this through for rkt.

@@ -569,6 +569,11 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse,

}

// If a user has been specified for the task, pass it through to the user
Copy link
Contributor

Choose a reason for hiding this comment

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

See if you can add tests to this and for docker

@chelseakomlo chelseakomlo changed the title Set user for Docker and rkt tasks Set user for rkt tasks Dec 4, 2017

select {
case res := <-resp.Handle.WaitCh():
assert.False(res.Successful())
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 assert it fails b/c the user

@chelseakomlo chelseakomlo merged commit fef15f4 into master Dec 5, 2017
@chelseakomlo chelseakomlo deleted the docker-rkt-user branch December 5, 2017 22:45
@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 Mar 17, 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.

3 participants