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

Add an entrypoint script #89

Merged
merged 3 commits into from
Nov 25, 2021
Merged

Add an entrypoint script #89

merged 3 commits into from
Nov 25, 2021

Conversation

kaysond
Copy link
Contributor

@kaysond kaysond commented Nov 25, 2021

The script allows secrets to be set via env var defined filenames, and preserves the ability to pass different lldap subcommands/args via the docker command.

Documentation was updated to note the changes.

I took the liberty of pinning the upstream builder and base containers to alpine 3.14 since I noticed alpine:latest points to 3.15 while rust:alpine uses 3.14. I doubt its causing any issues, but it seems smart to control which versions are used rather than just pointing to latest. Let me know if you have a reason to keep it on latest and I can remove that change.

Addresses #84

@kaysond
Copy link
Contributor Author

kaysond commented Nov 25, 2021

I did some preliminary testing - the build works fine, and the entrypoint loads the variables appropriately.
If the PR looks good, I can do a little more detailed testing to make sure the configuration takes. Should be as simple as turning on verbosity iirc

Copy link
Member

@nitnelave nitnelave 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 the PR! Just a small thing, we don't have to install bash to keep the container small, but otherwise it looks good!

Dockerfile Outdated Show resolved Hide resolved
docker-entrypoint.sh Show resolved Hide resolved
docker-entrypoint.sh Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #89 (c0de017) into main (df889ee) will not change coverage.
The diff coverage is n/a.

❗ Current head c0de017 differs from pull request most recent head ac199c2. Consider uploading reports for the commit ac199c2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #89   +/-   ##
=======================================
  Coverage   74.79%   74.79%           
=======================================
  Files          13       13           
  Lines        3170     3170           
=======================================
  Hits         2371     2371           
  Misses        799      799           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df889ee...ac199c2. Read the comment docs.

@kaysond
Copy link
Contributor Author

kaysond commented Nov 25, 2021

I think bash is a much better choice for a startup script (or any script really). It supports proper variable indirection, so you don't have to use eval (it does pose a security risk, even if minimal). Also if the entrypoint script ever gets expanded, I think its a lot easier to manage in bash than sh.

I do appreciate the desire to keep the container small. I checked and adding bash only increases container size by 6.6%. No one will bat an eye at an extra 2.1MB.
image

Of course its up to you and I'm happy to switch it to sh if you want to save the space. I would probably avoid using eval, though and just hard code the secret variable names since there aren't many. I used the indirection because its safe and elegant in bash and figured it makes it slightly easier if more secrets are ever added.

@nitnelave
Copy link
Member

I mean, if they hack their container by setting a weird environment variable, we can't protect them against themselves :D
But point taken for simplicity. Can you just merge the RUN that installs bash with the previous one, to save one docker layer?

Thanks,

@nitnelave
Copy link
Member

Oh, and please rebase against main as well.

@kaysond
Copy link
Contributor Author

kaysond commented Nov 25, 2021

should be good to go now

@nitnelave
Copy link
Member

Final nit: can you add some docs to the lldap_config template? Most people will only look at that when configuring the service

@kaysond
Copy link
Contributor Author

kaysond commented Nov 25, 2021

I mean, if they hack their container by setting a weird environment variable, we can't protect them against themselves :D

trust me, if its a possibility, someone will do something stupid XD

@kaysond
Copy link
Contributor Author

kaysond commented Nov 25, 2021

Final nit: can you add some docs to the lldap_config template? Most people will only look at that when configuring the service

np. done

@nitnelave nitnelave merged commit a1e50de into lldap:main Nov 25, 2021
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.

None yet

2 participants