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 upgrading vault when installed with archive #166

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

Conversation

rgevaert
Copy link
Contributor

@rgevaert rgevaert commented Sep 8, 2020

This makes it possible to upgrade vault when it was installed with an archive.

Copy link
Owner

@jsok jsok left a comment

Choose a reason for hiding this comment

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

Thanks so much for this improvement, just a small change and some clarification around why the chosen approach was taken.

@@ -35,6 +35,7 @@
owner => $::vault::user,
group => $::vault::group,
mode => $::vault::config_mode,
notify => Class['vault::service'],
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please split this out into its own commit as it addresses a different issue.

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 don't really understand why it should be a different commit. As when you update the binary you need to restart the service. I can do it, just want to understand why :)

Choose a reason for hiding this comment

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

It’d be good to have this in its own commit b/c it addresses a different new/changed functionality. Just in case it needs to be reverted separately from the addition of versioned installs.

}

exec { 'install_versioned_vault':
command => "/bin/cp -f ${_vault_versioned_bin} ${vault_bin}",
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason to do a cp instead of a symlink? This approach was discussed in #63 but I can't recall the specifics of if it was/wasn't feasible.

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 tried with a symlink. But the issue is that you can't set the file_capabilities on a symlink. Keeping to the symlink path meant changing the code in other places. This was the easiest way to introduce this functionality.

Choose a reason for hiding this comment

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

@rgevaert symlinks are pass-through, so whatever they link to is the mode in play.

E.g., if you symlink ${_vault_versioned_bin}${vault_bin}, whatever mode is assigned to ${vault_bin} will be the mode used.

So this is probably cleaner done as a symlink:

file { $vault_bin:
  ensure => link,
  target => $vault_versioned_bin,
  notify => Class['vault::service'],
}

And let whatever mode the actual bin has be the mode in play. If it needs to be set, I believe you’ll want to do that in the Archive["${::vault::download_dir}/${::vault::download_filename}"] resource above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I tried that first but had issues like mentioned before. If someone else has time to pick it up, please do.

Copy link

Choose a reason for hiding this comment

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

Using a symlink will throw an error when setting mlock on the $vault_bin, which is done at

file => $vault_bin,
. A way to fix this would be to reference $_vault_versioned_bin instead.

@rgevaert
Copy link
Contributor Author

rgevaert commented Dec 9, 2020

@jsok I did you changes you requested.

@Fuco1
Copy link

Fuco1 commented Jun 9, 2021

What's the status here?

@jeffbyrnes
Copy link

This came up today with an in-place upgrade my team was trying to do, and it’d be great to get the last few changes in & get it merged!

@thunderpants73
Copy link

Any update on this? This seems like it would be a great addition....

@e1nh4nd3r
Copy link

Any updates on this PR?

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.

7 participants