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

Update LoadSecrets() #7796

Merged

Conversation

lakshmimsft
Copy link
Contributor

@lakshmimsft lakshmimsft commented Aug 9, 2024

Description

  1. Update LoadSecrets() to return data for all keys for corresponding secret store when no keys filter is provided.
  2. Update return type to include SecretData{}
  3. Updated existing LoadSecrets() to return data collated for multiple secret store ids
  4. Update to helper function populateSecretData() and it's unit tests which populates secret data map returned.
  5. Update functional test to include envSecrets input

Type of change

Fixes: (Part of #6917)

if secretData[secretStoreID] == nil {
secretData[secretStoreID] = make(map[string]string)
}
secretData[secretStoreID][secretKey] = *secretDataValue.Value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ln96- ln105 is just ln76-ln82 refactored

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.

Project coverage is 61.06%. Comparing base (a58b8cb) to head (c151322).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/recipes/configloader/secrets.go 76.19% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7796      +/-   ##
==========================================
+ Coverage   61.05%   61.06%   +0.01%     
==========================================
  Files         523      523              
  Lines       27466    27474       +8     
==========================================
+ Hits        16770    16778       +8     
- Misses       9211     9212       +1     
+ Partials     1485     1484       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@radius-functional-tests
Copy link

radius-functional-tests bot commented Aug 9, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository lakshmimsft/radius-mainfork
Commit ref e8e4385
Unique ID func017672a42d
Image tag pr-func017672a42d
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.1.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func017672a42d
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func017672a42d
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func017672a42d
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func017672a42d
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

@lakshmimsft lakshmimsft force-pushed the lakshmimsft/updtoLoadSecrets branch from e8e4385 to 5ed6808 Compare August 13, 2024 23:22
@lakshmimsft lakshmimsft marked this pull request as ready for review August 13, 2024 23:22
@lakshmimsft lakshmimsft requested review from a team as code owners August 13, 2024 23:22
@radius-functional-tests
Copy link

radius-functional-tests bot commented Aug 13, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository lakshmimsft/radius-mainfork
Commit ref 5ed6808
Unique ID func09d6d3a6f7
Image tag pr-func09d6d3a6f7
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func09d6d3a6f7
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func09d6d3a6f7
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func09d6d3a6f7
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func09d6d3a6f7
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

@lakshmimsft lakshmimsft force-pushed the lakshmimsft/updtoLoadSecrets branch from 5ed6808 to 6f13718 Compare August 15, 2024 20:52
// If the input secret keys filter for a secret store ID is nil or empty, it retrieves secret data for all keys for that secret store.
// When the secret keys filter is populated for a secret store ID, it retrieves secret data for the specified keys for the associated secret store ID.
// The function returns a map of secret data, where the keys are the secret store IDs and the values are maps of secret keys and their corresponding values.
func (e *secretsLoader) LoadSecrets(ctx context.Context, secretStoreIDKeysFilter map[string][]string) (secretData map[string]map[string]string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just name this filter and give an example in the doc above?

Suggested change
func (e *secretsLoader) LoadSecrets(ctx context.Context, secretStoreIDKeysFilter map[string][]string) (secretData map[string]map[string]string, err error) {
func (e *secretsLoader) LoadSecrets(ctx context.Context, filter map[string][]string) (secretData map[string]map[string]string, err error) {

@lakshmimsft lakshmimsft force-pushed the lakshmimsft/updtoLoadSecrets branch from fbbad7c to 1aec7a4 Compare August 22, 2024 16:15
@lakshmimsft lakshmimsft changed the title Update LoadSecrets() to return data for all keys for corresponding secret store when no keys filter is provided Update LoadSecrets() Aug 22, 2024
@radius-functional-tests
Copy link

radius-functional-tests bot commented Aug 22, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository lakshmimsft/radius-mainfork
Commit ref cbf1cbc
Unique ID funcf15ed1e337
Image tag pr-funcf15ed1e337
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcf15ed1e337
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcf15ed1e337
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-funcf15ed1e337
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcf15ed1e337
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

@@ -24,13 +24,15 @@ resource env 'Applications.Core/environments@2023-10-01-preview' = {
providers: {
Copy link
Contributor

Choose a reason for hiding this comment

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

are these changes related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. The functional test needs to be updated with envSecrets block. This PR updates code that can affect running of functional test with envSecrets. We'll include this test in the PR.

@@ -225,7 +225,7 @@ func (e executor) setEnvironmentVariables(tf *tfexec.Terraform, options Options)
for secretName, secretReference := range recipeConfig.EnvSecrets {
// Extract secret value from the secrets input
if secretIDs, ok := options.Secrets[secretReference.Source]; ok {
if secretValue, ok := secretIDs[secretReference.Key]; ok {
if secretValue, ok := secretIDs.Data[secretReference.Key]; ok {
envVarUpdate = true
Copy link
Contributor

Choose a reason for hiding this comment

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

id it gonna throw nil pointer if secretIDs is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

secretIDs is a struct and will not be nil. Renamed variable to secretData. There exists a unit test to check for secretData.Data when nil.

bicep-types Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

are these changes related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed from PR.

@lakshmimsft lakshmimsft force-pushed the lakshmimsft/updtoLoadSecrets branch 3 times, most recently from 1aec7a4 to 93f111b Compare August 27, 2024 23:33
@radius-functional-tests
Copy link

radius-functional-tests bot commented Aug 27, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository lakshmimsft/radius-mainfork
Commit ref 93f111b
Unique ID func4f779ac8c1
Image tag pr-func4f779ac8c1
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func4f779ac8c1
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func4f779ac8c1
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-func4f779ac8c1
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func4f779ac8c1
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded
❌ corerp-cloud functional test failed. Please check the logs for more details
⌛ Starting corerp-cloud functional tests...
❌ corerp-cloud functional test failed. Please check the logs for more details
⌛ Starting corerp-cloud functional tests...
❌ corerp-cloud functional test failed. Please check the logs for more details

Copy link
Contributor

@vishwahiremat vishwahiremat left a comment

Choose a reason for hiding this comment

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

lgtm

@lakshmimsft lakshmimsft force-pushed the lakshmimsft/updtoLoadSecrets branch from 93f111b to c151322 Compare August 29, 2024 17:13
@radius-functional-tests
Copy link

radius-functional-tests bot commented Aug 29, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository lakshmimsft/radius-mainfork
Commit ref c151322
Unique ID funcf1269ca09b
Image tag pr-funcf1269ca09b
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr: 1.12.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcf1269ca09b
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcf1269ca09b
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-funcf1269ca09b
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcf1269ca09b
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

@lakshmimsft lakshmimsft merged commit 84cb120 into radius-project:main Aug 29, 2024
28 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.

3 participants