Docker refactoring #3018

Merged
konrad merged 6 commits from vlasov-y/frontend:main into main 2023-01-29 14:47:24 +00:00
Member

Hi!

Your product looks awesome and I have decided to spawn an instance for myself. I have written a Helm chart and tried to deploy your application.
I has failed because of wrong Dockerfile architecture. You run Nginx as non-root user, but do not override cache paths so that it fails to make chown for /var/cache/nginx/*.

First, I have started building crutches using init containers, but then thought that maybe I can participate in open-source and share my experience. I have refactored your frontend. Also I plan to review your backend as well. Then I will add flexible Helm charts for both, if you do not mind :)

Changelog

  1. Splitted pnpm install and build to speed up the build in total
  2. Added optional JSON logging format that can be set using VIKUNJA_LOG_FORMAT env var during the runtime. (check other env variables in the Dockerfile)
  3. Refactored your run.sh and now it is scripts/docker/injector.sh
  4. Carefully copied your custom Nginx configuration in my set, so I should break nothing
  5. Used native Nginx docker-entrypoint.d to run the injector
  6. Switched the whole container to non-root in Dockerfile directly
  7. Built and launched the frontend locally - works
  8. Added Docker Buildx comment in the very beginning of Dockerfile
  9. Added Docker build documentation notes to README.md
  10. Changed command to generate src/version.json (without changing version format, of course) using jq to generate a valid JSON
Hi! ✋ Your product looks *awesome* and I have decided to spawn an instance for myself. I have written a Helm chart and tried to deploy your application. I has failed because of wrong Dockerfile architecture. You run Nginx as non-root user, but do not override cache paths so that it fails to make chown for /var/cache/nginx/\*. First, I have started building crutches using init containers, but then thought that maybe I can participate in open-source and share my experience. I have refactored your frontend. Also I plan to review your **backend** as well. Then I will add flexible **Helm charts** for both, if you do not mind :) ### Changelog 1. Splitted pnpm install and build to speed up the build in total 2. Added optional JSON logging format that can be set using `VIKUNJA_LOG_FORMAT` env var during the runtime. (*check other env variables in the Dockerfile*) 3. Refactored your run.sh and now it is scripts/docker/injector.sh 4. Carefully copied your custom Nginx configuration in my set, so I should break nothing 5. Used native Nginx docker-entrypoint.d to run the injector 6. Switched the whole container to non-root in Dockerfile directly 7. Built and launched the frontend locally - works 8. Added Docker Buildx comment in the very beginning of Dockerfile 9. Added Docker build documentation notes to README.md 10. Changed command to generate src/version.json (without changing version format, of course) using jq to generate a valid JSON
vlasov-y added 1 commit 2023-01-27 09:06:04 +00:00
continuous-integration/drone/pr Build is passing Details
11799a6829
Docker refactoring
vlasov-y added spent time 2023-01-27 09:09:55 +00:00
2 hours
Member

Hi vlasov-y!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3018-main--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi vlasov-y! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3018-main--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad requested changes 2023-01-27 11:39:20 +00:00
konrad left a comment
Owner

Nice work!

The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker

Nice work! The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker
Dockerfile Outdated
@ -46,2 +51,2 @@
# installs usermod and groupmod
shadow
ENV VIKUNJA_HTTP_PORT 8080
ENV VIKUNJA_HTTP2_PORT 8081

Do we even need to let users change these ports? Isn't it enough to choose a different "outer" port when running and publishing the container?

Do we even need to let users change these ports? Isn't it enough to choose a different "outer" port when running and publishing the container?
Author
Member

Depends... Maybe someone will want to run the container in host network. If you want, I can just hardcode to 8080 and 8081.

Depends... Maybe someone will want to run the container in host network. If you want, I can just hardcode to 8080 and 8081.
Dockerfile Outdated
@ -49,1 +58,3 @@
CMD "/run.sh"
COPY scripts/docker/injector.sh /docker-entrypoint.d/50-injector.sh
COPY scripts/docker/nginx.conf /etc/nginx/nginx.conf
COPY scripts/docker/templates/. /etc/nginx/templates/

Can we move this from scripts/docker to just docker? Since this now also contains config file I think it's more than just scripts.

Can we move this from `scripts/docker` to just `docker`? Since this now also contains config file I think it's more than just scripts.
Author
Member

Sure! Usually, I create folder .devops. I will move files to folder named docker, as you asked.

Sure! Usually, I create folder `.devops`. I will move files to folder named *docker*, as you asked.
Owner

I built this container locally but it looks like it does not work:

$ curl localhost:8888
curl: (56) Recv failure: Connection reset by peer

(running as docker run -p 8888:80 vikunja/frontend:dev)

I built this container locally but it looks like it does not work: ``` $ curl localhost:8888 curl: (56) Recv failure: Connection reset by peer ``` (running as `docker run -p 8888:80 vikunja/frontend:dev`)
Author
Member

I built this container locally but it looks like it does not work:

$ curl localhost:8888
curl: (56) Recv failure: Connection reset by peer

(running as docker run -p 8888:80 vikunja/frontend:dev)

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

docker run --rm -d --name vikunja-test -p 8888:8080 vikunja/frontend:dev
curl -v http://localhost:8888
> I built this container locally but it looks like it does not work: > > ``` > $ curl localhost:8888 > curl: (56) Recv failure: Connection reset by peer > ``` > > (running as `docker run -p 8888:80 vikunja/frontend:dev`) @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. ```shell docker run --rm -d --name vikunja-test -p 8888:8080 vikunja/frontend:dev curl -v http://localhost:8888 ```
vlasov-y added 1 commit 2023-01-27 12:26:54 +00:00
continuous-integration/drone/pr Build is passing Details
827dff09c5
Moved docker files out of scripts folder
vlasov-y requested review from konrad 2023-01-27 12:26:57 +00:00
Owner

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

> @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.
Owner

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

> > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.
konrad reviewed 2023-01-27 14:51:17 +00:00
@ -0,0 +19,4 @@
server_tokens off;
types_hash_max_size 2048;
types_hash_bucket_size 64;
client_max_body_size 500M;

This seems unnecessary as the frontend does not handle any file uploads. I don't know if it would hurt to leave it in there, though.

This seems unnecessary as the frontend does not handle any file uploads. I don't know if it would hurt to leave it in there, though.
Author
Member

It is just a part of template I use everywhere. I agree, this should be removed.

It is just a part of template I use everywhere. I agree, this should be removed.
Author
Member

fixed

fixed
konrad marked this conversation as resolved
@ -0,0 +95,4 @@
image/svg+xml
image/x-icon
audio/wav;
gzip_disable "MSIE [1-6]\.";

I think we can omit this, we don't support IE at all

I think we can omit this, we don't support IE at all
Author
Member

Okay

Okay
Author
Member

fixed

fixed
konrad marked this conversation as resolved
konrad reviewed 2023-01-27 14:54:10 +00:00
@ -0,0 +49,4 @@
'"request_method": "$request_method",'
'"request_time": "$request_time",'
'"server_addr": "$server_addr",'
'"server_name": "$server_name",'

I think this can be removed since the server name is always _.

I think this can be removed since the server name is always `_`.
Author
Member

Okay

Okay
Author
Member

fixed

fixed
konrad marked this conversation as resolved
@ -0,0 +58,4 @@
'"upstream_connect_time": "$upstream_connect_time",'
'"upstream_header_time": "$upstream_header_time",'
'"upstream_response_length": "$upstream_response_length",'
'"upstream_response_time": "$upstream_response_time",'

Same for the upstream log messages since they are only used when nginx is used as a proxy?

Same for the upstream log messages since they are only used when nginx is used as a proxy?
Author
Member

Yeah, it is also a part of my template :) Will remove as well

Yeah, it is also a part of my template :) Will remove as well
Author
Member

fixed

fixed
konrad marked this conversation as resolved
Author
Member

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

Okay. It works, but I am confused. Nginx works as non-root o_O

image

> > > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. > > Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup. Okay. It works, but I am confused. Nginx works as non-root o_O ![image](/attachments/cc3c350f-7b98-479f-9024-ea5e87808b0b)
vlasov-y added 1 commit 2023-01-27 15:58:31 +00:00
continuous-integration/drone/pr Build is passing Details
d3b0876054
Removed unnecessary Nginx configuration
vlasov-y added 1 commit 2023-01-27 15:59:53 +00:00
continuous-integration/drone/pr Build is failing Details
fd1844c924
Changed Docker port to 80 by default
Author
Member

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

done

> > > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. > > Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup. done
vlasov-y added 1 commit 2023-01-27 16:16:19 +00:00
continuous-integration/drone/pr Build is passing Details
3cd0bedd1b
Fixed http2 port
vlasov-y added 1 commit 2023-01-27 16:18:24 +00:00
continuous-integration/drone/pr Build is passing Details
5cb678263f
Switched to nginx user
konrad approved these changes 2023-01-29 14:47:02 +00:00
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit e4499f44b7 into main 2023-01-29 14:47:24 +00:00
konrad referenced this issue from a commit 2023-01-29 14:47:24 +00:00
Owner

Okay. It works, but I am confused. Nginx works as non-root o_O

I'm not sure how they do it, but this stack exchange answer says it is possible. The nginx container probably did this.

> Okay. It works, but I am confused. Nginx works as non-root o_O I'm not sure how they do it, but [this stack exchange answer says it is possible](https://superuser.com/a/892391). The nginx container probably did this.
Author
Member

Nice work!

The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker

Could you please point me to the documentation repository? I am not quite sure where is it.

> Nice work! > > The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker Could you please point me to the documentation repository? I am not quite sure where is it.
Owner

The docs are here: https://kolaente.dev/vikunja/api/src/branch/main/docs

I've already adjusted the docs about the puid and gid of the Frontend container.

The docs are here: https://kolaente.dev/vikunja/api/src/branch/main/docs I've already adjusted the docs about the puid and gid of the Frontend container.
viehlieb referenced this issue from a commit 2023-02-01 16:07:50 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.