-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add datamodel changes for secret support to Terraform Providers #7731
Add datamodel changes for secret support to Terraform Providers #7731
Conversation
3d1cf71
to
ab22cc1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7731 +/- ##
==========================================
+ Coverage 61.09% 61.12% +0.02%
==========================================
Files 520 520
Lines 27164 27190 +26
==========================================
+ Hits 16596 16620 +24
- Misses 9102 9103 +1
- Partials 1466 1467 +1 ☔ View full report in Codecov by Sentry. |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
e8f7dc9
to
1bc32ad
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
1bc32ad
to
3ffbe4d
Compare
3ffbe4d
to
d5ed611
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lakshmimsft could you update the description with link to the API design doc corresponding to these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lakshmimsft are typespec changes mostly pulled in from #7199 or have they diverged significantly since then?
} | ||
|
||
dm := map[string][]datamodel.ProviderConfigProperties{} | ||
for k, v := range config.Terraform.Providers { | ||
dm[k] = []datamodel.ProviderConfigProperties{} | ||
|
||
for _, providerAdditionalProperties := range v { | ||
var addnlProperties map[string]any | ||
var convertedSecrets map[string]datamodel.SecretReference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted from what? Can we just call it tfProviderSecrets
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function has been updated. pls take another look.
// Account for possibility of nested additional properties during unmarshalling | ||
// https://json-schema.org/understanding-json-schema/reference/object#additionalproperties | ||
if tmpAddnlProperties, ok := addnlProperties["additionalProperties"]; ok && addnlProperties["additionalProperties"] != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect users to add additional properties within each provider config? Could you point me to the relevant section of the design doc for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is not needed. function has been updated. pls take another look.
if tmpSecret, ok := value.(map[string]*SecretReference); !ok { | ||
return nil, v1.ErrInvalidModelConversion | ||
} else { | ||
propSecrets := getSecretsFromProviderProperties(tmpSecret) | ||
|
||
// Add the secrets to the convertedSecrets map | ||
for secretKey, secretValue := range propSecrets { | ||
if convertedSecrets == nil { | ||
convertedSecrets = map[string]datamodel.SecretReference{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is getting difficult to follow with abbreviations being used which I'm sure are obvious to you while writing the code but I don't know how to interpret tmpSecret
and propSecret
. Can you look into making it more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed. pls take another look.
// check if additional properties contain Secrets | ||
// get AdditionalProperties from providerAdditionalProperties excluding the 'secrets' key. | ||
for key, value := range addnlProperties { | ||
if key == "secrets" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we fetching it from additionalProperties instead of this explicit field assigned for secrets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed and has been removed.
test/functional-portable/corerp/noncloud/resources/recipe_terraform_test.go
Outdated
Show resolved
Hide resolved
/*{ | ||
Name: secretName, | ||
Type: validation.SecretStoresResource, | ||
},*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these to be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will enable these to test functional test updates once bicep PR is merged in.
@@ -181,6 +187,7 @@ func Test_TerraformRecipe_KubernetesPostgres(t *testing.T) { | |||
ValidateLabels(false), | |||
validation.NewK8sPodForResource(appName, "postgres"). | |||
ValidateLabels(false), | |||
// validation.NewK8sSecretForResourceWithResourceName(secretResourceName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will uncomment this to test functional test updates once bicep PR is merged in.
6888a35
to
305d9df
Compare
305d9df
to
9f8b2d0
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
361cfb4
to
a807390
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
a807390
to
848376c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go once the functional test is uncommented
@@ -73,4 +78,16 @@ resource pgsapp 'Applications.Core/extenders@2023-10-01-preview' = { | |||
} | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may need a reformat here
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
# Description Added datamodel updates for secret support to Terraform Providers configuration. This includes secrets in recipeConfig under specific Provider configurations and environment variables. Pls see link to associated [Design Doc](https://github.com/radius-project/design-notes/pull/47/files) NOTE: Updates to functional tests are commented till we merge in corresponding PR in bicep repo : radius-project/bicep#751 ## Type of change - This pull request fixes a bug in Radius and has an approved issue (#6539) Fixes: #6539
…us-project#7731) # Description Added datamodel updates for secret support to Terraform Providers configuration. This includes secrets in recipeConfig under specific Provider configurations and environment variables. Pls see link to associated [Design Doc](https://github.com/radius-project/design-notes/pull/47/files) NOTE: Updates to functional tests are commented till we merge in corresponding PR in bicep repo : radius-project/bicep#751 ## Type of change - This pull request fixes a bug in Radius and has an approved issue (radius-project#6539) Fixes: radius-project#6539 Signed-off-by: Reshma Abdul Rahim <[email protected]>
Description
Added datamodel updates for secret support to Terraform Providers configuration. This includes secrets in recipeConfig under specific Provider configurations and environment variables.
Pls see link to associated Design Doc
NOTE: Updates to functional tests are commented till we merge in corresponding PR in bicep repo : radius-project/bicep#751
Type of change
Fixes: #6539