-
Notifications
You must be signed in to change notification settings - Fork 3
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
WIP initial version #1
Conversation
insecure no auth no s3 just local storage
## In the case of nginx performing auth, the header will be unset | ||
## since nginx is auth-ing before proxying. | ||
map $upstream_http_docker_distribution_api_version $docker_distribution_api_version { | ||
'registry/2.0' ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to set the header to:
docker-distribution-api-version: registry/2.0, \
which then confuses sdc-docker when checking against this header (sdc-docker uses this to see if the registry supports v2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. d6468fb fixed this down on line 67:
- add_header 'Docker-Distribution-Api-Version' \$docker_distribution_api_version always;
+ add_header 'Docker-Distribution-Api-Version' $docker_distribution_api_version always;
If we don't care about LE challenges, we could add a firewall rule like this to allow sdc-docker and imgapi for Triton Cloud: $ triton fwrule create -D "sdc-docker private registry access" 'FROM (ip 165.225.134.21 OR ip 165.225.134.27 OR ip 165.225.148.4 OR ip 165.225.164.15 OR ip 165.225.164.6 OR ip 165.225.172.28 OR ip 165.225.172.6 OR ip 199.192.240.203 OR ip 199.192.240.41 OR ip 37.153.102.26 OR ip 37.153.102.30 OR ip 8.19.32.41) TO tag "triton.cns.services" = docker-registry-nginx ALLOW tcp (PORT 80 AND PORT 443)' |
{ | ||
"name": "docker-registry", | ||
"port": 5000, | ||
"health": "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be, um, better...er, more effective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even just a GET /v2/
will hit the API Version Check so that we know the registry is listening.
# Optional Docker user info | ||
# This supports a single user/password pair. | ||
# More complex use-cases can be built using this as a base image. | ||
# These generated strings might be a good user/password combo for your registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the comments clarify this a bit for end-user uses? Something more like "doing X and Y is how you would have your Docker client login"? And/or include links to what piece of the registry auth we're implementing here?
[![DockerPulls](https://img.shields.io/docker/pulls/autopilotpattern/docker-registry.svg)](https://registry.hub.docker.com/u/autopilotpattern/docker-registry/) | ||
[![DockerStars](https://img.shields.io/docker/stars/autopilotpattern/docker-registry.svg)](https://registry.hub.docker.com/u/autopilotpattern/docker-registry/) | ||
|
||
### Hello world example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the README is missing a description of the components and how they interact; stating plainly that a Nginx container is proxying to the Registry container, whether connectivity between the Nginx container and Registry is encrypted, etc.
### Hello world example | ||
|
||
1. [Get a Joyent account](https://my.joyent.com/landing/signup/) and [add your SSH key](https://docs.joyent.com/public-cloud/getting-started). | ||
1. Install the [Docker Toolbox](https://docs.docker.com/installation/mac/) (including `docker` and `docker-compose`) on your laptop or other environment, as well as the [Joyent Triton CLI](https://docs.joyent.com/public-cloud/api-access/cloudapi). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Install Docker (includes docker-compose
and docker-machine
)
S3BUCKET=<your s3 bucket> | ||
``` | ||
|
||
### "It doesn't do X" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ### Contributing
? This feels a little "PRs welcome nyah nyah" to me
"timeout": "10m" | ||
}{{ end }} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've got a missing line-ending/EOF on a few files. I'd recommend fixing this in your editor settings b/c it can cause issues when trying to text-manipulate source files.
until | ||
cmd=$1 | ||
if [ -z "$cmd" ]; then | ||
onChange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no onChange
handler in this file. Given there's a single command here maybe we should just cut all the command parsing? Or even just put the consul-template
call into the containerpilot.json?
registry: | ||
image: autopilotpattern/docker-registry:latest | ||
restart: always | ||
mem_limit: 128m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we tested this with larger image layers? I'm worried large image layers don't end up getting streamed in-memory without giant chunks because, well, I don't trust upstream not to foul that up. 😀
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
proxy_set_header X-Forwarded-Proto $scheme; | ||
proxy_read_timeout 900; | ||
}{{ end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking I like adding an {{ else }}
block here to return a 503. And in this case it tells the Docker client "hey no backends are available and shit's broken" instead of returning 404 which tells the client that we simply don't implement v2 of the spec
mem_limit: 128m | ||
env_file: _env | ||
expose: | ||
- 53 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to expose many of these ports if we don't recommend using them.
Given that one needs to use DNS to talk to the registry anyways (both for TLS and the namespacing), should this be replaced by the work in https://github.com/joyent/product-automation/tree/master/registry? |
Closing in lieu of #7 |
filesystem
ors3
Support for traditional SSL certs, in addition to Let's Encrypt(depends on Patterns for injecting SSL certificate and key nginx#38, cutting from MVP)Here's an example of what's expected in the
_env
file:Here's an example of the S3 config, if that's used instead of
STORAGEDRIVER=filesystem
: