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

Jump to bottom

Support AppGraph connection for direct route #7063

Closed
wants to merge 13 commits into from
Closed

Support AppGraph connection for direct route #7063

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
590e100
Support direct route
youngbupark Jan 20, 2024
147fae9
Merge branch 'main' into youngp/fix-directroute
youngbupark Jan 23, 2024
a3370d0
Merge branch 'main' into youngp/fix-directroute
youngbupark Jan 24, 2024
42ffbc7
wip
youngbupark Jan 24, 2024
becaecc
fix
youngbupark Jan 24, 2024
985110f
fix cr
youngbupark Jan 24, 2024
7beb51d
Merge branch 'main' into youngp/fix-directroute
youngbupark Jan 24, 2024
f601cb5
Fix CR
youngbupark Jan 24, 2024
7e45ef1
Fix testdata
youngbupark Jan 24, 2024
4dbcefc
test
youngbupark Jan 24, 2024
f43ef05
wip
youngbupark Jan 24, 2024
82a3b5f
wip
youngbupark Jan 24, 2024
431c5a7
wip
youngbupark Jan 24, 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
94 changes: 59 additions & 35 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 @@ -19,6 +19,7 @@ package applications
import (
"context"
"encoding/json"
"net/url"
"sort"
"strings"

Expand All @@ -29,6 +30,7 @@ import (
aztoken "github.com/radius-project/radius/pkg/azure/tokencredentials"
"github.com/radius-project/radius/pkg/cli/clients_new/generated"
corerpv20231001preview "github.com/radius-project/radius/pkg/corerp/api/v20231001preview"
"github.com/radius-project/radius/pkg/to"
"github.com/radius-project/radius/pkg/ucp/resources"
)

Expand Down Expand Up @@ -208,11 +210,11 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
resources = append(resources, resource)

// Application-scoped resources are by definition "in" the application
resourcesByIDInApplication[*resource.ID] = true
resourcesByIDInApplication[to.String(resource.ID)] = true
youngbupark marked this conversation as resolved.
Show resolved Hide resolved
}

for _, resource := range environmentResources {
_, found := resourcesByIDInApplication[*resource.ID]
_, found := resourcesByIDInApplication[to.String(resource.ID)]
if found {
// Appears in both application and environment lists, avoid duplicates.
continue
Expand All @@ -221,7 +223,7 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
// This is an environment-scoped resource. We need to process the connections
// to determine if it's part of the application.
resources = append(resources, resource)
resourcesByIDInApplication[*resource.ID] = false
resourcesByIDInApplication[to.String(resource.ID)] = false
}

// Next we need to process each entry in the resources list and build up the application graph.
Expand All @@ -244,7 +246,7 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
connections = append(connections, providesFromAPIData(resource)...) // Inbound connections based on 'provides'

sort.Slice(connections, func(i, j int) bool {
return *connections[i].ID < *connections[j].ID
return to.String(connections[i].ID) < to.String(connections[j].ID)
})

applicationGraphResource.Connections = connections
Expand Down Expand Up @@ -275,8 +277,8 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
// First process add resources we *know* are in the application to the queue. As we explore the graph we'll
// visit resources outside the application if necessary.
for _, entry := range applicationGraphResourcesByID {
if resourcesByIDInApplication[*entry.ID] {
queue = append(queue, *entry.ID)
if resourcesByIDInApplication[to.String(entry.ID)] {
queue = append(queue, to.String(entry.ID))
}
}

Expand Down Expand Up @@ -357,7 +359,7 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
// Print connections in stable order.
sort.Slice(entry.Connections, func(i, j int) bool {
// Connections are guaranteed to have a name.
return *entry.Connections[i].ID < *entry.Connections[j].ID
return to.String(entry.Connections[i].ID) < to.String(entry.Connections[j].ID)
})

graph.Resources = append(graph.Resources, &entry)
Expand All @@ -372,29 +374,21 @@ func applicationGraphResourceFromID(id string) *corerpv20231001preview.Applicati
return nil // Invalid resource ID, skip
}

appName := application.Name()
appType := application.Type()
provisioningState := string(v1.ProvisioningStateSucceeded)

return &corerpv20231001preview.ApplicationGraphResource{ID: &id,
Name: &appName,
Type: &appType,
ProvisioningState: &provisioningState,
return &corerpv20231001preview.ApplicationGraphResource{
Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

ID: to.Ptr(id),
youngbupark marked this conversation as resolved.
Show resolved Hide resolved
Name: to.Ptr(application.Name()),
Type: to.Ptr(application.Type()),
ProvisioningState: to.Ptr(string(v1.ProvisioningStateSucceeded)),
}

}

// outputResourceEntryFromID creates a outputResourceEntry from a resource ID.
func outputResourceEntryFromID(id resources.ID) corerpv20231001preview.ApplicationGraphOutputResource {
orID := id.String()
orName := id.Name()
orType := id.Type()
entry := corerpv20231001preview.ApplicationGraphOutputResource{ID: &orID,
Name: &orName,
Type: &orType,
return corerpv20231001preview.ApplicationGraphOutputResource{
ID: to.Ptr(id.String()),
Name: to.Ptr(id.Name()),
Type: to.Ptr(id.Type()),
}

return entry
}

// outputResourcesFromAPIData processes the generic resource representation returned by the Radius API
Expand Down Expand Up @@ -447,13 +441,13 @@ func outputResourcesFromAPIData(resource generated.GenericResource) []*corerpv20

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

})

Expand Down Expand Up @@ -497,21 +491,50 @@ func connectionsFromAPIData(resource generated.GenericResource) []*corerpv202310
data := corerpv20231001preview.ConnectionProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
sourceID, ok := parseSource(to.String(resource.ID), to.String(data.Source))
if !ok {
continue
}

entries = append(entries, &corerpv20231001preview.ApplicationGraphConnection{
ID: data.Source,
Direction: &dir,
ID: to.Ptr(sourceID),
Direction: to.Ptr(dir),
})
}
}

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

return entries
}

func parseSource(parentID, source string) (string, bool) {
youngbupark marked this conversation as resolved.
Show resolved Hide resolved
id, err := resources.Parse(source)
if err == nil && id.IsResource() {
return id.String(), true
}

sourceURL, err := url.Parse(source)
if err != nil {
return "", false
}

if sourceURL.Hostname() == "" || strings.Contains(sourceURL.Hostname(), ".") {
youngbupark marked this conversation as resolved.
Show resolved Hide resolved
return "", false
}

parentIDParsed, err := resources.Parse(parentID)
if err != nil {
return "", false
}

sourceID := strings.Join([]string{parentIDParsed.RootScope(), resources.ProvidersSegment, parentIDParsed.Type(), sourceURL.Hostname()}, resources.SegmentSeparator)
return sourceID, true
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what if I have a connection to something outside of Radius? eg: http://example.com.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I found the bug. The following direction must be OutBound.

radius/pkg/corerp/frontend/controller/applications/graph_util.go

Line 496 in fc4594e

dir := corerpv20231001preview.DirectionInbound

@nithyatsu @rynowak Please review the new testdata to see if each resource connection has the right direction.

}

// 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.
Expand Down Expand Up @@ -550,19 +573,20 @@ func providesFromAPIData(resource generated.GenericResource) []*corerpv20231001p
data := corerpv20231001preview.ContainerPortProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
if *data.Provides == "" {
if to.String(data.Provides) == "" {
continue
}

entries = append(entries, &corerpv20231001preview.ApplicationGraphConnection{
ID: data.Provides,
Direction: &dir,
Direction: to.Ptr(dir),
})
}
}

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

return entries
Expand Down
Loading
Loading