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

Simplify secret request UX #102

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Simplify secret request UX #102

merged 3 commits into from
Aug 11, 2020

Conversation

jasonodonnell
Copy link
Contributor

@jasonodonnell jasonodonnell commented Aug 10, 2020

Overview

This makes a backwards incompatible change to the UX for requesting secrets from Vault. Changing the UX in a required way is not something I take lightly but I believe now is the best time to get this right.

This change makes requesting secrets more explicit by requiring the full path of the secret, rather than configuring extra parameters such as path and kv-version. By being more explicit with the secret path, mixing secret engines is possible and there are less parameters to configure.

Additionally I added a bit of logic to unpack responses from Vault data smarter (instead of relying on parameters to change the selector logic).

New UX:

- name: Import Secrets
   uses: hashicorp/vault-action
   with:
     url: https://vault.mycompany.com:8200
     token: ${{ secrets.VaultToken }}
     secrets: |
         aws/ci/app accessKey | AWS_ACCESS_KEY_ID ;
         aws/ci/app secretKey | AWS_SECRET_ACCESS_KEY ;
         my-secret/data/ci npm_token

Old UX:

- name: Import Secrets
   uses: hashicorp/vault-action
   with:
     url: https://vault.mycompany.com:8200
     token: ${{ secrets.VaultToken }}
     path: "aws"
     kv-version: 1
     secrets: |
         ci/app accessKey | AWS_ACCESS_KEY_ID ;
         ci/app secretKey | AWS_SECRET_ACCESS_KEY ;
         /my-secret/data/ci npm_token

Testing

To execute the unit tests, run the following commands:

$ docker-compose up -d vault
$ npm run build
$ npm install
$ ./node_modules/jest/bin/jest.js -c integrationTests/basic/jest.config.js

The e2e tests are currently done via a GitHub workflow and cannot be run locally. These will be moved to local integration tests and executed by the workflow in the future.

@jasonodonnell jasonodonnell changed the title Secrets Simplify secret request UX Aug 10, 2020
@RichiCoder1
Copy link
Contributor

RichiCoder1 commented Aug 10, 2020

I was wondering what you meant by simplifying the UX. Looks great, this is exactly what I wanted to do in a breaking change at some point. So much fragile logic in the short-form K/V logic.

Copy link

@Valarissa Valarissa left a comment

Choose a reason for hiding this comment

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

Less magic = more better. Tests pass and changes look good.

@jasonodonnell jasonodonnell merged commit f7f0d5a into master Aug 11, 2020
@jasonodonnell jasonodonnell deleted the secrets branch August 11, 2020 14:06
@vitek-urbanec
Copy link

The breaking change reference here from v2.x release is a bit misleading, because the secret scopes are missing the secret/data prefix in the "new UX" example, like it is described in the master readme. Took me a bit to figure that out.

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.

4 participants