-
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
Adding changes to extend secret stores scope to global #7155
Changes from 16 commits
012c5c3
f7b7e81
68589d6
aba834d
4b8ed5e
240d9f9
b0023e5
3813e86
c784e27
990ca8f
8fc6f02
349b126
01a3435
a3f43b2
720df1c
28b9bdb
2125896
c331be1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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" | ||||
|
@@ -179,11 +180,17 @@ 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 | ||||
} | ||||
|
||||
// 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 | ||||
|
@@ -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}}) | ||||
kachawla marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use kubeutil for creating or updating namespace like below:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated it |
||||
if err != nil && apierrors.IsAlreadyExists(err) { | ||||
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 | ||||
} | ||||
|
@@ -208,8 +224,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) | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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, | ||
"", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
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" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5059,7 +5059,6 @@ | |
} | ||
}, | ||
"required": [ | ||
"application", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
] | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add SecretStores_CreateOrUpdate_GlobalScope.json example like below ? Overall LGTM :) |
||
|
||
#suppress "@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property" | ||
@doc("The type of secret store data") | ||
|
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 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.
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?
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.
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