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

Add daemon option to push foreign layers #33151

Merged
merged 1 commit into from May 17, 2017

Conversation

@nwt
Contributor

nwt commented May 10, 2017

The --air-gapped-registry --allow-nondistributable-artifacts daemon option specifies registries to which foreign layers should be pushed. (By default, foreign layers are not pushed to registries.)

Additionally, to make this option effective, foreign layers are now pulled from the registry if possible, falling back to the URLs in the image manifest otherwise.

This option is useful when pushing images containing foreign layers to an air-gapped registry so that air-gapped hosts a registry on an air-gapped network so hosts on that network can pull the images without connecting to another server.

Caveats:

  1. "Air-gapped" might not be the right terminology for this.
  2. distribution/pull_v2_windows.go, distribution/push_v2.go, isAirGappedRegistry, and isCIDRMatch have no tests.
@nwt

This comment has been minimized.

Show comment
Hide comment
@nwt
Contributor

nwt commented May 10, 2017

@stevvooe stevvooe requested review from dmcgowan and cpuguy83 May 10, 2017

Show outdated Hide outdated docs/reference/commandline/dockerd.md Outdated
Show outdated Hide outdated docs/reference/commandline/dockerd.md Outdated
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 12, 2017

Contributor

Design LGTM

I think the changes to pull_v2 need test coverage.

Contributor

aaronlehmann commented May 12, 2017

Design LGTM

I think the changes to pull_v2 need test coverage.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 12, 2017

Contributor

Do we need to block hub here or is hub specifically ignoring non-distributable layers?

Contributor

cpuguy83 commented May 12, 2017

Do we need to block hub here or is hub specifically ignoring non-distributable layers?

@nwt

This comment has been minimized.

Show comment
Hide comment
@nwt

nwt May 12, 2017

Contributor

Do we need to block hub here or is hub specifically ignoring non-distributable layers?

https://github.com/nwt/docker/blob/598f1c2e12dcd6937a05ac26bd8690f3f031ca86/registry/service_v2.go#L12 should ensure that APIEndpoints.AllowNondistributableArtifacts is always false for Hub.

Contributor

nwt commented May 12, 2017

Do we need to block hub here or is hub specifically ignoring non-distributable layers?

https://github.com/nwt/docker/blob/598f1c2e12dcd6937a05ac26bd8690f3f031ca86/registry/service_v2.go#L12 should ensure that APIEndpoints.AllowNondistributableArtifacts is always false for Hub.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 12, 2017

Contributor

Design OK here.
Maybe @stevvooe could give it another quick review before we move to code review?

Contributor

cpuguy83 commented May 12, 2017

Design OK here.
Maybe @stevvooe could give it another quick review before we move to code review?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe May 12, 2017

Contributor

LGTM

@nwt "Are you sure you want to use this feature? You are going to have to do a lot of typing..." 👅

I like the name though! It describes it correctly!

Contributor

stevvooe commented May 12, 2017

LGTM

@nwt "Are you sure you want to use this feature? You are going to have to do a lot of typing..." 👅

I like the name though! It describes it correctly!

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom May 15, 2017

Member

I don't understand this, if foreign layers are meant to not be pushed to registries, why is this needed? can you explain the use case?

Member

runcom commented May 15, 2017

I don't understand this, if foreign layers are meant to not be pushed to registries, why is this needed? can you explain the use case?

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism May 15, 2017

Contributor

@runcom the use-case is running Docker in air-gapped datacenters with no access to the internet. With this change, users can move images across air-gaps and push foreign layers to private registries in these kinds of settings.

Contributor

friism commented May 15, 2017

@runcom the use-case is running Docker in air-gapped datacenters with no access to the internet. With this change, users can move images across air-gaps and push foreign layers to private registries in these kinds of settings.

@GordonTheTurtle

This comment has been minimized.

Show comment
Hide comment
@GordonTheTurtle

GordonTheTurtle May 15, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "push-foreign-layers" git@github.com:nwt/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354567184
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

GordonTheTurtle commented May 15, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "push-foreign-layers" git@github.com:nwt/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354567184
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels May 15, 2017

@cpuguy83

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 15, 2017

Contributor

Before merge can we squash all the commits to 1?
Thanks!

Contributor

cpuguy83 commented May 15, 2017

Before merge can we squash all the commits to 1?
Thanks!

@nwt

This comment has been minimized.

Show comment
Hide comment
@nwt

nwt May 15, 2017

Contributor

Squashed.

Contributor

nwt commented May 15, 2017

Squashed.

@nwt

This comment has been minimized.

Show comment
Hide comment
@nwt

nwt May 15, 2017

Contributor

I think the changes to pull_v2 need test coverage.

@aaronlehmann: integration-cli/docker_cli_pull_local_test.go looks like the natural place to add test coverage for pull_v2_windows.go, but DockerRegistrySuite doesn't run on Windows. Do you see another straightforward way to test this?

Contributor

nwt commented May 15, 2017

I think the changes to pull_v2 need test coverage.

@aaronlehmann: integration-cli/docker_cli_pull_local_test.go looks like the natural place to add test coverage for pull_v2_windows.go, but DockerRegistrySuite doesn't run on Windows. Do you see another straightforward way to test this?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 15, 2017

Contributor

@aaronlehmann: integration-cli/docker_cli_pull_local_test.go looks like the natural place to add test coverage for pull_v2_windows.go, but DockerRegistrySuite doesn't run on Windows. Do you see another straightforward way to test this?

No, I don't.

Contributor

aaronlehmann commented May 15, 2017

@aaronlehmann: integration-cli/docker_cli_pull_local_test.go looks like the natural place to add test coverage for pull_v2_windows.go, but DockerRegistrySuite doesn't run on Windows. Do you see another straightforward way to test this?

No, I don't.

return rsc, nil
}
rsc.Close()
}

This comment has been minimized.

@aaronlehmann

aaronlehmann May 16, 2017

Contributor

It might be a good idea to add debug logs for the attempt to pull the foreign layer from the registry, and a failure to do so, as is done below for the case where it's pulled from the specified URL.

@aaronlehmann

aaronlehmann May 16, 2017

Contributor

It might be a good idea to add debug logs for the attempt to pull the foreign layer from the registry, and a failure to do so, as is done below for the case where it's pulled from the specified URL.

@dmcgowan

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 16, 2017

Contributor

ping @thaJeztah @vdemeester Can you take a look at docs?

Contributor

cpuguy83 commented May 16, 2017

ping @thaJeztah @vdemeester Can you take a look at docs?

@@ -828,6 +829,27 @@ To set the DNS search domain for all Docker containers, use:
$ sudo dockerd --dns-search example.com
```
#### Allow push of nondistributable artifacts

This comment has been minimized.

@thaJeztah

thaJeztah May 16, 2017

Member

Can the option also be configured through the daemon.json configuration file?

@thaJeztah

thaJeztah May 16, 2017

Member

Can the option also be configured through the daemon.json configuration file?

This comment has been minimized.

@nwt

nwt May 16, 2017

Contributor

@thaJeztah: Yes, via "allow-nondistributable-artifacts": ["registry1", "registry2"].

@nwt

nwt May 16, 2017

Contributor

@thaJeztah: Yes, via "allow-nondistributable-artifacts": ["registry1", "registry2"].

This comment has been minimized.

@nwt

nwt May 16, 2017

Contributor

@thaJeztah: I documented this in 93ca5e6. If that looks OK, I'll squash it.

@nwt

nwt May 16, 2017

Contributor

@thaJeztah: I documented this in 93ca5e6. If that looks OK, I'll squash it.

This option is useful when pushing images containing nondistributable artifacts
to a registry on an air-gapped network so hosts on that network can pull the
images without connecting to another server.

This comment has been minimized.

@thaJeztah

thaJeztah May 16, 2017

Member

@friism can you verify if this needs additional / explicit warnings (i.e. pushing layers to a publicly accessible registry not being allowed, void the license, yadayada?) If more is needed, it should be added something like;

> **Warning**: .........
> .....
@thaJeztah

thaJeztah May 16, 2017

Member

@friism can you verify if this needs additional / explicit warnings (i.e. pushing layers to a publicly accessible registry not being allowed, void the license, yadayada?) If more is needed, it should be added something like;

> **Warning**: .........
> .....

This comment has been minimized.

@friism

friism May 16, 2017

Contributor

I think that's a good idea. Maybe something like:


Warning: Nondistributable artifacts typically have restrictions on how and where they can be distributed and shared. Only use this feature to push artifacts to private registries and ensure that you are in compliance with any terms that cover redistributing nondistributable artifacts.


it's pretty orwellian though

@friism

friism May 16, 2017

Contributor

I think that's a good idea. Maybe something like:


Warning: Nondistributable artifacts typically have restrictions on how and where they can be distributed and shared. Only use this feature to push artifacts to private registries and ensure that you are in compliance with any terms that cover redistributing nondistributable artifacts.


it's pretty orwellian though

This comment has been minimized.

@nwt

nwt May 16, 2017

Contributor

@friism, @thaJeztah: Warning added in 3a0b63c. If it looks OK, I'll squash it.

@nwt

nwt May 16, 2017

Contributor

@friism, @thaJeztah: Warning added in 3a0b63c. If it looks OK, I'll squash it.

@thaJeztah

docs LGTM, but asked @friism to have another look

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism May 16, 2017

Contributor

lgtm

Contributor

friism commented May 16, 2017

lgtm

Add daemon option to push foreign layers
The --allow-nondistributable-artifacts daemon option specifies
registries to which foreign layers should be pushed.  (By default,
foreign layers are not pushed to registries.)

Additionally, to make this option effective, foreign layers are now
pulled from the registry if possible, falling back to the URLs in the
image manifest otherwise.

This option is useful when pushing images containing foreign layers to a
registry on an air-gapped network so hosts on that network can pull the
images without connecting to another server.

Signed-off-by: Noah Treuhaft <noah.treuhaft@docker.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 16, 2017

Member

moving to "merge"

Member

thaJeztah commented May 16, 2017

moving to "merge"

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 16, 2017

Member

/cc @mstanleyjones in case we need to document this option elsewhere in the documentation

Member

thaJeztah commented May 16, 2017

/cc @mstanleyjones in case we need to document this option elsewhere in the documentation

@thaJeztah thaJeztah merged commit a30ef99 into moby:master May 17, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34213 has succeeded
Details
janky Jenkins build Docker-PRs 42813 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3198 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14049 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2917 has succeeded
Details

@thaJeztah thaJeztah referenced this pull request Jun 13, 2017

Closed

Caching foreign layers #33650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment