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

Fix AWS ECR private repository usage #858

Merged
merged 7 commits into from
Mar 8, 2016
Merged

Fix AWS ECR private repository usage #858

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2bb234
allows AWS ECR to work
ErikEvenson Feb 27, 2016
2ae1f44
refactors
ErikEvenson Feb 27, 2016
bfbd4d6
updates docs
ErikEvenson Feb 27, 2016
4aa575b
Merge branch 'master' into study/aws-ecr-1
ErikEvenson Feb 29, 2016
d5142c2
adds comments and removes redundant false flag setting
ErikEvenson Feb 29, 2016
5859c95
Merge branch 'master' into study/aws-ecr-1
ErikEvenson Mar 7, 2016
d1f2843
moves transform to Init()
ErikEvenson Mar 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion client/driver/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type DockerDriverConfig struct {
LabelsRaw []map[string]string `mapstructure:"labels"` //
Labels map[string]string `mapstructure:"-"` // Labels to set when the container starts up
Auth []DockerDriverAuth `mapstructure:"auth"` // Authentication credentials for a private Docker registry
SSL bool `mapstructure:"ssl"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add this.

}

func (c *DockerDriverConfig) Validate() error {
Expand Down Expand Up @@ -408,6 +409,14 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle
if err := mapstructure.WeakDecode(task.Config, &driverConfig); err != nil {
return nil, err
}

if strings.Contains(driverConfig.ImageName, "https://") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this logic out of validate into the createContainer method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This transformation has to occur before the image is pulled and inspected (around line 496) and therefore before the createContainer method is called (around line 543).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ErikEvenson Oh I see. Can we create another method called Init() then? This could be called before Validate()

driverConfig.SSL = true
driverConfig.ImageName = strings.Replace(driverConfig.ImageName, "https://", "", 1)
} else {
driverConfig.SSL = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to set this as false as default value for bools are false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed -- will change.

}

image := driverConfig.ImageName

if err := driverConfig.Validate(); err != nil {
Expand Down Expand Up @@ -471,7 +480,14 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle
if authConfigurations, err = docker.NewAuthConfigurations(f); err != nil {
return nil, fmt.Errorf("Failed to create docker auth object: %v", err)
}
if authConfiguration, ok := authConfigurations.Configs[repo]; ok {

authConfigurationKey := ""
if driverConfig.SSL {
authConfigurationKey += "https://"
}

authConfigurationKey += strings.Split(driverConfig.ImageName, "/")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does strings.Split(driverConfig.ImageName, "/")[0] return the repo? In that case we could just use repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the split command returns the image string with the url scheme and without the url path -- which is different from the repo string.

if authConfiguration, ok := authConfigurations.Configs[authConfigurationKey]; ok {
authOptions = authConfiguration
}
} else {
Expand Down
4 changes: 2 additions & 2 deletions website/source/docs/drivers/docker.html.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ task "webservice" {

The following options are available for use in the job specification.

* `image` - The Docker image to run. The image may include a tag or custom URL.
* `image` - The Docker image to run. The image may include a tag or custom URL and should include `https://` if required.
By default it will be fetched from Docker Hub.

* `command` - (Optional) The command to run when starting the container.
Expand Down Expand Up @@ -248,7 +248,7 @@ The `docker` driver has the following host-level configuration options:
location).

* `docker.auth.config` - Allows an operator to specify a json file which is in
the dockercfg format containing authentication information for private registry.
the dockercfg format containing authentication information for private registry.

* `docker.tls.cert` - Path to the server's certificate file (`.pem`). Specify
this along with `docker.tls.key` and `docker.tls.ca` to use a TLS client to
Expand Down