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

refactor(ibmsm): Support v2 API and KV secret types #513

Merged
merged 4 commits into from
Jun 18, 2023

Conversation

jkayani
Copy link
Member

@jkayani jkayani commented May 22, 2023

Description

IBM Cloud SM API v1 is going out of support in a few months and clients must migrate to v2: https://cloud.ibm.com/apidocs/secrets-manager/secrets-manager-v1. This implements the migration for AVP. It also adds support for KV secret types along with logging to indicate when "new" types, that are added by IBM Cloud SM that AVP might not support, are found and skipped. Finally, test for data races since ibmsm uses goroutines to speed things up.

Notable changes in the v2 API/SDK:

  • Secret type isn't a necessary parameter to API calls (ID uniquely identifies a secret regardless of group/type)
  • All secret types are versioned now, so AVP should not ignore version for arbitrary and other secret types when specified in placeholder/annotation
  • The IBM Cloud SM SDK no longer returns the generic SecretResource struct SecretData key that allowed AVP to access the metadata and payload for (almost) any secret type. Instead structs specific to each type are returned (example: https://github.com/IBM/secrets-manager-go-sdk/blob/v2.0.0/secretsmanagerv2/secrets_manager_v2.go#L7211). While they all implement a common interface, it has 0 useful methods. This means AVP must handle each possible type explicitly. For this reason, the SDK return values are wrapped in new types IBMSecretData, IBMVersionedSecretData, IBMSecretMetadata; each type implements a generic GetMetadata or GetSecret method, so that the type-by-type code can be in 1 place and not each function
  • Due to the above, only select keys (whatever was previously included in the SecretData in the v1 API) for each secret type are available for replacing a placeholder. This isn't configurable outside the codebase

Checklist

Please make sure that your PR fulfills the following requirements:

  • Reviewed the guidelines for contributing to this repository
  • The commit message follows the Conventional Commits Guidelines.
  • Tests for the changes have been updated
  • Are you adding dependencies? If so, please run go mod tidy -compat=1.17 to ensure only the minimum is pulled in.
  • Docs have been added / updated
  • Optional. My organization is added to USERS.md.

Type of Change

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe)

Other information

@jkayani jkayani self-assigned this May 22, 2023
@jkayani jkayani marked this pull request as ready for review May 22, 2023 23:30
@jkayani jkayani requested a review from werne2j as a code owner May 22, 2023 23:30
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #513 (9186ef9) into main (717f1c0) will decrease coverage by 4.08%.
The diff coverage is 47.45%.

@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
- Coverage   75.38%   71.30%   -4.08%     
==========================================
  Files          25       25              
  Lines        1694     1903     +209     
==========================================
+ Hits         1277     1357      +80     
- Misses        323      450     +127     
- Partials       94       96       +2     
Impacted Files Coverage Δ
pkg/backends/ibmsecretsmanager.go 60.69% <47.24%> (-18.82%) ⬇️
pkg/config/config.go 84.14% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@werne2j werne2j left a comment

Choose a reason for hiding this comment

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

Looks good

@werne2j werne2j merged commit dbaf2d0 into argoproj-labs:main Jun 18, 2023
3 checks passed
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.

None yet

3 participants