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 all 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
146 changes: 78 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,9 +42,13 @@ var (

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

// resolver is a function type to resolve appgraph connection.
type resolver func(any) (string, corerpv20231001preview.Direction, error)

// listAllResourcesByApplication takes a context, applicationID and clientOptions
// and returns a slice of GenericResources in the application and also an error if one occurs.
func listAllResourcesByApplication(ctx context.Context, applicationID resources.ID, clientOptions *policy.ClientOptions) ([]generated.GenericResource, error) {
Expand Down Expand Up @@ -248,8 +252,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, connectionsResolver(resources))
Copy link
Contributor

@rynowak rynowak 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.

Should we be checking the resource type here? It looks like all of these "resolvers" apply to all resource types. So if a resource has a property .properties.connections this code assumes it works like Applications.Core/containers, has the same schema, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is similar to the design I was talking about here: https://github.com/radius-project/radius/pull/7079/files#r1467935133 but without being driven by Capabilities.

Copy link
Author

Choose a reason for hiding this comment

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

Should we be checking the resource type here? It looks like all of these "resolvers" apply to all resource types. So if a resource has a property .properties.connections this code assumes it works like Applications.Core/containers, has the same schema, etc.

The original code didn't check the type. I was thinking that the original one might want to support multiple resource types which have the connection properties. So I didn't check the resource type as is. We can discuss it later when we work further for https://github.com/radius-project/radius/pull/7079/files#r1467935133

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with this for now 👍 we will revisit when we do extensibility.

// Resolve Outbound connections based on 'routes'.
connections = append(connections, resolveConnections(resource, routesPath, routesPathResolver(resources))...)
// Resolve Inbound connections based on 'provides'.
connections = append(connections, resolveConnections(resource, portsPath, providersResolver)...)

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 +364,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 +467,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 resolver) []*corerpv20231001preview.ApplicationGraphConnection {
// 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 +482,19 @@ func connectionsFromAPIData(resource generated.GenericResource, allResources []g
return []*corerpv20231001preview.ApplicationGraphConnection{}
}

connections, ok := raw.(map[string]any)
if !ok {
items := []any{}

// Convert array and map connection data to a slice of items.
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 +504,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 +556,62 @@ 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())
// connectionsResolver 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 connectionsResolver(resources []generated.GenericResource) resolver {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not relevant for this review, but long term I'd like to make this behavior consistent across APIs by using Capabilities + Well-Known API shapes, or Well-Known Child resources.

Consider how we'd make this extensible 👍 When users can define their own types, they should be able to support all of the features of the built-in ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically anyplace we write code that handles a specific resource type like Applications.Core/gateways, we might need to reconsider in the future.

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 intentionally didn't try to refactor it further to keep this change short. Instead of strongly typed one, I would try to use jsonpointer to look up the connections.destination. So, we can support multiple types. Also for custom resource, we can ask contributors to use the specific connection properties if they want to use appgraph. If you like this idea, I can make the change now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should change it now - we should discuss during the design of user-defined-types. I just wanted to mention the idea.

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{}
// routesPathResolver resolves the outbound connections of Applications.Core/gateway resource.
func routesPathResolver(resources []generated.GenericResource) resolver {
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{}
// providersResolver 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 providersResolver(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