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

feat: add vault preset #833

Merged
merged 7 commits into from
Mar 17, 2023
Merged

feat: add vault preset #833

merged 7 commits into from
Mar 17, 2023

Conversation

zbindenren
Copy link
Contributor

A preset for hashicorp vault.

@zbindenren zbindenren force-pushed the vault branch 2 times, most recently from 6c25883 to ca63b26 Compare March 8, 2023 15:41
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Patch coverage: 81.08% and project coverage change: -7.29 ⚠️

Comparison is base (97f7ed9) 85.87% compared to head (0f5ded8) 78.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
- Coverage   85.87%   78.58%   -7.29%     
==========================================
  Files          50       52       +2     
  Lines        2350     2424      +74     
==========================================
- Hits         2018     1905     -113     
- Misses        173      374     +201     
+ Partials      159      145      -14     
Impacted Files Coverage Δ
preset/vault/preset.go 77.41% <77.41%> (ø)
preset/vault/options.go 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@orlangure
Copy link
Owner

Hi @zbindenren and thank you for the contribution!
This looks very cool and has great tests, well done!

There are a couple more steps that we would need to make in order to finalize this feature (see the contribution guide): preset registry, swagger docs, and most importantly - github actions and circleci jobs to make sure that vault preset is tested on every build (right now your tests are not executed automatically).

Please let me know if you are available to move forward with the additional steps, or if you would like me to take over and add the missing pieces.

Thank you!

chore: add swagger

chore: add vault to registry

chore: update readme

chore: update readme

chore: add tests
@zbindenren
Copy link
Contributor Author

Hi @orlangure

I updated the MR. The swagger tests work too (at least on my laptop). I am not sure if it is good to create a file in /tmp in the swagger test, because creating a random file is not possible. I let you decide. Feel free to remove that option:

{
  "preset": {
    "version": "latest",
    "auth_token": "gnomock-vault-token",
    "auth": [
      {
        "path": "k8s_cluster1",
        "type": "kubernetes"
      }
    ],
    "policies": [
      {
        "name": "empty_policy",
        "data": "{}"
      }
    ],
    "token_create": {
      "policies": [
        "default"
      ],
      "file_path": "/tmp/token"
    }
  },
  "options": {
  }
}

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

Got a closer look at this, amazing job!
I have one concern about TokenCreate option, and would like to hear your opinion on it. Otherwise looks great!

README.md Outdated Show resolved Hide resolved
preset/vault/options.go Outdated Show resolved Hide resolved
}
}

if p.TokenCreate != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure this option fits here. Generally preset options modify the container, but this one actually makes changes to the client file system.

I do like the idea of helper functions though. There is Ingest function in Splunk preset for example. It does not modify the client, but it also doesn't belong in the options. What do you think about removing it as an option, but creating a helper function instead that can be called by clients (test code) whenever a new token needs to be configured?

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 agree with you. I removed the option and created two helper methods which can be handy in tests:

// Client creates a configured vault client for the provided container and token.
func Client(c *gnomock.Container, token string) (*api.Client, error) {
...
}

and

// CreateToken creates an additional access token with the provided policies. Use the same password you provided
// with the WithAuthToken option.
func CreateToken(c *gnomock.Container, rootToken string, policies ...string) (string, error) {
...
}

If all is ok, I can squash the commits and update the MR if you want.

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

This is great, thank you for the contribution!

@orlangure orlangure merged commit eb20d20 into orlangure:master Mar 17, 2023
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.

3 participants