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 17 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.

17 changes: 15 additions & 2 deletions pkg/corerp/frontend/controller/secretstores/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/radius-project/radius/pkg/armrpc/rest"
"github.com/radius-project/radius/pkg/corerp/datamodel"
"github.com/radius-project/radius/pkg/kubernetes"
"github.com/radius-project/radius/pkg/kubeutil"
rpv1 "github.com/radius-project/radius/pkg/rp/v1"
"github.com/radius-project/radius/pkg/to"
"github.com/radius-project/radius/pkg/ucp/resources"
Expand Down Expand Up @@ -184,6 +185,11 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore,
ref = old.Properties.Resource
}

// resource property cannot be empty for global scoped resource.
if newResource.Properties.BasicResourceProperties.IsGlobalScopedResource() && ref == "" {
return rest.NewBadRequestResponse("$.properties.resource cannot be empty for global scoped resource."), nil
}

ns, name, err := fromResourceID(ref)
if err != nil {
return nil, err
Expand All @@ -195,6 +201,12 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore,
}
}

// Create namespace if not exists.
err = kubeutil.PatchNamespace(ctx, options.KubeClient, ns)
if err != nil {
return nil, err
}

if name == "" {
name = newResource.Name
}
Expand All @@ -208,8 +220,9 @@ func UpsertSecret(ctx context.Context, newResource, old *datamodel.SecretStore,
ksecret := &corev1.Secret{}
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 resource in incoming request references resource, then the resource must exist for a application/environment scoped resource.
// For global scoped resource create the kubernetes resource if not exists.
if ref != "" && !newResource.Properties.BasicResourceProperties.IsGlobalScopedResource() {
return rest.NewBadRequestResponse(fmt.Sprintf("'%s' referenced resource does not exist.", ref)), nil
}
app, _ := resources.ParseResource(newResource.Properties.Application)
Expand Down
112 changes: 109 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,12 @@ 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"
testFileGenericValueInvalidResource = "secretstores_datamodel_global_scope_invalid_resource.json"
testFileGenericValueEmptyResource = "secretstores_datamodel_global_scope_empty_resource.json"
)

func TestGetNamespace(t *testing.T) {
Expand Down Expand Up @@ -466,6 +469,109 @@ 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])
})

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

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

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.Error(t, err)
require.Equal(t, err.Error(), "no Kubernetes namespace")
})

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

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

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

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

// assert
r := resp.(*rest.BadRequestResponse)
require.Equal(t, "$.properties.resource cannot be empty for global scoped resource.", r.Body.Error.Message)
})

t.Run("add secret values to the existing secret store 1 ", func(t *testing.T) {
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValue)
newResource.Properties.Resource = "default/secret"

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

resp, _ := UpsertSecret(context.TODO(), newResource, nil, opt)
r := resp.(*rest.BadRequestResponse)
require.Equal(t, "'default/secret' referenced resource does not exist.", r.Body.Error.Message)
})

t.Run("inherit old resource id for global scoped resource", func(t *testing.T) {
oldResource := testutil.MustGetTestData[datamodel.SecretStore](testFileGenericValueGlobalScope)
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileGenericValueEmptyResource)

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

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

// assert
require.Equal(t, oldResource.Properties.Resource, newResource.Properties.Resource)
})
}

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
@@ -0,0 +1,36 @@
{
"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": {
"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
@@ -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": "secret0",
"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"
}
5 changes: 5 additions & 0 deletions pkg/rp/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ func (b *BasicResourceProperties) EqualLinkedResource(prop *BasicResourcePropert
return strings.EqualFold(b.Application, prop.Application) && strings.EqualFold(b.Environment, prop.Environment)
}

// Method IsGlobalScopedResource checks if resource is global scoped.
func (b *BasicResourceProperties) IsGlobalScopedResource() bool {
return b.Application == "" && b.Environment == ""
}

// ResourceStatus represents the output status of Radius resource.
type ResourceStatus struct {
// Compute represents a resource presented in the underlying platform.
Expand Down
35 changes: 35 additions & 0 deletions pkg/rp/v1/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,38 @@ func TestEqualLinkedResource(t *testing.T) {
require.Equal(t, tt.propA.EqualLinkedResource(&tt.propB), tt.eq)
}
}

func Test_isGlobalScopedResource(t *testing.T) {
tests := []struct {
desc string
basicResourceProperties BasicResourceProperties
isGlobal bool
}{
{
desc: "application scope",
basicResourceProperties: BasicResourceProperties{
Application: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/radius-test-rg/providers/Applications.Core/applications/app1",
},
isGlobal: false,
},
{
desc: "environment scope",
basicResourceProperties: BasicResourceProperties{
Environment: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/radius-test-rg/providers/Applications.core/environments/env0",
},
isGlobal: false,
},
{
desc: "global scope",
basicResourceProperties: BasicResourceProperties{},
isGlobal: true,
},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
act := tt.basicResourceProperties.IsGlobalScopedResource()
require.Equal(t, act, tt.isGlobal)
})
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -5059,7 +5059,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