Adding changes to extend secret stores scope to global #7155
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
pkg/cli/clients/management.go
Outdated
| @@ -51,6 +51,7 @@ var _ ApplicationsManagementClient = (*UCPApplicationsManagementClient)(nil) | |||
|
|
|||
| var ( | |||
| ResourceTypesList = []string{ | |||
| sstr_ctrl.ResourceTypeName, | |||
There was a problem hiding this comment.
Any reason why we moved this to the top?
There was a problem hiding this comment.
I added this change to quickly fix this issue #7070 for testing, will revert this change
| @@ -209,7 +221,7 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore, | |||
| err = options.KubeClient.Get(ctx, runtimeclient.ObjectKey{Namespace: ns, Name: name}, ksecret) | |||
| if apierrors.IsNotFound(err) { | |||
| // If resource in incoming request references resource, then the resource must exist. | |||
| if ref != "" { | |||
| if ref != "" && newResource.Properties.Application != "" { | |||
There was a problem hiding this comment.
If the reference is not empty and the application is not empty, then we have an error, right? Can you explain this part more? And maybe write a short doc on top of the if clause?
There was a problem hiding this comment.
sure, i will add a doc. for global scoped resource ref is not empty and resource may not exist. So here erroring out only if its application scoped
There was a problem hiding this comment.
+1 on adding comment.
Also, are these scenarios (with app empty and non empty) covered in the unit tests?
There was a problem hiding this comment.
added the tests
| LocalID: "Secret", | ||
| ID: resources_kubernetes.IDFromParts( | ||
| resources_kubernetes.PlaneNameTODO, | ||
| "", |
There was a problem hiding this comment.
Does this make a secret resource global scoped?
| @@ -5077,7 +5077,6 @@ | |||
| } | |||
| }, | |||
| "required": [ | |||
| "application", | |||
There was a problem hiding this comment.
The secret store no longer requires to be a part of an application. Makes sense.
| @@ -1,33 +1,32 @@ | |||
| import kubernetes as kubernetes { | |||
| kubeConfig: '' | |||
| namespace: context.runtime.kubernetes.namespace | |||
| namespace: 'radius-testing' | |||
There was a problem hiding this comment.
Why did you decide to hard-code this?
There was a problem hiding this comment.
added these changes for testing, will revert this file
test/functional/shared/resources/testdata/recipes/test-bicep-recipes/rabbitmq-recipe.bicep
Outdated
Show resolved
Hide resolved
test/functional/shared/resources/testdata/recipes/test-bicep-recipes/rabbitmq-recipe.bicep
Outdated
Show resolved
Hide resolved
| @doc("Fully qualified resource ID for the environment that the application is linked to") | ||
| environment?: string; | ||
|
|
||
| @doc("Fully qualified resource ID for the application") | ||
| application?: string; |
There was a problem hiding this comment.
Can a globally scoped resource still have these set?
There was a problem hiding this comment.
My question is should they be optional or not exist at all?
There was a problem hiding this comment.
Here we are just making application as optional so user can skip providing env/app to make it global scope.
just how we have application as optional in environment scoped resources.
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
| @@ -194,6 +198,14 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore, | |||
| return nil, err | |||
| } | |||
| } | |||
| err = options.KubeClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}) | |||
There was a problem hiding this comment.
Why do we need to search for the namespace again if the previous step is getting it from the app/env?
There was a problem hiding this comment.
Is it bc the secret store doesn't have to be scoped to an app anymore? So in the case that it's not, we may need to create a new namespace?
There was a problem hiding this comment.
that is correct, since its not app namespace anymore, we need to chekc if its already present and create a new one if it doesnt exist
pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_global_scope.json
Show resolved
Hide resolved
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
kachawla
dismissed
their stale review
approved by mistake. Pending a few open comments.
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Planning to add it as part of the private repo functional work! as we need to create a global scoped secret store resource. |
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
| if resource.Properties.Application == "" && resource.Properties.Environment == "" { | ||
| return true | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
| if resource.Properties.Application == "" && resource.Properties.Environment == "" { | |
| return true | |
| } | |
| return false | |
| return resource.Properties.Application == "" && resource.Properties.Environment == "" |
There was a problem hiding this comment.
I would move this function to BaseResourceProperties like EqualLinkedResource because this is the function for application and environment in BaseResourceProperties and can be referenced by the other codes:
Lines 54 to 56 in c52aa00
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Karishma Chawla <[email protected]>
Signed-off-by: Karishma Chawla <[email protected]>
kachawla
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
| @@ -195,6 +202,15 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore, | |||
| } | |||
| } | |||
|
|
|||
| err = options.KubeClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}) | |||
There was a problem hiding this comment.
Please use kubeutil for creating or updating namespace like below:
Signed-off-by: Vishwanath Hiremath <[email protected]>
| @@ -47,7 +47,7 @@ model SecretStoreResource is TrackedResourceRequired<SecretStoreProperties, "sec | |||
|
|
|||
| @doc("The properties of SecretStore") | |||
| model SecretStoreProperties { | |||
| ...ApplicationScopedResource; | |||
| ...GlobalScopedResource; | |||
There was a problem hiding this comment.
Can you please add SecretStores_CreateOrUpdate_GlobalScope.json example like below ?
Overall LGTM :)
Signed-off-by: Vishwanath Hiremath <[email protected]>
vishwahiremat
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
…t#7155) # Description - Adding changes to support global scope - Remove appliction as a requred property - Create namespace provided by user if not exists - Adding unit tests Design doc: radius-project/design-notes#38 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7030 --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Signed-off-by: Karishma Chawla <[email protected]> Co-authored-by: Karishma Chawla <[email protected]> Signed-off-by: willdavsmith <[email protected]>
…t#7155) # Description - Adding changes to support global scope - Remove appliction as a requred property - Create namespace provided by user if not exists - Adding unit tests Design doc: radius-project/design-notes#38 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7030 --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Signed-off-by: Karishma Chawla <[email protected]> Co-authored-by: Karishma Chawla <[email protected]>
…t#7155) # Description - Adding changes to support global scope - Remove appliction as a requred property - Create namespace provided by user if not exists - Adding unit tests Design doc: radius-project/design-notes#38 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7030 --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Signed-off-by: Karishma Chawla <[email protected]> Co-authored-by: Karishma Chawla <[email protected]> Signed-off-by: willdavsmith <[email protected]>
Description
Design doc: radius-project/design-notes#38
Type of change
Fixes: #7030