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

Adding changes to extend secret stores scope to global #7155

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions pkg/corerp/api/v20231001preview/zz_generated_models.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 14 additions & 2 deletions pkg/corerp/frontend/controller/secretstores/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/radius-project/radius/pkg/ucp/resources"
resources_kubernetes "github.com/radius-project/radius/pkg/ucp/resources/kubernetes"
"github.com/radius-project/radius/pkg/ucp/store"
"github.com/radius-project/radius/pkg/ucp/ucplog"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -179,11 +180,14 @@ func fromResourceID(id string) (ns string, name string, err error) {
// UpsertSecret creates or updates a Kubernetes secret based on the incoming request and returns the secret's location in
// the output resource.
func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore, options *controller.Options) (rest.Response, error) {
logger := ucplog.FromContextOrDiscard(ctx)
ref := newResource.Properties.Resource
if ref == "" && old != nil {
ref = old.Properties.Resource
}

if newResource.Properties.Application == "" && newResource.Properties.Resource == "" {
return rest.NewBadRequestResponse("$.properties.resource cannot be \"\" for global scoped resource."), nil
kachawla marked this conversation as resolved.
Show resolved Hide resolved
kachawla marked this conversation as resolved.
Show resolved Hide resolved
}
ns, name, err := fromResourceID(ref)
vishwahiremat marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
Expand All @@ -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}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to search for the namespace again if the previous step is getting it from the app/env?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

kachawla marked this conversation as resolved.
Show resolved Hide resolved
vishwahiremat marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

Please use kubeutil for creating or updating namespace like below:

err = kubeutil.PatchNamespace(ctx, p.Client, component.GetNamespace())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated it

if apierrors.IsAlreadyExists(err) {
kachawla marked this conversation as resolved.
Show resolved Hide resolved
logger.Info("Using existing namespace", "namespace", ns)
} else if err != nil {
return nil, err
} else {
logger.Info("Created the namespace", "namespace", ns)
}

if name == "" {
name = newResource.Name
Expand All @@ -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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on adding comment.

Also, are these scenarios (with app empty and non empty) covered in the unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the tests

return rest.NewBadRequestResponse(fmt.Sprintf("'%s' referenced resource does not exist.", ref)), nil
}
app, _ := resources.ParseResource(newResource.Properties.Application)
Expand Down
44 changes: 41 additions & 3 deletions pkg/corerp/frontend/controller/secretstores/kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ const (
testEnvID = testRootScope + "/Applications.Core/environments/env0"
testAppID = testRootScope + "/Applications.Core/applications/app0"

testFileCertValueFrom = "secretstores_datamodel_cert_valuefrom.json"
testFileCertValue = "secretstores_datamodel_cert_value.json"
testFileGenericValue = "secretstores_datamodel_generic.json"
testFileCertValueFrom = "secretstores_datamodel_cert_valuefrom.json"
testFileCertValue = "secretstores_datamodel_cert_value.json"
testFileGenericValue = "secretstores_datamodel_generic.json"
testFileGenericValueGlobalScope = "secretstores_datamodel_global_scope.json"
)

func TestGetNamespace(t *testing.T) {
Expand Down Expand Up @@ -466,6 +467,43 @@ func TestUpsertSecret(t *testing.T) {
require.Equal(t, "'app0-ns/secret1' of $.properties.resource must be same as 'app0-ns/secret0'.", r.Body.Error.Message)
})

t.Run("create a new secret resource with global scope", func(t *testing.T) {
ctrl := gomock.NewController(t)
sc := store.NewMockStorageClient(ctrl)

newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileGenericValueGlobalScope)

opt := &controller.Options{
StorageClient: sc,
KubeClient: k8sutil.NewFakeKubeClient(nil),
}

_, err := ValidateAndMutateRequest(context.TODO(), newResource, nil, opt)
require.NoError(t, err)
_, err = UpsertSecret(context.TODO(), newResource, nil, opt)
require.NoError(t, err)

// assert
require.Equal(t, "test-namespace/secret0", newResource.Properties.Resource)
ksecret := &corev1.Secret{}

err = opt.KubeClient.Get(context.TODO(), runtimeclient.ObjectKey{Namespace: "test-namespace", Name: "secret0"}, ksecret)
require.NoError(t, err)

require.Equal(t, "dGxzLmNydA==", string(ksecret.Data["tls.crt"]))
require.Equal(t, "dGxzLmNlcnQK", string(ksecret.Data["tls.key"]))
require.Equal(t, "MTAwMDAwMDAtMTAwMC0xMDAwLTAwMDAtMDAwMDAwMDAwMDAw", string(ksecret.Data["servicePrincipalPassword"]))
require.Equal(t, rpv1.OutputResource{
LocalID: "Secret",
ID: resources_kubernetes.IDFromParts(
resources_kubernetes.PlaneNameTODO,
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make a secret resource global scoped?

resources_kubernetes.KindSecret,
"test-namespace",
"secret0"),
}, newResource.Properties.Status.OutputResources[0])
})

}

func TestDeleteSecret(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Applications.Core/secretStores/secret0",
"name": "secret0",
"type": "applications.core/secretstores",
"location": "global",
"systemData": {
"createdAt": "2022-03-22T18:54:52.6857175Z",
"createdBy": "[email protected]",
"createdByType": "User",
"lastModifiedAt": "2022-03-22T18:57:52.6857175Z",
"lastModifiedBy": "[email protected]",
"lastModifiedByType": "User"
},
"provisioningState": "Succeeded",
"properties": {
"resource": "test-namespace/secret0",
kachawla marked this conversation as resolved.
Show resolved Hide resolved
"type": "generic",
"data": {
"tls.crt": {
"encoding": "raw",
"value": "tls.crt"
},
"tls.key": {
"encoding": "base64",
"value": "dGxzLmNlcnQK"
},
"servicePrincipalPassword": {
"value": "10000000-1000-1000-0000-000000000000"
}
}
},
"tenantId": "00000000-0000-0000-0000-000000000000",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"resourceGroup": "testGroup",
"createdApiVersion": "2023-10-01-preview",
"updatedApiVersion": "2023-10-01-preview"
}
Original file line number Diff line number Diff line change
Expand Up @@ -5077,7 +5077,6 @@
}
},
"required": [
"application",
Copy link
Contributor

Choose a reason for hiding this comment

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

The secret store no longer requires to be a part of an application. Makes sense.

"data"
]
},
Expand Down
2 changes: 1 addition & 1 deletion typespec/Applications.Core/secretstores.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ model SecretStoreResource is TrackedResourceRequired<SecretStoreProperties, "sec

@doc("The properties of SecretStore")
model SecretStoreProperties {
...ApplicationScopedResource;
...GlobalScopedResource;

Choose a reason for hiding this comment

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

Can you please add SecretStores_CreateOrUpdate_GlobalScope.json example like below ?

https://github.com/radius-project/radius/blob/main/typespec/Applications.Core/examples/2023-10-01-preview/SecretStores_CreateOrUpdate.json

Overall LGTM :)


#suppress "@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property"
@doc("The type of secret store data")
Expand Down
17 changes: 17 additions & 0 deletions typespec/radius/v1/resources.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,23 @@ model ApplicationScopedResource {
status?: ResourceStatus;
}

@doc("Base properties of a Global-scoped resource")
model GlobalScopedResource {
@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;
Comment on lines +67 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a globally scoped resource still have these set?

Copy link
Contributor

Choose a reason for hiding this comment

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

My question is should they be optional or not exist at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


@doc("The status of the asynchronous operation.")
@visibility("read")
provisioningState?: ProvisioningState;

@doc("Status of a resource.")
@visibility("read")
status?: ResourceStatus;
}

@doc("IdentitySettings is the external identity setting.")
model IdentitySettings {
@doc("kind of identity setting")
Expand Down
Loading