-
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 3 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,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 | ||||
|
@@ -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. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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
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 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 | ||||
|
@@ -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. Choose a reason for hiding this commentThe 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? 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. 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. Choose a reason for hiding this commentThe 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? 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. added the tests |
||||
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,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) { | ||
|
@@ -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, | ||
"", | ||
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]) | ||
}) | ||
|
||
} | ||
|
||
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 |
---|---|---|
|
@@ -5077,7 +5077,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 |
---|---|---|
@@ -1,33 +1,32 @@ | ||
import kubernetes as kubernetes { | ||
kubeConfig: '' | ||
namespace: context.runtime.kubernetes.namespace | ||
namespace: 'radius-testing' | ||
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. Why did you decide to hard-code this? 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. added these changes for testing, will revert this file |
||
} | ||
|
||
param context object | ||
|
||
@description('Specifies the RabbitMQ username.') | ||
param username string = 'guest' | ||
|
||
@description('Specifies the RabbitMQ password.') | ||
@secure() | ||
param password string | ||
param password string = 'guest' | ||
|
||
resource rabbitmq 'apps/Deployment@v1' = { | ||
metadata: { | ||
name: 'rabbitmq-${uniqueString(context.resource.id)}' | ||
name: 'rabbitmq-test' | ||
vishwahiremat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
spec: { | ||
selector: { | ||
matchLabels: { | ||
app: 'rabbitmq' | ||
resource: context.resource.name | ||
resource: 'rabbitmq-test' | ||
} | ||
} | ||
template: { | ||
metadata: { | ||
labels: { | ||
app: 'rabbitmq' | ||
resource: context.resource.name | ||
resource: 'rabbitmq-test' | ||
} | ||
} | ||
spec: { | ||
|
@@ -59,13 +58,13 @@ resource rabbitmq 'apps/Deployment@v1' = { | |
|
||
resource svc 'core/Service@v1' = { | ||
metadata: { | ||
name: 'rabbitmq-${uniqueString(context.resource.id)}' | ||
name: 'rabbitmq-svc' | ||
} | ||
spec: { | ||
type: 'ClusterIP' | ||
selector: { | ||
app: 'rabbitmq' | ||
resource: context.resource.name | ||
resource: 'rabbitmq-test' | ||
vishwahiremat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
ports: [ | ||
{ | ||
|
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") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
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 a globally scoped resource still have these set? 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. My question is should they be optional or not exist at all? 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. Here we are just making application as optional so user can skip providing env/app to make it global scope. |
||
|
||
@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") | ||
|
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.
Any reason why we moved this to the top?
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 added this change to quickly fix this issue #7070 for testing, will revert this change