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 support for template args #255

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

chriswells0
Copy link
Collaborator

@chriswells0 chriswells0 commented Sep 6, 2020

Arguments can be defined in a Bastillefile using ARG:

# With a default:
ARG user=root
# Without a default:
ARG domain
# Then used in subsequent values:
CMD echo "${username}@${domain}"

An ARG file can also be used to define a list of variables one per line (e.g., for use in template hooks).

Values can be provided individually when applying the template:

bastille template webjail nginx --arg username=admin --arg domain=example.com

They can also be placed one per line in a file such as values.txt:

username=admin
domain=example.com

Then provide the file name using --arg-file:

bastille template webjail nginx --arg-file values.txt

Note that in both cases, only variables defined via ARG (command or hook file) will have their values replaced.

Closes #211

@tobiastom
Copy link
Contributor

Thank you @chriswells0 for doing this work. I really like to see this feature in Bastille.

Adding ARG version=74 to a Bastillefile works, and I can successfully use it inside the template. This is already great!
During a template call, I can also successfully add the --arg version=73 arguments and it will also work.

One thing I cannot get to work is the --arg-file. If the file does not exists, the template command gives me the appropriate error.
If I specify a correct one with the following content, it does not seem to be used. It will fallback to the default value, from the ARG version=74 statement inside the Bastillefile.

version=73

Also, I noticed a small usage "issue": if the ARG does not have a default value, and I do not specify it, I would assume that Bastille complains about it. Right now it will just use an empty value and continue with its work.
Kid of related to this, I also would expect it to complain if I provided an ARG, which is not defined inside the Bastillefile.
Do you think this makes sense?

Except that, this is pretty awesome and if there is anything I can help with to get this into Bastille, let me know.


P.S.: One thing I'm missing, which might not be part of this pull request, is the ability to use ARG values inside the OVERLAY or COPY statements. This would make it possible to e.g. provide an IP as --arg php_backend=192.168.1.2 and I could configure a Nginx backend like this:

location /phpinfo {
        fastcgi_pass ${php_backend}:9000;
        fastcgi_param SCRIPT_FILENAME /usr/local/www/phpinfo.php;
}

@chriswells0
Copy link
Collaborator Author

If I specify a correct one with the following content, it does not seem to be used.

I had a double grep on line 65. I'm sure I tested that feature, so I must have added it later by mistake. Thanks for catching that.

if the ARG does not have a default value, and I do not specify it, I would assume that Bastille complains about it.

I toyed with that idea also, but I left it out because forcing a value to be provided means optional values aren't possible. I'm open to feedback on it, but that was my reasoning. In general, it would be good to follow how it works in a Dockerfile--not because "they're always right" but because it makes for an easier back and forth transition for users. I checked their docs and found this:

If a user specifies a build argument that was not defined in the Dockerfile, the build outputs a warning.

That seems like a good approach: a warning message in yellow or whatever color existing Bastille warnings use. What do you think?

One thing I'm missing, which might not be part of this pull request, is the ability to use ARG values inside the OVERLAY or COPY statements.

Funny enough, your example (except for Apache) is the use case I have that made me want to implement this feature. I just haven't made the time to actually work on my template, so I haven't looked at how I would do it. I'm hesitant to change every file copied into a jail, because that could lead to unexpected results (imagine a script that looks for a placeholder having its placeholder replaced with a value). Plus, it would mean we have to recursively iterate every file being copied in, which is far more complicated than the bulk copies that are happening now.

In thinking about it briefly, it seems letting the user include their own script to update specific files makes sense. I've done that in Docker for NGINX deployments except in the entrypoint script, which we don't have. For us, it could be called in the template and passed the values it needs (substituted at time of execution): CMD initConfig.sh ${domain}

Thoughts?

@tobiastom
Copy link
Contributor

I had a double grep on line 65. I'm sure I tested that feature, so I must have added it later by mistake. Thanks for catching that.

Works like a charm now.

if the ARG does not have a default value, and I do not specify it, I would assume that Bastille complains about it.

I toyed with that idea also, but I left it out because forcing a value to be provided means optional values aren't possible. I'm open to feedback on it, but that was my reasoning.

Just a little side note: right now they are not optional values as well. They are just empty. If you would want to have a default empty value, it could be defined like version=.

In general, it would be good to follow how it works in a Dockerfile--not because "they're always right" but because it makes for an easier back and forth transition for users. I checked their docs and found this:

If a user specifies a build argument that was not defined in the Dockerfile, the build outputs a warning.

That seems like a good approach: a warning message in yellow or whatever color existing Bastille warnings use. What do you think?

Perfect solution for me.

One thing I'm missing, which might not be part of this pull request, is the ability to use ARG values inside the OVERLAY or COPY statements.

Funny enough, your example (except for Apache) is the use case I have that made me want to implement this feature. I just haven't made the time to actually work on my template, so I haven't looked at how I would do it. I'm hesitant to change every file copied into a jail, because that could lead to unexpected results (imagine a script that looks for a placeholder having its placeholder replaced with a value). Plus, it would mean we have to recursively iterate every file being copied in, which is far more complicated than the bulk copies that are happening now.

Yeah, it should not be the default behaviour of COPY or TEMPLATE. These files might contain anything, so it could get messy quickly.

In thinking about it briefly, it seems letting the user include their own script to update specific files makes sense. I've done that in Docker for NGINX deployments except in the entrypoint script, which we don't have. For us, it could be called in the template and passed the values it needs (substituted at time of execution): CMD initConfig.sh ${domain}

Thoughts?

Thats how I implemented it right now, but I see one problem with this approach: everyone would need to implement the replacement of the variables themselves. From what I understand Bastille should make the life in jails easier.

What about a new statement: APPLY /usr/local/etc/nginx.conf (name if of course just an initial idea, a more intuitive one would be nice). This would search for all defined ARGs inside the provided file (e.g. $(version)) and replace them with their values.

This would also make the use of OVERLAY and COPY very straightforward: first copy the template into the jail, and after that apply the variables to individual files. If you run it again, the same order would apply and the same result would appear. If the templates is changed, they get copied again and the new template will get the defined variables applied.

What do you think?

@chriswells0
Copy link
Collaborator Author

I had thought about a new command for it also, but I was hesitant to create a new one. I could add and see what @cedwards thinks about it. Maybe something related to args to make it clear? REPLACE_ARGS, ARG_SUB, what?

I don't know if I'll be able to get to it this weekend. I'm replacing the kitchen sink and faucet, and I had to replace the pipes to get everything aligned. :(

@tobiastom
Copy link
Contributor

I think from these options I like APPLY or REPLACE_ARGS best. ARG_SUB has too many abbreviations for my taste.

@chriswells0
Copy link
Collaborator Author

My apologies for the delay. I've pushed an update for this PR to add the warning about empty arg values. There is also a new RENDER hook and Bastillefile command that allows you to specify a file or directory whose contents should have the args replaced by their values.

@tobiastom
Copy link
Contributor

Thank you! I'll test this as soon as possible!

@tobiastom
Copy link
Contributor

RENDER works perfectly for me. Also the warning message is really nice. For me this makes Bastille a lot more flexible and I cannot wait to see it inside an upcoming release.

One thing which might be nice: display the template that is processed and where it is stored (like OVERLAY). Right now it is quite silent about what it is doing.

Thank you again @chriswells0 for working on this!

@cedwards cedwards merged commit 2483fdd into BastilleBSD:master Nov 10, 2020
@chriswells0 chriswells0 deleted the template-args branch November 26, 2020 01:13
indgy pushed a commit to indgy/bastille that referenced this pull request Apr 23, 2021
indgy pushed a commit to indgy/bastille that referenced this pull request Apr 23, 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.

Provide way to capture user input in templates[ENHANCEMENT]
3 participants