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

Initial draft #2

Merged
merged 6 commits into from
Mar 9, 2022
Merged

Initial draft #2

merged 6 commits into from
Mar 9, 2022

Conversation

TDDR
Copy link
Contributor

@TDDR TDDR commented Feb 27, 2022

This is my initial draft of how I think the docker-compose, registry-config, and the nginx.config should look like.

I know there is still a lot of work to be done but I figured I would get something up so we can start collaborating.

I included the links for the templates I used to make each of the files at the top of each file. I made altercations to both the docker-compose and the registry-config. The nginx.conf however was a straight cut and paste from the linked website.

@TDDR TDDR closed this Feb 27, 2022
@TDDR TDDR reopened this Feb 27, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started. I've left some initial thoughts.

config/nginx.conf Outdated Show resolved Hide resolved

# SSL
ssl_certificate /etc/nginx/conf.d/domain.crt;
ssl_certificate_key /etc/nginx/conf.d/domain.key;
Copy link
Contributor

Choose a reason for hiding this comment

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

@manekenpix has certs, he'll know what he wants done here.


# To add basic authentication to v2 use auth_basic setting.
auth_basic "Registry realm";
auth_basic_user_file /etc/nginx/conf.d/nginx.htpasswd;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine to start, until we put in a better GitHub based auth solution.

We'll need to generate nginx.htpasswd with some users to test.

config/registry-config.yaml Outdated Show resolved Hide resolved
service: registry
storage:
cache:
blobdescriptor: inmemory # or redis?
Copy link
Contributor

Choose a reason for hiding this comment

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

Memory is fine for now.

http:
addr: 443:5000 # Why the 443 -> https://docs.docker.com/registry/configuration/#letsencrypt
host: docker.cdot.systems
secret: sUperDuperSecret
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't do this. We'll need an option for how we add this (env?)

docker-compose.yaml Outdated Show resolved Hide resolved
docker-compose.yaml Outdated Show resolved Hide resolved
REGISTRY_AUTH_TOKEN_ISSUER: Registry token issuer
REGISTRY_AUTH_TOKEN_ROOTCERTBUNDLE: /root/certs/bundle
volumes:
- /telescope/data:/var/lib/registry
Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out where in the host we're going to put our stuff.

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 /usr/share/our_directory would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say our_directory, would that mean this directory, /mnt/docker0storage/registry?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, so it would be:

volumes:
  - /mnt/docker0storage/registry:/var/lib/registry

Such that the container will overlay /mnt/docker0storage/registry onto /var/lib/registry within the container.

docker-compose.yaml Show resolved Hide resolved
@TDDR TDDR requested a review from humphd February 27, 2022 22:21
@TDDR TDDR force-pushed the initial-docker-compose branch from d76c9e8 to 9cb3dbc Compare February 27, 2022 22:27
@TDDR TDDR force-pushed the initial-docker-compose branch from 9cb3dbc to 32605b0 Compare February 27, 2022 22:28
#https://docs.docker.com/registry/deploying/#deploy-your-registry-using-a-compose-file
services:
nginx:
image: "nginx:alpine"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use nginx:stable-alpine. Boring and safe vs cutting edge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to use bcrypt?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

nginx:stable-alpine is still alpine, just a stable vs. development version of nginx

docker-compose.yaml Show resolved Hide resolved
ports:
- 5043:443
restart: unless-stopped
links:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this, since our registry is named registry already, and will be reachable by nginx.

links:
- registry:registry
volumes:
- ./auth:/etc/nginx/conf.d
Copy link
Contributor

Choose a reason for hiding this comment

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

@manekenpix where should we put our files in the host? It might make sense to have ./nginx but I don't think auth makes as much sense for these.

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 a config folder would be ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had thought the same thing originally, but after going back and looking at the documentation I saw this.
image
from here https://docs.docker.com/registry/recipes/nginx/#setting-things-up

So I decided to go with the current folder structure. I can create and move it back to a config folder though?

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 the easiest way to do this is to mimic the layout of what we'll pass in within our repo:

docker-compose.yml
config/ (or this could be called volume/)
  etc/
    nginx/
      conf.d

This way you can look at the overlays we have in our repo, and understand quickly where they end-up in the container.

volumes:
  - ./config/etc/nginx/conf.d:/etc/nginx/conf.d

image: certbot/certbot
container_name: 'certbot'
volumes:
- ../../certbot/conf:/etc/letsencrypt
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here, we need to determine our local path

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe somewhere in /etc, like /etc/letsencrypt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, config/etc/letsencrypt would work similar to my comment above.

docker-compose.yaml Show resolved Hide resolved
auth/nginx.conf Outdated

server {
listen 443 ssl;
server_name docker.cdot.systems;
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 should use a variable here, in case we decide to move it somewhere else.

Suggested change
server_name docker.cdot.systems;
server_name ${DOMAIN};

auth/nginx.conf Outdated
# required to avoid HTTP 411: see Issue #1486 (https://github.com/moby/moby/issues/1486)
chunked_transfer_encoding on;

location /v2/ {
Copy link
Contributor

@Kevan-Y Kevan-Y Feb 28, 2022

Choose a reason for hiding this comment

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

Just to make sure I understand. We will push our image to DOMAIN/v2 route?

rootdirectory: /mnt/docker0storage/registry
http:
addr: 443:5000 # https://docs.docker.com/registry/configuration/#letsencrypt
host: docker.cdot.systems
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change it to ${DOMAIN} like what @manekenpix suggested in one of the other file?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do, we'll have to use envsubst or something to replace the values before we use this file.

auth/nginx.conf Outdated
## See the map directive above where this variable is defined.
add_header 'Docker-Distribution-Api-Version' $docker_distribution_api_version always;

proxy_pass http://docker-registry;

Choose a reason for hiding this comment

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

Should this be docker.cdot.systems?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be http://registry:${PORT}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we probably don't need the upstream and can use the docker network.

config/etc/nginx/conf.d Outdated Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
@TDDR TDDR force-pushed the initial-docker-compose branch from 8fd80bf to f25c5ef Compare March 1, 2022 15:19
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

We should move toward testing this out.

Because you're using variables in the nginx conf, see the section "Using environment variables in nginx configuration (new in 1.19)" in https://hub.docker.com/_/nginx on how to pass these values in and have the container swap them for you (it won't happen automatically).


# SSL
ssl_certificate /etc/nginx/conf.d/domain.crt;
ssl_certificate_key /etc/nginx/conf.d/domain.key;
Copy link
Contributor

Choose a reason for hiding this comment

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

@manekenpix where will certbot put these?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're setting /etc/letsencrypt below, but I don't know where under there it puts things.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be like what we do for telescope's front-end, here.
I think we'll have to change the docker file to match this.

return 404;
}

# To add basic authentication to v2 use auth_basic setting.
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: for testing on the box, we could remove this completely. And when we do token-based/oauth, we won't need it.

certificate: /path/to/x509/public
key: /path/to/x509/private
letsencrypt:
cachefile: /path/to/cache-file
Copy link
Contributor

Choose a reason for hiding this comment

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

This section seems wrong. Do we need to do any SSL stuff in here, since nginx is handling it? I doubt it.

@@ -0,0 +1,41 @@
# https://docs.docker.com/registry/configuration/
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this file used? it seems we're not passing its path as argument anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the answer to this question? Can we remove it? If it's going to be used it would be in the docker-compose file below, but you have it commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unsure of the volume naming convention for it, and it would seem that the default configuration is adequate for what we have so far. I'm not sure this config file is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing we really need from this is to configure auth. Before we can use this for real, we need to have auth. In a follow-up we need:

Copy link
Contributor

Choose a reason for hiding this comment

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

@TDDR delete this file until we need it.

@TDDR
Copy link
Contributor Author

TDDR commented Mar 3, 2022

After meeting with @manekenpix tonight this should be a testable version of the initial draft. I say should be because I ran out of time to test it out in the box tonight.

Copy link
Contributor

@humphd humphd left a 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 yet? I'd love to go into next week with this (mostly) ready to land.


server {
listen 443 ssl;
server_name docker.cdot.systems #${DOMAIN};
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the comment, and just hard-code this value. It's not going to change often enough to bother with the extra machinery to rotate it based on env vars.

@@ -0,0 +1,41 @@
# https://docs.docker.com/registry/configuration/
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the answer to this question? Can we remove it? If it's going to be used it would be in the docker-compose file below, but you have it commented out.

docker-compose.yaml Show resolved Hide resolved
@TDDR TDDR force-pushed the initial-docker-compose branch from 5319817 to 8abbeb5 Compare March 5, 2022 01:03
@TDDR
Copy link
Contributor Author

TDDR commented Mar 5, 2022

@humphd looks like I was testing it out as you where reviewing. The version that is in here now has been tested and it works
image

image

I can either work on resolving the final few comments possibly Saturday or Sunday. Or, maybe we could land what's here and make some issues of the remaining comments?

@TDDR TDDR requested review from humphd and DukeManh March 5, 2022 01:28
@TDDR TDDR requested review from manekenpix and Kevan-Y March 5, 2022 01:28

server {
listen 443 ssl;
server_name docker.cdot.systems; #${DOMAIN};
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the comment at the end of the line.

@@ -0,0 +1,41 @@
# https://docs.docker.com/registry/configuration/
Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing we really need from this is to configure auth. Before we can use this for real, we need to have auth. In a follow-up we need:

docker-compose.yaml Show resolved Hide resolved
image: registry:2
volumes:
- /mnt/docker0storage/registry:/var/lib/registry
#- ./config/registry-config.yaml:/config/registry-config.yaml Untested use of the registry-config.yaml file
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this if not being used instead of commenting out.

@TDDR TDDR requested a review from humphd March 8, 2022 01:21
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

One change and this is good to go!

@@ -0,0 +1,41 @@
# https://docs.docker.com/registry/configuration/
Copy link
Contributor

Choose a reason for hiding this comment

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

@TDDR delete this file until we need it.

@TDDR TDDR force-pushed the initial-docker-compose branch from 2ba7b21 to 8231325 Compare March 8, 2022 16:20
@TDDR TDDR requested a review from humphd March 8, 2022 16:23
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Great. File the follow-up issues to do the rest, and this is good to land from my perspective.

@humphd
Copy link
Contributor

humphd commented Mar 9, 2022

@TDDR let's merge this, first of many!

@TDDR TDDR merged commit b936d5f into DevelopingSpace:main Mar 9, 2022
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.

5 participants