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

Use /cert less images, newer hook and some more docker-compose clean ups. #141

Merged
merged 12 commits into from
Jun 23, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Jun 22, 2022

Description

  • Update nixpkgs to get a newer docker-compose
  • Echo a message when setup.sh is all done
  • Change tink-server healthcheck endpoint to healthz instead of fetching /cert
  • Ensure command line args are always double quoted
  • Get rid of conflicting boots listen address values
  • Update boots/hegel/tink* container images to latest sha
  • Drop useless env vars (both packet/em specific and outdated ones)
  • Keep versions in variables for DRYness
  • Move all configurable env vars to .env (also for DRYness)
  • Specify the same version of tink-worker as tink-serve and tink-cli to hook
  • Update hook to v0.7.0

Why is this needed

Two main things going on in this PR. First is cleaning up both the docker-compose.yml and .env files so that the docker-compose file can be written as if all the envvars it wants are always specified. Actual environment variables still supersede the values in .env like they also supersede when ${NAME:-default} is specified in the docker-compose.yml file. All in all this means we only need to specify the value of an env var with default once instead of all over the docker-compose.yml file.

The other thing going on in this PR is updates to all the container images and the version of hook being used. The container images have been updated to a relatively recent version that no longer serves or fetches the grpc tls cert via the /cert http url endpoint. The version of Hook has been updated too since the previously used one is almost a year old and thus we haven't been keeping up with hook commit activity. All the versions have been properly pinned down (including tink-worker being kept in sync with tink-server and tink-cli).

How Has This Been Tested?

Both vagrant up and terraform apply have been run and work.

mmlb added 8 commits June 2, 2022 16:50
Without this docker-compose complains about depends_on config settings:

> ERROR: The Compose file './docker-compose.yml' is invalid because:
> services.tink-server.depends_on.generate-tls-certs.condition contains "service_completed_successfully", which is an invalid type, it should be a service_started, or a service_healthy
> services.web-assets-server.depends_on.fetch-and-convert-ubuntu-img.condition contains "service_completed_successfully", which is an invalid type, it should be a service_started, or a service_healthy
> services.registry.depends_on.generate-tls-certs.condition contains "service_completed_successfully", which is an invalid type, it should be a service_started, or a service_healthy

This was due to docker/compose#8154.

Signed-off-by: Manuel Mendez <[email protected]>
It is hard to tell when setup.sh is done in vagrant without having to
read/parse the output to figure out if we're at the last step. This change
makes it obvious. Its not so necessary for terraform but I'd like to keep
both scripts about as similar as possible.

Signed-off-by: Manuel Mendez <[email protected]>
cert is going away and was never really a good one anyway.

Signed-off-by: Manuel Mendez <[email protected]>
Don't need quotes for strings in yaml dicts (512M is a string due to the M)
and we definitely want them whenever they appear in a shell command.

Signed-off-by: Manuel Mendez <[email protected]>
To avoid confusion between the environment and cli params.

Signed-off-by: Manuel Mendez <[email protected]>
Added TINKERBELL_TLS (default false) since self-signed certs are going to
cause tink-worker to fail to connect.

Signed-off-by: Manuel Mendez <[email protected]>
These are no longer needed (some never were) so we lets drop them.

Signed-off-by: Manuel Mendez <[email protected]>
This way we can be sure that all the versions are kept in sync easily.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb force-pushed the drop-cert branch 2 times, most recently from 9806515 to b8f5b3b Compare June 22, 2022 17:00
@mmlb mmlb marked this pull request as ready for review June 22, 2022 20:29
Copy link
Member

@jacobweinstock jacobweinstock left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. One small comment.

deploy/compose/docker-compose.yml Show resolved Hide resolved
Lets make .env the central/source-of-truth for env configuration. This
means we don't have to deal with empty vars and default values all over the
docker-compose.yml file. This does not cause any change in treatment of
environment variables. Actual environment variables still supersede those set
in .env. The values in .env are thus just the default if left unspecified,
exactly like ${VAR:-default} in the docker-compose.yml file.

I also got rid of container configs that used the `environment` config to
inject the env vars so that the `command` can use it. Having the values
in the environment would only really be useful if we expect to `exec`
into the container and run some commands. I haven't needed to do that yet
so would rather avoid repetition until absoulutely necessary. We can just
have docker-compose inject the values directly into the `command` line.

Signed-off-by: Manuel Mendez <[email protected]>
mmlb added 3 commits June 22, 2022 17:47
${} isn't actually needed in any of these uses so lets just use the shorter form.

Signed-off-by: Manuel Mendez <[email protected]>
…nk-cli to hook

This way all the tink images are in sync and managed in just one place
(the .env file).

Signed-off-by: Manuel Mendez <[email protected]>
This has a bunch of linuxkit updates and more recent container images that have
dropped /cert support.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb requested a review from jacobweinstock June 22, 2022 21:58
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Jun 22, 2022
@mmlb mmlb removed the request for review from displague June 23, 2022 14:36
@mergify mergify bot merged commit 8eec029 into tinkerbell:main Jun 23, 2022
@mmlb mmlb deleted the drop-cert branch June 23, 2022 14:38
ttwd80 pushed a commit to ttwd80/tinkerbell-playground that referenced this pull request Sep 7, 2024
…ups. (tinkerbell#141)

## Description

- Update nixpkgs to get a newer docker-compose
- Echo a message when setup.sh is all done
- Change tink-server healthcheck endpoint to healthz instead of fetching /cert
- Ensure command line args are always double quoted
- Get rid of conflicting boots listen address values
- Update boots/hegel/tink* container images to latest sha
- Drop useless env vars (both packet/em specific and outdated ones)
- Keep versions in variables for DRYness
- Move all configurable env vars to .env (also for DRYness)
- Specify the same version of tink-worker as tink-serve and tink-cli to hook
- Update hook to v0.7.0

## Why is this needed

Two main things going on in this PR. First is cleaning up both the docker-compose.yml and .env files so that the docker-compose file can be written as if all the envvars it wants are always specified. Actual environment variables still supersede the values in .env like they also supersede when `${NAME:-default}` is specified in the docker-compose.yml file. All in all this means we only need to specify the value of an env var with default once instead of all over the docker-compose.yml file.

The other thing going on in this PR is updates to all the container images and the version of hook being used. The container images have been updated to a relatively recent version that no longer serves or fetches the grpc tls cert via the /cert http url endpoint. The version of Hook has been updated too since the previously used one is almost a year old and thus we haven't been keeping up with hook commit activity. All the versions have been properly pinned down (including tink-worker being kept in sync with tink-server and tink-cli).

## How Has This Been Tested?

Both `vagrant up` and `terraform apply` have been run and work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants