-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Templating of Server Config & Monitoring #168
base: master
Are you sure you want to change the base?
Conversation
Hi Matt, Thanks a lot for your generous contribution! I'll review ASAP |
Ok so I changed my mind.. the |
Sure thing. Thanks a lot once again for your contribution |
No problem.. my router that I usually run my VPN on started acting up and I got to looking for a simple docker image that I could use and ran across yours... then of course I wanted to make modifications... and well one things leads to another and I might as well push them back up.. |
CLIENT_ID="$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1)" | ||
ARG1=$1 | ||
DEFAULT_ID="$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1)" | ||
CLIENT_ID="${ARG1:=$DEFAULT_ID}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I more or less did what was mentioned here. I also modified how start.sh
called this function.
@@ -38,10 +40,14 @@ then | |||
fi | |||
;; | |||
o) | |||
CLIENT_PATH="$(createConfig $2)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and for all the other cases I moved the createConfig
call to the cases and passed in the second/third optional argument which is mean to be the cert name. If its not passed the cert name will revert to the random name.
The last thing I am going to probably do is add |
Hi Matt, I really appreciate your commitment. Like I said I'll have a detailed look when I finalize the feature I'm currently working on. Generally speaking it's always better to raise smaller PRs for incremental improvements, but it's OK to have it as one lump too. I'll then cover with tests here https://github.com/dockovpn/dockovpn-it |
CLIENT_PATH="$APP_PERSIST_DIR/clients/$CLIENT_ID" | ||
[ -d $CLIENT_PATH ] && CLIENT_PATH=${CLIENT_PATH}_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if given path already exists it's better to return an error code. Otherwise, we will end up in a situation where we have two certificates with the same subject name (CLIENT_ID is present in generated certificate in subject field), which in its turn may lead to other problems. For instance, when we remove client with rmclient.sh
we supply subject name as a parameter and certificate is revoked.
|
||
WORKDIR ${APP_INSTALL_PATH} | ||
|
||
COPY scripts . | ||
COPY config ./config | ||
COPY VERSION ./config | ||
|
||
RUN apk add --no-cache openvpn easy-rsa bash netcat-openbsd zip dumb-init && \ | ||
RUN apk add --no-cache python3 py-pip openvpn easy-rsa bash netcat-openbsd zip dumb-init && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thoroughly reviewed the templating part and it's truly amazing! However it brings in another execution environment i.e python and package dependencies which leads to much greater image size. At the moment I'd like to keep resulting image as small as possible for the benefit of embedded and RISK architectures.
I'm still working out some bugs but I wanted to open this PR to make you aware of my modifications. I wanted to be able to modify some of the server configs a little easier and so I modified it to be more like a template. Instead of copying the
server.conf
to/etc/openvpn
at build time, I am doing it as part of thestart.sh
. I created variables that have defaults of the original values, and modified thedocker-compose.yml
to demo some of them.Additionally I ran across this project and thought it to be very useful and a simple thing to add to the
docker-compose.yml
.If you want me to break these two things out into seperate PRs I can.
I also thought about using something like cookie to template the
server.conf
but you're using analpine
image and I didn't really want to muck around trying to get it installed bloating the image more than was actually needed. Instead I just went with a simpleeval
method that will inject variables into theserver.conf
I dunno..... thoughts?