Support AppGraph connection for direct route #7063
Conversation
Signed-off-by: Young Bu Park <youngp@microsoft.com>
pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute-in.json
Show resolved
Hide resolved
pkg/corerp/frontend/controller/applications/testdata/graph-app-directroute-in.json
Show resolved
Hide resolved
| Name: &appName, | ||
| Type: &appType, | ||
| ProvisioningState: &provisioningState, | ||
| return &corerpv20231001preview.ApplicationGraphResource{ |
There was a problem hiding this comment.
I understand that input resource can be version-specific. Ideally, the appgraph's response needs to be version agnostic datamodel. @nithyatsu is there any reason why we use the version-specific response here ?
There was a problem hiding this comment.
Ideally the app graph API would follow the same versioning principles we have for other APIs 👍
I think we just didn't think about versioning since it was the first one, but the idea makes sense.
pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute-in.json
Outdated
Show resolved
Hide resolved
youngbupark
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
| } | ||
|
|
||
| sourceID := strings.Join([]string{parentIDParsed.RootScope(), resources.ProvidersSegment, parentIDParsed.Type(), sourceURL.Hostname()}, resources.SegmentSeparator) | ||
| return sourceID, true |
There was a problem hiding this comment.
Does this assume that the resource being connected to is in the same scope? So if I use multiple resource groups this will fall a part?
There was a problem hiding this comment.
Also, what if I have a connection to something outside of Radius? eg: http://example.com.
There was a problem hiding this comment.
I thought that we would support only <scheme>://<resourcename> format for connection, so the parser is designed for the case. To support the external endpoint, I would modify the connection format like below:
"connections": [
{
"direction": "Inbound | Outbound",
// "id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp"
"target": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp"
"type": "Resource | URL"
}
],instead of having id, we can have target and type (or something else) to define connection. Putting url on id field doesn't seem right.
cc/ @nithyatsu @rynowak
There was a problem hiding this comment.
Design doc for the features that connections support are here: #6375 @nithyatsu is the best resource for what parts of this have been implemented so far.
I don't think we discussed yet how an "external" URL should be represented in the app graph API, so I'm open to suggestions.
I want to make sure that we can represent a connection from container a -> container b and translate from URL -> resource ID in the app graph. The design for service-discovery was chosen to a support this.
There was a problem hiding this comment.
I found the bug. The following direction must be OutBound.
@nithyatsu @rynowak Please review the new testdata to see if each resource connection has the right direction.
pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute-out.json
Outdated
Show resolved
Hide resolved
| @@ -493,25 +487,60 @@ func connectionsFromAPIData(resource generated.GenericResource) []*corerpv202310 | |||
| // If we encounter an error processing this data, just skip "invalid" connection entry. | |||
| entries := []*corerpv20231001preview.ApplicationGraphConnection{} | |||
| for _, connection := range connections { | |||
| dir := corerpv20231001preview.DirectionInbound | |||
| dir := corerpv20231001preview.DirectionOutbound | |||
There was a problem hiding this comment.
@nithyatsu @rynowak I think this must be Outbound because this function resolves outbound connection.
There was a problem hiding this comment.
functional test is failing duplicated connections.
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Core/containers/http-front-ctnr-simple1"
},
// DUPE. this route should be backend container only.
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Core/containers/http-front-ctnr-simple1"
}
],
"id": "/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Core/httpRoutes/http-back-rte-simple1",
"name": "http-back-rte-simple1",
"outputResources": [
{
"id": "/planes/kubernetes/local/namespaces/default-corerp-application-simple1/providers/core/Service/http-back-rte-simple1",
"name": "http-back-rte-simple1",
"type": "core/Service"
}
],
"provisioningState": "Succeeded",
"type": "Applications.Core/httpRoutes"
}
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... |
Signed-off-by: Young Bu Park <youngp@microsoft.com>
youngbupark
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
youngbupark
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
|
closed by #7072 |
youngbupark
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Description
topackageType of change
Fixes: #7004 #6937