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

Merging in code from my private module #26

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

avitacco
Copy link

Pull Request (PR) description

This is a large PR and it addresses several (IMO) shortcomings with the existing module. Because this PR is large and contains opinionated code, I am more than happy to change/remove/split any of the proposed changes.

First, it makes a lot of changes to the main class parameters. It reorders most of the parameters, adds data types for all parameters, and adds documentation in the same order as they are defined. It also adds parameters for every possible configuration option currently available for vault.

Second, it adds the capability to output HCL configuration (via the config_output parameter). This is handled with a lot of templates. They include and iterate over other templates.

And, finally, it adds the ability to upgrade archive installs through the version parameter. The code uses an exec to see if the currently installed version of vault is equal to the version defined. If it isn't the binary is deleted so the archive stanza can install the expected version.

This Pull Request (PR) fixes the following issues

Fixes #18

Copy link

@sboyd-m sboyd-m left a comment

Choose a reason for hiding this comment

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

There must be a better way to do those templates than all the if not undef. It makes them completely unreadable, and very hard to review for typos.

You may be able to use the * pattern to pass in parameters to the templates, and if you combine that with the delete_undef_values function, you can really just for-loop the templates to be key value expansions. I have not tried if the * pattern works on templates, and if it does not, it would be a shame.

README.md Outdated
@@ -36,6 +36,7 @@ Please see [The official documentation](https://www.vaultproject.io/docs/configu
* `manage_group`: Whether or not the module should create the group.
* `bin_dir`: Directory the vault executable will be installed in.
* `config_dir`: Directory the vault configuration will be kept in.
* `config_output`: What format to output the configuration in (`hcl` or `json`) (defalut: `json`)
Copy link

Choose a reason for hiding this comment

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

Suggested change
* `config_output`: What format to output the configuration in (`hcl` or `json`) (defalut: `json`)
* `config_output`: What format to output the configuration in (`hcl` or `json`) (default: `json`)

extra_config => $vault::extra_config,
}
)
}
file { "${vault::config_dir}/config.json":
Copy link

Choose a reason for hiding this comment

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

This file path is not really correct anymore, and furthermore there shouldn't be hcl in a file called .json.
Note that the package-provided service has forced the use of a different file for quite some time:
https://github.com/hashicorp/vault/blob/main/.release/linux/package/usr/lib/systemd/system/vault.service#L23

The location of this file should be another parameter, but if the user sets manage repo, changing this parameter would need to be blocked and forced to this path given by hashicorp.

Copy link
Author

Choose a reason for hiding this comment

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

I think the path should be correct here. It is /etc/vault.d if the user is installing from a repo or /etc/vault if they aren't. If that isn't correct, what should it be?

AmbientCapabilities=CAP_IPC_LOCK
CapabilityBoundingSet=CAP_SYSLOG CAP_IPC_LOCK
NoNewPrivileges=yes
ExecStart=<%= $bin_dir %>vault server -config=<%= $config_dir %>vault.hcl
Copy link

Choose a reason for hiding this comment

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

This filename will need to match whatever is actually set by the module

@avitacco
Copy link
Author

I am sorry that I completely disappeared after making this PR. I switched jobs and roles and life has been pretty chaotic.

To address the question of using the * parameter, I did originally consider that because it seemed like the simplest way to go, however, that pattern doesn't work in this case since data can be different types. I'm open to changing the templates because having a check for every variable sucks, but I can't think of a better way to do it that will still ensure that all of the types are set correctly.

avitacco added a commit to avitacco/puppet-vault that referenced this pull request May 23, 2024
* Fixes typo in README
* Ensures the config file name is correct
* Sets the config file extension to reflect the type of config
* Updates the service template to use the correct config file
avitacco added 11 commits July 15, 2024 15:15
* Fixes typo in README
* Ensures the config file name is correct
* Sets the config file extension to reflect the type of config
* Updates the service template to use the correct config file
* Updated spec use systemd unit file (and removed ensure)
* Updated systemd template to have configurable user and group
* Updated systemd template to have service options
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.

Vault preferres HCL
6 participants