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

Support routes.*.destination of gateway for App Graph #7079

Merged
merged 6 commits into from
Jan 26, 2024
Merged

Support routes.*.destination of gateway for App Graph #7079

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
63b8f8b
wip
youngbupark Jan 26, 2024
afbd67f
support gw
youngbupark Jan 25, 2024
0acfc19
support gw
youngbupark Jan 26, 2024
9e9d0de
clean up
youngbupark Jan 26, 2024
c940f16
Add comment
youngbupark Jan 26, 2024
15b2478
more clean up
youngbupark Jan 26, 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
140 changes: 72 additions & 68 deletions pkg/corerp/frontend/controller/applications/graph_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ var (

const (
connectionsPath = "/properties/connections"
routesPath = "/properties/routes"
portsPath = "/properties/container/ports"
)

Expand Down Expand Up @@ -248,8 +249,12 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
applicationGraphResource.ProvisioningState = &state
}

connections := connectionsFromAPIData(resource, resources) // Outbound connections based on 'connections'
connections = append(connections, providesFromAPIData(resource)...) // Inbound connections based on 'provides'
// Resolve Outbound connections based on 'connections'.
connections := resolveConnections(resource, connectionsPath, connectionsConverter(resources))
// Resolve Outbound connections based on 'routes'.
connections = append(connections, resolveConnections(resource, routesPath, routesPathConverter(resources))...)
// Resolve Inbound connections based on 'provides'.
connections = append(connections, resolveConnections(resource, portsPath, providersConverter)...)

sort.Slice(connections, func(i, j int) bool {
return to.String(connections[i].ID) < to.String(connections[j].ID)
Expand Down Expand Up @@ -356,8 +361,8 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
connectionsIn := connectionsByDestination[id]

entryConnections := make([]*corerpv20231001preview.ApplicationGraphConnection, len(connectionsIn))
for i, conn := range connectionsIn {
Copy link
Author

@youngbupark youngbupark Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the code bug. &conn refered to the same variable so that entryConnections array held the same pointer values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh.

The good news is that they are fixing this: golang/go#60078

I wasn't able to find the link, but we can use the Go compiler to detect these cases.

entryConnections[i] = &conn
for i := range connectionsIn {
entryConnections[i] = &connectionsIn[i]
}
entry.Connections = append(entry.Connections, entryConnections...)

Expand Down Expand Up @@ -459,16 +464,10 @@ func outputResourcesFromAPIData(resource generated.GenericResource) []*corerpv20
return entries
}

// connectionsFromAPIData resolves the outbound connections for a resource from the generic resource representation.
// For example if the resource is an 'Applications.Core/container' then this function can find it's connections
// to other resources like databases. Conversely if the resource is a database then this function
// will not find any connections (because they are inbound). Inbound connections are processed later.
func connectionsFromAPIData(resource generated.GenericResource, allResources []generated.GenericResource) []*corerpv20231001preview.ApplicationGraphConnection {
func resolveConnections(resource generated.GenericResource, jsonRefPath string, converter func(any) (string, corerpv20231001preview.Direction, error)) []*corerpv20231001preview.ApplicationGraphConnection {
Copy link
Author

@youngbupark youngbupark Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I refactored the code to simplify the code for resolving connections, routes, and provides instead of repeating the same code multiple times.

// We need to access the connections in a weakly-typed way since the data type we're
// working with is a property bag.
//
// Any Radius resource type that supports connections uses the following property path to return them.
p, err := jsonpointer.New(connectionsPath)
p, err := jsonpointer.New(jsonRefPath)
if err != nil {
// This should never fail since we're hard-coding the path.
panic("parsing JSON pointer should not fail: " + err.Error())
Expand All @@ -480,8 +479,17 @@ func connectionsFromAPIData(resource generated.GenericResource, allResources []g
return []*corerpv20231001preview.ApplicationGraphConnection{}
}

connections, ok := raw.(map[string]any)
if !ok {
items := []any{}
switch conn := raw.(type) {
case []any:
items = conn
case map[string]any:
for _, v := range conn {
items = append(items, v)
}
}

if len(items) == 0 {
// Not a map of objects, this is fine.
return []*corerpv20231001preview.ApplicationGraphConnection{}
}
Expand All @@ -491,13 +499,9 @@ func connectionsFromAPIData(resource generated.GenericResource, allResources []g
//
// If we encounter an error processing this data, just skip "invalid" connection entry.
entries := []*corerpv20231001preview.ApplicationGraphConnection{}
for _, connection := range connections {
dir := corerpv20231001preview.DirectionOutbound
data := corerpv20231001preview.ConnectionProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
sourceID, _ := findSourceResource(to.String(data.Source), allResources)

for _, item := range items {
sourceID, dir, err := converter(item)
if err == nil && sourceID != "" {
entries = append(entries, &corerpv20231001preview.ApplicationGraphConnection{
ID: to.Ptr(sourceID),
Direction: to.Ptr(dir),
Expand Down Expand Up @@ -547,61 +551,61 @@ func findSourceResource(source string, allResources []generated.GenericResource)
return orig, ErrInvalidSource
}

// providesFromAPIData is specifically to support HTTPRoute.
func providesFromAPIData(resource generated.GenericResource) []*corerpv20231001preview.ApplicationGraphConnection {
// Any Radius resource type that exposes a port uses the following property path to return them.
// The port may have a 'provides' attribute that specifies a httproute.
// This route should be parsed to find the connections between containers.
// For example, if container A provides a route and container B consumes it,
// then we have port.provides in container A and container.connection in container B.
// This gives us the connection: container A --> route R --> container B.
// Without parsing the 'provides' attribute, we would miss the connection between container A and route R.

p, err := jsonpointer.New(portsPath)
if err != nil {
// This should never fail since we're hard-coding the path.
panic("parsing JSON pointer should not fail: " + err.Error())
// connectionsConverter resolves the outbound connections for a resource from the generic resource representation.
// For example if the resource is an 'Applications.Core/container' then this function can find it's connections
// to other resources like databases. Conversely if the resource is a database then this function
// will not find any connections (because they are inbound). Inbound connections are processed later.
func connectionsConverter(resources []generated.GenericResource) func(any) (string, corerpv20231001preview.Direction, error) {
return func(item any) (string, corerpv20231001preview.Direction, error) {
data := &corerpv20231001preview.ConnectionProperties{}
err := toStronglyTypedData(item, data)
if err != nil {
return "", "", err
}
sourceID, err := findSourceResource(to.String(data.Source), resources)
if err != nil {
return "", "", err
}
return sourceID, corerpv20231001preview.DirectionOutbound, nil
}
}

raw, _, err := p.Get(&resource)
if err != nil {
// Not found, this is fine.
return []*corerpv20231001preview.ApplicationGraphConnection{}
func routesPathConverter(resources []generated.GenericResource) func(any) (string, corerpv20231001preview.Direction, error) {
return func(item any) (string, corerpv20231001preview.Direction, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have one function for all three and just pass in the resource type?

Copy link
Author

@youngbupark youngbupark Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can't we have one function for all three and just pass in the resource type?

That way makes more complicated and each one has different logic for scanning resource. it is not worth.

data := &corerpv20231001preview.GatewayRoute{}
err := toStronglyTypedData(item, data)
if err != nil {
return "", "", err
}
sourceID, err := findSourceResource(to.String(data.Destination), resources)
if err != nil {
return "", "", err
}
return sourceID, corerpv20231001preview.DirectionOutbound, nil
}
}

connections, ok := raw.(map[string]any)
if !ok {
// Not a map of objects, this is fine.
return []*corerpv20231001preview.ApplicationGraphConnection{}
// providersConverter is specifically to support HTTPRoute.
// Any Radius resource type that exposes a port uses the following property path to return them.
// The port may have a 'provides' attribute that specifies a httproute.
// This route should be parsed to find the connections between containers.
// For example, if container A provides a route and container B consumes it,
// then we have port.provides in container A and container.connection in container B.
// This gives us the connection: container A --> route R --> container B.
// Without parsing the 'provides' attribute, we would miss the connection between container A and route R.
func providersConverter(item any) (string, corerpv20231001preview.Direction, error) {
data := &corerpv20231001preview.ContainerPortProperties{}
err := toStronglyTypedData(item, data)
if err != nil {
return "", "", err
}

// The data is returned as a map of JSON objects. We need to convert each object from a map[string]any
// to the strongly-typed format we understand.
//
// If we encounter an error processing this data, just skip "invalid" connection entry.
entries := []*corerpv20231001preview.ApplicationGraphConnection{}
for _, connection := range connections {
dir := corerpv20231001preview.DirectionInbound
data := corerpv20231001preview.ContainerPortProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
if to.String(data.Provides) == "" {
continue
}

entries = append(entries, &corerpv20231001preview.ApplicationGraphConnection{
ID: data.Provides,
Direction: to.Ptr(dir),
})
}
id := to.String(data.Provides)
if id == "" {
return "", "", nil
}

// Produce a stable output
sort.Slice(entries, func(i, j int) bool {
return to.String(entries[i].ID) < to.String(entries[j].ID)
})

return entries
return id, corerpv20231001preview.DirectionInbound, nil
}

// toStronglyTypedData uses JSON marshalling and unmarshalling to convert a weakly-typed
Expand Down
11 changes: 9 additions & 2 deletions pkg/corerp/frontend/controller/applications/graph_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,14 @@ func Test_computeGraph(t *testing.T) {
expectedDataFile string
}{
{
name: "using httproute",
name: "using httproute without inbound resource",
applicationName: "myapp",
appResourceDataFile: "graph-app-httproute-in.json",
envResourceDataFile: "",
expectedDataFile: "graph-app-httproute-out.json",
},
{
name: "using httproute 2",
name: "using httproute with inbound resource",
applicationName: "myapp",
appResourceDataFile: "graph-app-httproute2-in.json",
envResourceDataFile: "",
Expand All @@ -154,6 +154,13 @@ func Test_computeGraph(t *testing.T) {
envResourceDataFile: "",
expectedDataFile: "graph-app-directroute-out.json",
},
{
name: "with gateway route",
applicationName: "myapp",
appResourceDataFile: "graph-app-gw-in.json",
envResourceDataFile: "",
expectedDataFile: "graph-app-gw-out.json",
},
}

for _, tt := range tests {
Expand Down
77 changes: 77 additions & 0 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-gw-in.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
[
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/gateways/httpgw",
"name": "httpgw",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"routes": [
{
"path": "/",
"destination": "http://frontend:8080"
},
{
"path": "/backendapi",
"destination": "http://backendapp:8080"
}
]
},
"type": "Applications.Core/containers"
},
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/frontend",
"name": "frontend",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"container": {
"image": "magpie:latest",
"readinessProbe": {
"kind": "httpGet",
"path": "/healthz",
"containerPort": 8080
},
"ports": {
"web": {
"port": 8080,
"protocol": "TCP"
}
}
},
"connections": {
"sql": {
"source": "http://backendapp:8080"
}
},
"provisioningState": "Succeeded",
"status": {
"outputResources": {
"id": "/some/thing/else",
"localId": "something"
}
}
},
"type": "Applications.Core/containers"
},
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp",
"name": "backendapp",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"container": {
"ports": {
"web": {
"port": 8080,
"protocol": "TCP"
}
}
},
"provisioningState": "Succeeded",
"status": {
"outputResources": {
"id": "/some/thing/else",
"localId": "something"
}
}
},
"type": "Applications.Core/containers"
}
]
53 changes: 53 additions & 0 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-gw-out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
[
Copy link
Author

Choose a reason for hiding this comment

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

@nithyatsu @rynowak Please review this appgraph output carefully. To me, the result makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks right to me 👍

{
"connections": [
{
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp"
},
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/gateways/httpgw"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/frontend",
"name": "frontend",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/containers"
},
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/frontend"
},
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/gateways/httpgw"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp",
"name": "backendapp",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/containers"
},
{
"connections": [
{
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp"
},
{
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/frontend"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/gateways/httpgw",
"name": "httpgw",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/gateways"
}
]
Loading