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

skopeo: 0.1.18 -> 0.1.22 #26935

Merged
merged 3 commits into from Jun 29, 2017
Merged

skopeo: 0.1.18 -> 0.1.22 #26935

merged 3 commits into from Jun 29, 2017

Conversation

calbrecht
Copy link
Member

@calbrecht calbrecht commented Jun 28, 2017

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@NeQuissimus
Copy link
Member

NeQuissimus commented Jun 28, 2017

With this new version, I have to sudo the command, which I never have to do with 0.1.18, is that intended?

λ skopeo -version
skopeo version 0.1.18

λ skopeo inspect docker://ubuntu:xenial # Works

λ ./result-bin/bin/skopeo -version
skopeo version 0.1.22

λ ./result-bin/bin/skopeo inspect docker://ubuntu:xenial                   
FATA[0000] open /etc/docker/certs.d/docker.io: permission denied

λ sudo ./result-bin/bin/skopeo inspect docker://ubuntu:xenial # Works

@calbrecht
Copy link
Member Author

calbrecht commented Jun 28, 2017

@NeQuissimus you are right.
There has to be a /etc/docker/certs.d directory that has to be readable by group docker.

because of the dependency https://github.com/projectatomic/skopeo/blob/master/vendor/github.com/containers/image/docker/docker_client.go that relies on this directory since containers/image@3aeae04

@calbrecht
Copy link
Member Author

Turns out the directory /etc/docker is created at the first start of docker daemon.
But the directory /etc/docker/certs.d is not.
It seems to be a well-known docker related directory https://docs.docker.com/engine/security/certificates/.

Copy link
Contributor

@0xABAB 0xABAB left a comment

Choose a reason for hiding this comment

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

LGTM modulo spacing.

@NeQuissimus
Copy link
Member

Oooh, I see... So normally, we patch nix packages to not rely on these folders. Could we do that here, please?

@calbrecht
Copy link
Member Author

@NeQuissimus i decided not to be that destructive and instead silently ignore the error caused by missing permissions.
The original code does ignore the cert dir if it is missing so i added the same logic for the permission error.

In case someone cares about maintaining its certs.d folder everything works out as expected.

the dependency docker_client.go tries to read /etc/docker/certs.d/
and fails if this is not readable.

the original code silently ignores this directory if it is not present
so path.patch adds the logic to ignore it in case of a permission error.
@NeQuissimus
Copy link
Member

@calbrecht Right, it's just that the behaviour changed between versions, that's why I preferred it being fixed.

@NeQuissimus NeQuissimus merged commit 32a351a into NixOS:master Jun 29, 2017
@calbrecht calbrecht deleted the upstream-skopeo branch July 1, 2017 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants