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

Adding an example of using symbolic links to map to a tempfile. #201

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

rpothier
Copy link
Contributor

@rpothier rpothier commented Feb 2, 2021

What does this PR do?

This adds example files and a README of how a symbolic link can
be used to map a temp file to a fixed.

Also updates the main Summon README.md

What ticket does this PR close?

Resolves #190

Checklists

Change log

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
  • This PR does not require updating any documentation

@rpothier rpothier requested a review from a team as a code owner February 2, 2021 20:55
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

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

These examples are going to be very helpful!
I took a stab at rewording some things for clarity,
although I suspect @izgeri will be able to vastly
improve on any suggestions that I make. :)


* `-i, --ignore` A secret path for which to ignore provider errors.
* `-i, --ignore <path-to-provider>` A secret path for which to ignore provider
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a weird one, but CodeClimate is complaining that "Lists should be surrounded by blank lines", even though this list is surrounded by blank lines. I suspect that this is happening because this list uses * to make bullets, and prior lists in the README used -. So replacing the *s in this list with -s might make these warnings go away.

Copy link
Contributor Author

@rpothier rpothier Feb 3, 2021

Choose a reason for hiding this comment

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

The other difference is previously this fit on one line under 80 chars, but with the change the line goes over 80 so it goes to the next line. So it CodeClimate saying that a list must be under 80 char and cannot go to the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i tried a commit changing the * to - as you can see in commit 4e1fdc5. But in this case the CodeClimate complaint persists. So it seems to make CodeClimate happy the line needs to be less that 80 chars, which may not be optimum for the readers. I have restored the -s back to the *s.

examples/symlinks/README.md Outdated Show resolved Hide resolved
examples/symlinks/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,22 @@
#!/bin/bash
Copy link
Contributor

@diverdane diverdane Feb 2, 2021

Choose a reason for hiding this comment

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

This should be changed to:

#!/usr/bin/env bash

to make this portable (esp. to get this to work on MacOS).
The reason this weird shebang is needed is that on Macs, /bin/bash is typically an old bash (Version 3) that doesn't support associative arrays. Using the above shebang will find the first bash in the $PATH, which will be a newer version (version 4 or newer) on Macs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change, however on my mac I am not getting bash version 4, it is still version 3. Starting with Catalina zsh is the default shell and it seems Apple is not going to bash 4.0 ( unless the user specifically downloads it).

removed by the first session that ends. If removing the symbolic link is
ommitted, the symbolic link will be orphined.

Note: the above examples were tested in a Linux (Ubuntu) environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be tested on Mac, given that the summon-symlinks script uses associative arrays, which probably won't work on Macs without the shebang change I mentioned for summon-symlinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some differences in the syntax for bash and zsh. As noted above the mac does not have bash 4 out of the box. One option is to add if and elif to test for bash of zsh, another would be to change the code not to use anything that is specific to one or the other. A third option is to upload a zsh example. I will post the latter for review but can change to one of the other options if that is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

another would be to change the code not to use anything that is specific to one or the other
I like this option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the script to use only what is compatible between zsh and bash. Tested on Ubuntu and macOS


## Method 2

This uses a helper script called summon-symlinks that is in the example
Copy link
Contributor

@diverdane diverdane Feb 2, 2021

Choose a reason for hiding this comment

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

Maybe clearer to describe when to use Method 2, e.g.:

When you have several secrets variables that you'd like to map to
fixed locations, then you may want to use the `summon-symlinks` helper
script. . . . <more description here>...

Also, we'll need a sample secrets-symlinks.yml file that shows how
to map tempfiles for each var to fixed target locations...
...OR, include a sample content as a comment in summon-symlinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the description. The secrets-symlinks.yml is already included.

examples/symlinks/README.md Outdated Show resolved Hide resolved
#!/bin/bash
set -euo pipefail;
declare -A mappings;

Copy link
Contributor

@diverdane diverdane Feb 2, 2021

Choose a reason for hiding this comment

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

We might want to consider allowing this script to be passed the name
of a secrets-symlinks YAML (as first arg $1), or maybe with a -f flag,
instead of a fixed secrets-symlinks.yml.
The complex part would be adjusting the "@" (all args)
on the last line to skip the optional file args.

BUT, if this is way complicated, perhaps skip this.

Either way, we should include a sample content for a secrets-symlinks.yml,
with maybe a one-liner explanation, e.g.:

This script provides a wrapper for a Summon subcommand to
perform the following:
- Add symbolic links mapping tempfiles for given secrets variables to
   fixed locations (before invoking subcommand)
- Remove these symbolic links (after invoking subcommand)
The mapping of secrets variables to desired fixed file locations
is specified in the `secrets-symlinks.yml` file. For example:

<EXAMPLE secrets-symlinks.yml CONTENT GOES HERE>

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an envvar works here i.e. "${SUMMON_SYMLINKS:-./secrets-symlinks.yml}".
Here's a command from testing:

SUMMON_SYMLINKS=<(echo 'A: ./C') \
  summon \
    --yaml 'A: !var:file meow' \
    -p /bin/echo \
    ./summon-symlink \
      sh -c 'cat $PWD/C'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is an example of secrets-symlinks.yml in the example directory. This has two rows to show it can be used
for multiple files.

examples/symlinks/summon-symlinks Outdated Show resolved Hide resolved
examples/symlinks/summon-symlinks Outdated Show resolved Hide resolved
@izgeri
Copy link
Contributor

izgeri commented Feb 3, 2021

@diverdane I came to check this out and you've already done a super thorough job!! :)

@rpothier rpothier force-pushed the summon-symlinks branch 2 times, most recently from 4e1fdc5 to 78aad96 Compare February 4, 2021 14:11
@rpothier rpothier self-assigned this Feb 4, 2021
@rpothier rpothier force-pushed the summon-symlinks branch 6 times, most recently from 4c30767 to 4f2a698 Compare February 9, 2021 16:40
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

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

LGTM!

README.md Outdated

There are times when you would like to have certain secrets values available at
fixed locations, e.g. `/etc/ssl/cert.pem` for an SSL certificate. This can be
accomplished by using symbolic links as describe in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo... "describe" should be "described".

@rpothier rpothier merged commit b072abe into master Feb 12, 2021
@rpothier rpothier deleted the summon-symlinks branch February 12, 2021 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Specify (non-variable) tempfile name
4 participants