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(vault): support for multiple general purpose credential paths #4360

Merged
merged 7 commits into from
May 22, 2023

Conversation

Jk1484
Copy link
Contributor

@Jk1484 Jk1484 commented May 15, 2023

Changes

Created a wrapper for an existing function to support multiple iterations of fetching secrets from the vault.

  • Tests
  • Documentation

@Jk1484 Jk1484 requested a review from a team as a code owner May 15, 2023 12:35
@Jk1484 Jk1484 force-pushed the muhammadali.VaultSeveralPaths branch 3 times, most recently from 383e7d3 to 2a0f6cd Compare May 15, 2023 19:24
@Jk1484 Jk1484 force-pushed the muhammadali.VaultSeveralPaths branch 4 times, most recently from 6262947 to 0118818 Compare May 16, 2023 13:03
Copy link
Member

@jliempt jliempt left a comment

Choose a reason for hiding this comment

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

I like the solution, and I checked that the original implementation with vaultCredentialPath (and vaultTestCredentialPath) as a string keeps working, but using multiple paths outputs Not fetching credentials from vault since they are not (properly) configured for me.

pkg/config/vault.go Outdated Show resolved Hide resolved
@Jk1484 Jk1484 force-pushed the muhammadali.VaultSeveralPaths branch from 0118818 to 599f1f6 Compare May 17, 2023 13:32
@Jk1484 Jk1484 force-pushed the muhammadali.VaultSeveralPaths branch from 599f1f6 to 6a527ab Compare May 17, 2023 13:49
@jliempt
Copy link
Member

jliempt commented May 17, 2023

/it-go

@jliempt jliempt changed the title feat(vault): support for multiple credential paths feat(vault): support for multiple general purpose credential paths May 17, 2023
Copy link
Member

@jliempt jliempt left a comment

Choose a reason for hiding this comment

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

It's working well now!

// Test empty and non-empty custom general purpose credential prefix
envPrefixes := []string{"CUSTOM_MYCRED1_", ""}
for idx, envPrefix := range envPrefixes {
tEnvPrefix := envPrefix
Copy link
Member

Choose a reason for hiding this comment

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

What does tEnvPrefix do, compared to just using envPrefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

envPrefix is passed to t.Run() but it is running in parallel using t.Parallel(), so envPrefix is mutated on each iteration leaving us with a bug, the value of envPrefix will be the last element of envPrefixes. So in order to avoid it, we can just create a variable on each iteration and pass it to t.Run() which is running in parallel.

pkg/config/vault_test.go Show resolved Hide resolved
@Jk1484 Jk1484 force-pushed the muhammadali.VaultSeveralPaths branch from cde154f to 1e5c36f Compare May 18, 2023 13:55
@Jk1484
Copy link
Contributor Author

Jk1484 commented May 18, 2023

/it-go

1 similar comment
@Jk1484
Copy link
Contributor Author

Jk1484 commented May 22, 2023

/it-go

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jliempt
Copy link
Member

jliempt commented May 22, 2023

/it-go

@jliempt jliempt merged commit 27c3c3c into master May 22, 2023
@jliempt jliempt deleted the muhammadali.VaultSeveralPaths branch May 22, 2023 08:49
maxatsap pushed a commit to maxatsap/jenkins-library that referenced this pull request Jul 23, 2024
…AP#4360)

* created wrapper

* tests added

* update documentation

* tests data race fix

---------

Co-authored-by: Jordi van Liempt <[email protected]>
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.

2 participants