Skip to content

Commit

Permalink
remove validation of resource types against the static list in recipe…
Browse files Browse the repository at this point in the history
…s codebase (#8391)

# Description

Since we now have APIs that lets us query the valid resourcetypes and
the resource types can include UDTs, we refactored the code to remove
the validation of resource types against the static list of redaius
resource types.

Also, After this PR, we covered the tests for these tasks. 

- Manually test registration of Recipe for a nonexistent resource type
- Manually test Recipe webhook for a nonexistent resource type.
Operation/processing might fail but creation should happen


## Type of change


- This pull request fixes a bug in Radius and has an approved issue
(issue link required).


Fixes:
https://dev.azure.com/azure-octo/Incubations/_workitems/edit/13653/

## Contributor checklist
Please verify that the PR meets the following requirements, where
applicable:

- An overview of proposed schema changes is included in a linked GitHub
issue.
    - [ ] Yes
    - [x] Not applicable
- A design document PR is created in the [design-notes
repository](https://github.com/radius-project/design-notes/), if new
APIs are being introduced.
    - [ ] Yes
    - [x] Not applicable
- The design document has been reviewed and approved by Radius
maintainers/approvers.
    - [ ] Yes
    - [x] Not applicable
- A PR for the [samples
repository](https://github.com/radius-project/samples) is created, if
existing samples are affected by the changes in this PR.
    - [ ] Yes
    - [x] Not applicable
- A PR for the [documentation
repository](https://github.com/radius-project/docs) is created, if the
changes in this PR affect the documentation or any user facing updates
are made.
    - [ ] Yes
    - [x] Not applicable
- A PR for the [recipes
repository](https://github.com/radius-project/recipes) is created, if
existing recipes are affected by the changes in this PR.
    - [ ] Yes
    - [x] Not applicable
  • Loading branch information
nithyatsu authored Mar 7, 2025
1 parent ba41b53 commit 0a138c7
Show file tree
Hide file tree
Showing 9 changed files with 15 additions and 123 deletions.
6 changes: 2 additions & 4 deletions pkg/controller/reconciler/recipe_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"strings"

radappiov1alpha3 "github.com/radius-project/radius/pkg/controller/api/radapp.io/v1alpha3"
portableresources "github.com/radius-project/radius/pkg/rp/portableresources"
"github.com/radius-project/radius/pkg/ucp/ucplog"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -90,11 +89,10 @@ func (r *RecipeWebhook) validateRecipeType(ctx context.Context, recipe *radappio
logger := ucplog.FromContextOrDiscard(ctx)
var errList field.ErrorList
flPath := field.NewPath("spec").Child("type")
validResourceTypes := strings.Join(portableresources.GetValidPortableResourceTypes(), ", ")

logger.Info("Validating Recipe Type %s in Recipe %s", recipe.Spec.Type, recipe.Name)
if !portableresources.IsValidPortableResourceType(recipe.Spec.Type) {
errList = append(errList, field.Invalid(flPath, recipe.Spec.Type, fmt.Sprintf("allowed values are: %s", validResourceTypes)))
if recipe.Spec.Type == "" || strings.Count(recipe.Spec.Type, "/") != 1 {
errList = append(errList, field.Invalid(flPath, recipe.Spec.Type, "must be in the format 'ResourceProvider.Namespace/resourceType'"))

return nil, apierrors.NewInvalid(
schema.GroupKind{Group: "radapp.io", Kind: "Recipe"},
Expand Down
7 changes: 2 additions & 5 deletions pkg/controller/reconciler/recipe_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@ import (
"fmt"
"net"
"net/http"
"strings"
"testing"
"time"

radappiov1alpha3 "github.com/radius-project/radius/pkg/controller/api/radapp.io/v1alpha3"
portableresources "github.com/radius-project/radius/pkg/rp/portableresources"
"github.com/radius-project/radius/test/testcontext"

"github.com/stretchr/testify/require"
Expand All @@ -44,7 +42,7 @@ import (
const (
defaultNamespace = "default"
validResourceType = "Applications.Core/extenders"
invalidResourceType = "Applications.Core/invalidType"
invalidResourceType = "invalidType"
webhookConfigName = "recipe-webhook-config"
)

Expand Down Expand Up @@ -234,8 +232,7 @@ func Test_Webhook_ValidateFunctions(t *testing.T) {
}

if tr.wantErr {
validResourceTypes := strings.Join(portableresources.GetValidPortableResourceTypes(), ", ")
expectedError := fmt.Sprintf("Recipe.radapp.io \"%s\" is invalid: spec.type: Invalid value: \"%s\": allowed values are: %s", tr.recipeName, tr.typeName, validResourceTypes)
expectedError := fmt.Sprintf("Recipe.radapp.io \"%s\" is invalid: spec.type: Invalid value: \"%s\": must be in the format 'ResourceProvider.Namespace/resourceType'", tr.recipeName, tr.typeName)
require.True(t, apierrors.IsInvalid(err))
require.EqualError(t, err, expectedError)

Expand Down
5 changes: 2 additions & 3 deletions pkg/corerp/api/v20231001preview/environment_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/radius-project/radius/pkg/kubernetes"
types "github.com/radius-project/radius/pkg/recipes"

rp_util "github.com/radius-project/radius/pkg/rp/portableresources"
rpv1 "github.com/radius-project/radius/pkg/rp/v1"
"github.com/radius-project/radius/pkg/to"
)
Expand Down Expand Up @@ -67,8 +66,8 @@ func (src *EnvironmentResource) ConvertTo() (v1.DataModelInterface, error) {
if src.Properties.Recipes != nil {
envRecipes := make(map[string]map[string]datamodel.EnvironmentRecipeProperties)
for resourceType, recipes := range src.Properties.Recipes {
if !rp_util.IsValidPortableResourceType(resourceType) {
return &datamodel.Environment{}, v1.NewClientErrInvalidRequest(fmt.Sprintf("invalid resource type: %q", resourceType))
if resourceType == "" || strings.Count(resourceType, "/") != 1 {
return &datamodel.Environment{}, v1.NewClientErrInvalidRequest(fmt.Sprintf("invalid resource type: %q. Must be in the format \"ResourceProvider.Namespace/resourceType\"", resourceType))
}
envRecipes[resourceType] = map[string]datamodel.EnvironmentRecipeProperties{}
for recipeName, recipeDetails := range recipes {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func TestConvertVersionedToDataModel(t *testing.T) {
},
{
filename: "environmentresource-invalid-resourcetype.json",
err: &v1.ErrClientRP{Code: v1.CodeInvalid, Message: "invalid resource type: \"Applications.Dapr/pubsub\""},
err: &v1.ErrClientRP{Code: v1.CodeInvalid, Message: "invalid resource type: \"pubsub\". Must be in the format \"ResourceProvider.Namespace/resourceType\""},
},
{
filename: "environmentresource-invalid-templatekind.json",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
}
},
"recipes": {
"Applications.Dapr/pubsub": {
"pubsub": {
"cosmos-recipe": {
"templateKind": "bicep",
"templatePath": "br:ghcr.io/sampleregistry/radius/recipes/pubsub"
Expand Down
11 changes: 6 additions & 5 deletions pkg/corerp/backend/deployment/deploymentprocessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"strings"

v1 "github.com/radius-project/radius/pkg/armrpc/api/v1"
rp_pr "github.com/radius-project/radius/pkg/rp/portableresources"
rp_util "github.com/radius-project/radius/pkg/rp/util"
rpv1 "github.com/radius-project/radius/pkg/rp/v1"

Expand Down Expand Up @@ -225,6 +224,9 @@ func (dp *deploymentProcessor) getApplicationAndEnvironmentForResourceID(ctx con

// 2. fetch the application properties from the DB
app := &corerp_dm.Application{}
if res.AppID == nil {
return nil, nil, fmt.Errorf("application ID is not set for the resource %q", id.String())
}
err = rp_util.FetchScopeResource(ctx, dp.databaseClient, res.AppID.String(), app)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -575,17 +577,16 @@ func (dp *deploymentProcessor) getResourceDataByID(ctx context.Context, resource

func (dp *deploymentProcessor) buildResourceDependency(resourceID resources.ID, applicationID string, resource v1.DataModelInterface, outputResources []rpv1.OutputResource, computedValues map[string]any, secretValues map[string]rpv1.SecretValueReference, recipeData portableresources.RecipeData) (ResourceData, error) {
var appID *resources.ID
// Application id is mandatory for some of the core resource types and is a required field.
if applicationID != "" {
parsedID, err := resources.ParseResource(applicationID)
if err != nil {
return ResourceData{}, v1.NewClientErrInvalidRequest(fmt.Sprintf("application ID %q for the resource %q is not a valid id. Error: %s", applicationID, resourceID.String(), err.Error()))
}
appID = &parsedID
} else if rp_pr.IsValidPortableResourceType(resourceID.TypeSegments()[0].Type) {
// Application id is optional for portable resource types
appID = nil
} else {
return ResourceData{}, fmt.Errorf("missing required application id for the resource %q", resourceID.String())
// Application id is optional for portable resource types and UDTs.
appID = nil
}

return ResourceData{
Expand Down
2 changes: 1 addition & 1 deletion pkg/corerp/backend/deployment/deploymentprocessor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ func Test_Render(t *testing.T) {

_, err := dp.Render(ctx, resourceID, &testResource)
require.Error(t, err)
require.Equal(t, "missing required application id for the resource \"/subscriptions/test-subscription/resourceGroups/test-resource-group/providers/Applications.Core/containers/test-resource\"", err.Error())
require.Equal(t, "application ID is not set for the resource \"/subscriptions/test-subscription/resourceGroups/test-resource-group/providers/Applications.Core/containers/test-resource\"", err.Error())
})

t.Run("Invalid application resource type", func(t *testing.T) {
Expand Down
70 changes: 0 additions & 70 deletions pkg/rp/portableresources/portableresources.go

This file was deleted.

33 changes: 0 additions & 33 deletions pkg/rp/portableresources/portableresources_test.go

This file was deleted.

0 comments on commit 0a138c7

Please sign in to comment.