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

Vault test setup tweaks #1322

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Vault test setup tweaks #1322

merged 5 commits into from
Aug 27, 2024

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 23, 2024

Description

Note: This PR builds on top of #1321.
This PR removes the need for all vault config related test files, including local/vault/approle.json and others.

The reason these files existed was to share the vault approle ID and secret (created when Vault was initialised) with the application. I found that it is possible to set a custom approle ID and secret so we can simply hardcode the values in tests and in the docker compose dev environment.

Another benefit of this is improved test caching. Previously tests involving approle.json were not able to be cached across Github action runs because the file contents changed when each new Vault instance was started.

@kian99 kian99 requested a review from a team as a code owner August 23, 2024 07:38
alesstimec
alesstimec previously approved these changes Aug 23, 2024
Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

this is really neat.. much better than the previous setup


# Add our policy and entrypoint
COPY policy.hcl /vault/policy.hcl
COPY entrypoint.sh /vault/entrypoint.sh
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Maybe less whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pretty much copied from the Vault sample repo. I think it looks kind of nice with the whitespace.

touch /tmp/healthy

# Keep container alive
tail -f /dev/null & trap 'kill %1' TERM ; wait
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a line break after the &?

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 copied this line from the vault sample application and it works well.
I understand the basics that it's running some process, then adding a trap to handle graceful cancellation of the container and then running wait to wait for the server job. But I'm not sure why exactly we need tail -f /dev/null &.

I did some googling to see if I could understand well why it's like this.
This SO answer has some explanation on tail -f /dev/null but I'll play around with it and see if just
trap 'kill %1' TERM ; wait doesn't do the job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Played around to understand it better,

  • tail -f /dev/null & creates a process in the background that won't return until the file descriptor it is reading from is closed.
  • The trap 'kill %1' TERM will send a signal to job 1 in the jobs list when a TERM signal is received. In this case %1 or job 1 would actually be the Vault server and tail would be job 2.
  • wait then waits for all background jobs to complete.

The line tail -f /dev/null & trap 'kill %1' TERM ; wait is a way to keep the container alive when no long running process is occurring. In this we have Vault running so I suspected the tail portion of the command wasn't necessary and removing it works fine. I've done that in the follow-up PR to this #1323

One thing that's a little weird is that wait without any arguments is supposed to wait for all background jobs to complete. Currently, I would expect the tail process to still be running because trap 'kill %1' TERM is stopping the Vault server. Possibly tail is also stopping for some unknown reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh and maybe the reason there is no line break after the & is key here? I didn't look into that much.

@kian99 kian99 requested a review from alesstimec August 27, 2024 06:43
@kian99 kian99 merged commit 5651863 into canonical:v3 Aug 27, 2024
4 checks passed
kian99 added a commit to kian99/jimm that referenced this pull request Sep 3, 2024
* remove need for approle.json

* remove references to vault env and approle files

* remove sql init volume

* Add new lines
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.

3 participants