-
Notifications
You must be signed in to change notification settings - Fork 93
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
Implementation of resource providers and resource types registration #7967
base: main
Are you sure you want to change the base?
Conversation
@@ -128,6 +128,25 @@ func HandlerForController(controller ctrl.Controller, operationType v1.Operation | |||
} | |||
} | |||
|
|||
// CreateHandler creates an http.Handler for the given resource type and operation method. |
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.
For reviewers, here's a suggested tour:
- Start with the
.tsp
, converters, and datamodel. That will give you an idea of the API surface being added. - Next look at the frontend changes. This is where I implemented support for child-resources.
- Next look at the routing changes in UCP. It was impossible to implement the correct behavior using the current design of routing. UCP's requirements are more complex.
- Last take a look at the integration tests + backend conrollers.
@@ -128,6 +128,25 @@ func HandlerForController(controller ctrl.Controller, operationType v1.Operation | |||
} | |||
} | |||
|
|||
// CreateHandler creates an http.Handler for the given resource type and operation method. | |||
func CreateHandler(ctx context.Context, resourceType string, operationMethod v1.OperationMethod, opts ctrl.Options, factory ControllerFactoryFunc) (http.HandlerFunc, error) { | |||
storageClient, err := opts.DataProvider.GetStorageClient(ctx, resourceType) |
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 needed to extract this logic into its own function to use with UCP routing.
@@ -24,8 +24,8 @@ import ( | |||
const ( | |||
// OperationProcess is the operation type for processing a tracked resource. | |||
OperationProcess = "PROCESS" | |||
// ResourceType is the resource type for a generic resource. | |||
ResourceType = "System.Resources/resources" |
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.
This was a naming conflict with a bunch of other things.
func (r *GetResourceProviderSummary) Run(ctx context.Context, w http.ResponseWriter, req *http.Request) (armrpc_rest.Response, error) { | ||
relativePath := middleware.GetRelativePath(r.Options().PathBase, req.URL.Path) | ||
|
||
scope, name, err := r.extractScopeAndName(relativePath) |
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.
This is a special case where the request URL isn't a resource ID. This is why I had to write bespoke controllers for the summary.
resourceGroupResourceRouter := server.NewSubrouter(baseRouter, resourceGroupResourcePath, apiValidator) | ||
|
||
handlerOptions := []server.HandlerOptions{ | ||
{ |
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.
The existing way of registering handlers works decently well for resource providers. UCP has a bunch of specialized functionality which conflicts.
It's always been a pain to work on this code in UCP, and I finally had a need to rewrite it. I think the result is much more maintainable.
address := "http://" + server.Listener.Addr().String() | ||
connection, err := sdk.NewDirectConnection(address + pathBase) | ||
require.NoError(t, err) | ||
|
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.
Needed to reorder some things here to get the URL of the server. The backend controllers call back into UCP to initiate a cascading delete, so the URL is required.
if !parsed.IsAbs() { | ||
requestUrl = ts.BaseURL + pathAndQuery | ||
} | ||
|
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.
This was needed to work with async operations.
@@ -468,6 +485,91 @@ func (tr *TestResponse) EqualsResponse(statusCode int, body []byte) { | |||
require.Equal(tr.t, statusCode, tr.Raw.StatusCode, "status code did not match expected") | |||
} | |||
|
|||
// EqualsValue compares a TestResponse against an expected status code and an response body. | |||
// | |||
// If the systemData propert is present in the response, it will be removed. |
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.
Added some new helpers for testing.
This change implements the API functionality for registering resource providers and related child-types through the Radius API. This is the first step towards making the Radius API surface extensible. For this first step there is no consumer of this data or these APIs. Subsequent changes will consume the data and implement the user-facing functionality. What's possible in this commit: - CRUDL operations on resource provider and related types like: - resource type - api version (child of resource type) - location Additionally, this change implements the resource provider "summary" API. The summary API provides a combined view of the most useful data from each resource provider and its children. --- For reviewers there are a few important notes: - Routing in UCP is complex, and error prone to work on. I abandoned the existing code and rewrote it in a form that's more native to go-chi. This was a significant improvement and made it much easier to debug the code. UCP has good integration tests so I'm not very concerned about regressions. - This is our first use-case for child resources in Radius. I used the async controllers to implement cascading deletion. - The "summary" API uses cached data. The async controllers for resource provider and other types update the cache. This is our first use of this pattern, but it felt right to me. Signed-off-by: Ryan Nowak <nowakra@gmail.com>
e4547fb
to
9f6be61
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
} | ||
|
||
@doc("The configuration of a resource provider in a specific location.") | ||
model ResourceProviderSummaryLocation {} |
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.
What data will this eventually have? (or will it contain any specific 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.
Whatever data we decide is relevant in the future. Right now this field indicates the existence of a location.
// ResourceProviderSummaryIDFromParts returns the resource id for a resource provider summary. | ||
// | ||
// Since ResourceProviderSummary is a virtual resource, the resource id is different from the URL used to access it. | ||
func ResourceProviderSummaryIDFromParts(scope string, name string) (resources.ID, error) { |
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.
Might have missed this in the design but why are the resource ID and the URL different? Why can't we query from /planes/radius/local/providers/System.Resources/resourceProviders/Contoso.Example
as opposed to /planes/radius/local/providers/Contoso.Example
?
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.
It's explained here:
// Since ResourceProviderSummary is a virtual resource, the resource id is different from the URL used to access it.
The resource provider summary isn't a "real" resource. But our database only stores resources.
Why can't we query from /planes/radius/local/providers/System.Resources/resourceProviders/Contoso.Example as opposed to /planes/radius/local/providers/Contoso.Example?
This is explained in the design document. They are optimized for different usecases.
/planes/radius/local/providers/System.Resources/resourceProviders/Contoso.Example
will get the resource provider resource. Most of the valuable fields are part of the child resources of this type./planes/radius/local/providers/Contoso.Example
returns a summary of the most valuable fields. Most clients (like therad
CLI) will only interact with the summary.
Description
This change implements the API functionality for registering resource providers and related child-types through the Radius API. This is the first step towards making the Radius API surface extensible. For this first step there is no consumer of this data or these APIs. Subsequent changes will consume the data and implement the user-facing functionality.
What's possible in this commit:
Additionally, this change implements the resource provider "summary" API. The summary API provides a combined view of the most useful data from each resource provider and its children.
For reviewers there are a few important notes:
Type of change
Part of: #6688