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.

Sign up for GitHub

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

Add new secret types to Applications.Core/secretstores #7816

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Add new secret types to Applications.Core/secretstores #7816

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1c8c832
upd to secretstore type
lakshmimsft Aug 16, 2024
ef474c3
format tsp
lakshmimsft Aug 16, 2024
d77161f
updating tests to be table driven
lakshmimsft Aug 16, 2024
8fc7505
correcting comment on recipe env variables
lakshmimsft Aug 22, 2024
81fbfbe
run make generate
lakshmimsft Aug 22, 2024
b5527f7
updated per comments
lakshmimsft Aug 29, 2024
09f8a82
Rename file to temporary name to register case change
lakshmimsft Aug 29, 2024
896cfc1
Rename file to lowercase name
lakshmimsft Aug 29, 2024
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
144 changes: 93 additions & 51 deletions ...bicep-types-radius/generated/applications/applications.core/2023-10-01-preview/types.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions hack/bicep-types-radius/generated/index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
"$ref": "applications/applications.core/2023-10-01-preview/types.json#/197"
},
"Applications.Core/secretStores@2023-10-01-preview": {
"$ref": "applications/applications.core/2023-10-01-preview/types.json#/226"
"$ref": "applications/applications.core/2023-10-01-preview/types.json#/232"
},
"Applications.Core/volumes@2023-10-01-preview": {
"$ref": "applications/applications.core/2023-10-01-preview/types.json#/263"
"$ref": "applications/applications.core/2023-10-01-preview/types.json#/269"
},
"Applications.Dapr/pubSubBrokers@2023-10-01-preview": {
"$ref": "applications/applications.dapr/2023-10-01-preview/types.json#/44"
Expand Down
12 changes: 12 additions & 0 deletions pkg/corerp/api/v20231001preview/secretstore_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ func toSecretStoreDataTypeDataModel(src *SecretStoreDataType) datamodel.SecretTy
return datamodel.SecretTypeGeneric
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add validations here during conversion if the required keys are present for the particular type. ex: "clientID" and "tenantID" keys should be present for type "azureWorkloadIdentity" type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed offline. Currently validations are included in

radius/pkg/corerp/frontend/controller/secretstores/kubernetes.go

Line 90 in e577624

func ValidateAndMutateRequest(ctx context.Context, newResource *datamodel.SecretStore, oldResource *datamodel.SecretStore, options *controller.Options) (rest.Response, error) {
in line with other validations required for creating this resource. This function is accessed by controller here:

radius/pkg/corerp/setup/setup.go

Line 172 in e577624

_ = ns.AddResource("secretStores", &builder.ResourceOption[*datamodel.SecretStore, datamodel.SecretStore]{

including @kachawla to confirm this is the right approach.

case SecretStoreDataTypeCertificate:
return datamodel.SecretTypeCert
case SecretStoreDataTypeBasicAuthentication:
return datamodel.SecretTypeBasicAuthentication
case SecretStoreDataTypeAzureWorkloadIdentity:
return datamodel.SecretTypeAzureWorkloadIdentity
case SecretStoreDataTypeAwsIRSA:
return datamodel.SecretTypeAWSIRSA
}

return datamodel.SecretTypeGeneric
Expand All @@ -117,6 +123,12 @@ func fromSecretStoreDataTypeDataModel(src datamodel.SecretType) *SecretStoreData
return to.Ptr(SecretStoreDataTypeGeneric)
case datamodel.SecretTypeCert:
return to.Ptr(SecretStoreDataTypeCertificate)
case datamodel.SecretTypeBasicAuthentication:
return to.Ptr(SecretStoreDataTypeBasicAuthentication)
case datamodel.SecretTypeAzureWorkloadIdentity:
return to.Ptr(SecretStoreDataTypeAzureWorkloadIdentity)
case datamodel.SecretTypeAWSIRSA:
return to.Ptr(SecretStoreDataTypeAwsIRSA)
Comment on lines +126 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Should a unit test be added?

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 in #7867

}
return nil
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/corerp/api/v20231001preview/zz_generated_constants.go
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion pkg/corerp/api/v20231001preview/zz_generated_models.go
View file
Open in desktop

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

6 changes: 6 additions & 0 deletions pkg/corerp/datamodel/secretstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ const (
SecretTypeGeneric SecretType = "generic"
// SecretTypeCert is the certificate secret type.
SecretTypeCert SecretType = "certificate"
// SecretTypeBasicAuthentication is the basicAuthentication secret type.
SecretTypeBasicAuthentication SecretType = "basicAuthentication"
// SecretTypeAzureWorkloadIdentity is the azureWorkloadIdentity secret type.
SecretTypeAzureWorkloadIdentity SecretType = "azureWorkloadIdentity"
// SecretTypeAWSIRSA is the awsIRSA secret type.
SecretTypeAWSIRSA SecretType = "awsIRSA"
)

// SecretStore represents secret store resource.
Expand Down
21 changes: 20 additions & 1 deletion pkg/corerp/frontend/controller/secretstores/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ func getOrDefaultType(t datamodel.SecretType) (datamodel.SecretType, error) {
t = datamodel.SecretTypeGeneric
case datamodel.SecretTypeCert:
case datamodel.SecretTypeGeneric:
case datamodel.SecretTypeBasicAuthentication:
case datamodel.SecretTypeAzureWorkloadIdentity:
case datamodel.SecretTypeAWSIRSA:
default:
err = fmt.Errorf("'%s' is invalid secret type", t)
}
Expand Down Expand Up @@ -75,8 +78,15 @@ func getOrDefaultEncoding(t datamodel.SecretType, e datamodel.SecretValueEncodin
return e, err
}

// Define a map of required keys for each SecretType
var requiredKeys = map[datamodel.SecretType][]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be only referenced from ValidateAndMutateRequest function so I'd assume it should be scoped to that. Could you tell me more about the motivation to define it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into function: #7867

datamodel.SecretTypeBasicAuthentication: {RequiredUsername, RequiredPassword},
datamodel.SecretTypeAzureWorkloadIdentity: {RequiredClientId, RequiredTenantId},
datamodel.SecretTypeAWSIRSA: {RequiredRoleARN},
}

// ValidateAndMutateRequest checks the type and encoding of the secret store, and ensures that the secret store data is
// valid. If any of these checks fail, a BadRequestResponse is returned.
// valid and required keys are present for the secret type. If any of these checks fail, a BadRequestResponse is returned.
func ValidateAndMutateRequest(ctx context.Context, newResource *datamodel.SecretStore, oldResource *datamodel.SecretStore, options *controller.Options) (rest.Response, error) {
var err error
newResource.Properties.Type, err = getOrDefaultType(newResource.Properties.Type)
Expand Down Expand Up @@ -116,6 +126,15 @@ func ValidateAndMutateRequest(ctx context.Context, newResource *datamodel.Secret
}
}

// Validate that required keys for the secret type are present in the secret data
if keys, ok := requiredKeys[newResource.Properties.Type]; ok {
for _, key := range keys {
if _, ok := newResource.Properties.Data[key]; !ok {
return rest.NewBadRequestResponse(fmt.Sprintf("$.properties.data must contain '%s' key for %s type.", key, newResource.Properties.Type)), nil
}
}
}

return nil, nil
}

Expand Down
153 changes: 107 additions & 46 deletions pkg/corerp/frontend/controller/secretstores/kubernetes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ const (
testFileGenericValueGlobalScope = "secretstores_datamodel_global_scope.json"
testFileGenericValueInvalidResource = "secretstores_datamodel_global_scope_invalid_resource.json"
testFileGenericValueEmptyResource = "secretstores_datamodel_global_scope_empty_resource.json"

testFileBasicAuthentication = "secretstores_datamodel_basicauth.json"
testFileBasicAuthenticationInvalid = "secretstores_datamodel_basicauth_invalid.json"
testFileAWSIRSA = "secretstores_datamodel_awsirsa.json"
testFileAzureWorkloadIdentity = "secretstores_datamodel_azwi.json"
)

func TestGetNamespace(t *testing.T) {
Expand Down Expand Up @@ -247,53 +252,109 @@ func TestGetOrDefaultEncoding(t *testing.T) {
}

func TestValidateAndMutateRequest(t *testing.T) {
t.Run("default type is generic", func(t *testing.T) {
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
newResource.Properties.Type = ""

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

// assert
require.Equal(t, datamodel.SecretTypeGeneric, newResource.Properties.Type)
})

t.Run("new resource, but referencing valueFrom", func(t *testing.T) {
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
newResource.Properties.Resource = ""
resp, err := ValidateAndMutateRequest(context.TODO(), newResource, nil, nil)
require.NoError(t, err)

// assert
r := resp.(*rest.BadRequestResponse)
require.True(t, r.Body.Error.Message == "$.properties.data[tls.crt].Value must be given to create the secret." ||
r.Body.Error.Message == "$.properties.data[tls.key].Value must be given to create the secret.")
})

t.Run("update the existing resource - type not matched", func(t *testing.T) {
oldResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
oldResource.Properties.Type = datamodel.SecretTypeGeneric
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
resp, err := ValidateAndMutateRequest(context.TODO(), newResource, oldResource, nil)
require.NoError(t, err)

// assert
r := resp.(*rest.BadRequestResponse)
require.Equal(t, "$.properties.type cannot change from 'generic' to 'certificate'.", r.Body.Error.Message)
})

t.Run("inherit resource id from existing resource", func(t *testing.T) {
oldResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
newResource.Properties.Resource = ""
resp, err := ValidateAndMutateRequest(context.TODO(), newResource, oldResource, nil)
tests := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

name string
testFile string
oldResource *datamodel.SecretStore
modifyResource func(*datamodel.SecretStore, *datamodel.SecretStore)
assertions func(*testing.T, rest.Response, error, *datamodel.SecretStore, *datamodel.SecretStore)
}{
{
name: "default type is generic",
testFile: testFileCertValueFrom,
modifyResource: func(newResource, oldResource *datamodel.SecretStore) {
newResource.Properties.Type = ""
},
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
require.Nil(t, resp)
require.Equal(t, datamodel.SecretTypeGeneric, newResource.Properties.Type)
},
},
{
name: "new resource, but referencing valueFrom",
testFile: testFileCertValueFrom,
modifyResource: func(newResource, oldResource *datamodel.SecretStore) {
newResource.Properties.Resource = ""
},
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
r := resp.(*rest.BadRequestResponse)
require.True(t, r.Body.Error.Message == "$.properties.data[tls.crt].Value must be given to create the secret." ||
r.Body.Error.Message == "$.properties.data[tls.key].Value must be given to create the secret.")
},
},
{
name: "update the existing resource - type not matched",
testFile: testFileCertValueFrom,
oldResource: testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom),
modifyResource: func(newResource, oldResource *datamodel.SecretStore) {
oldResource.Properties.Type = datamodel.SecretTypeGeneric
},
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
r := resp.(*rest.BadRequestResponse)
require.Equal(t, "$.properties.type cannot change from 'generic' to 'certificate'.", r.Body.Error.Message)
},
},
{
name: "inherit resource id from existing resource",
testFile: testFileCertValueFrom,
oldResource: testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom),
modifyResource: func(newResource, oldResource *datamodel.SecretStore) {
newResource.Properties.Resource = ""
},
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
require.Nil(t, resp)
require.Equal(t, oldResource.Properties.Resource, newResource.Properties.Resource)
},
},
{
name: "new basicAuthentication resource",
testFile: testFileBasicAuthentication,
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
require.Nil(t, resp)
},
},
{
name: "new awsIRSA resource",
testFile: testFileAWSIRSA,
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
require.Nil(t, resp)
},
},
{
name: "new azureWorkloadIdentity resource",
testFile: testFileAzureWorkloadIdentity,
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
require.Nil(t, resp)
},
},
{
name: "invalid basicAuthentication resource",
testFile: testFileBasicAuthenticationInvalid,
assertions: func(t *testing.T, resp rest.Response, err error, newResource, oldResource *datamodel.SecretStore) {
require.NoError(t, err)
r := resp.(*rest.BadRequestResponse)
require.True(t, r.Body.Error.Message == "$.properties.data must contain 'password' key for basicAuthentication type.")
},
},
}

// assert
require.NoError(t, err)
require.Nil(t, resp)
require.Equal(t, oldResource.Properties.Resource, newResource.Properties.Resource)
})
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
newResource := testutil.MustGetTestData[datamodel.SecretStore](tt.testFile)
if tt.modifyResource != nil {
tt.modifyResource(newResource, tt.oldResource)
}
resp, err := ValidateAndMutateRequest(context.TODO(), newResource, tt.oldResource, nil)
tt.assertions(t, resp, err, newResource, tt.oldResource)
})
}
}

func TestUpsertSecret(t *testing.T) {
Expand Down
29 changes: 29 additions & 0 deletions pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_AWSIRSA.json
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_awsirsa.json

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"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": {
"application": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Applications.Core/applications/app0",
"type": "awsIRSA",
"data": {
"roleARN": {
"value": "test-role-arn"
}
}
},
"tenantId": "00000000-0000-0000-0000-000000000000",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"resourceGroup": "testGroup",
"createdApiVersion": "2023-10-01-preview",
"updatedApiVersion": "2023-10-01-preview"
}
32 changes: 32 additions & 0 deletions pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_azwi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"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": {
"application": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Applications.Core/applications/app0",
"type": "azureWorkloadIdentity",
"data": {
"clientId": {
"value": "test-client-Id"
},
"tenantId": {
"value": "test-tenant-Id"
}
}
},
"tenantId": "00000000-0000-0000-0000-000000000000",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"resourceGroup": "testGroup",
"createdApiVersion": "2023-10-01-preview",
"updatedApiVersion": "2023-10-01-preview"
}
32 changes: 32 additions & 0 deletions pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_basicauth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"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": {
"application": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Applications.Core/applications/app0",
"type": "basicAuthentication",
"data": {
"username": {
"value": "uname123"
},
"password": {
"value": "testpwd-dGxzLmNlcnQK"
}
}
},
"tenantId": "00000000-0000-0000-0000-000000000000",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"resourceGroup": "testGroup",
"createdApiVersion": "2023-10-01-preview",
"updatedApiVersion": "2023-10-01-preview"
}
Loading
Loading