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

Fix connection direction #7076

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Fix connection direction #7076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a56ecf5
Fix connection direction
youngbupark Jan 25, 2024
4cff3b0
refactor functional test code
youngbupark Jan 25, 2024
7d4e653
clean up
youngbupark Jan 25, 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
11 changes: 5 additions & 6 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 @@ -295,7 +295,7 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
entry := applicationGraphResourcesByID[id]

for _, connection := range entry.Connections {
otherID := *connection.ID
otherID := to.String(connection.ID)
direction := connection.Direction

// For each connection let's make sure the destination is also part of the application graph. This handles
Expand Down Expand Up @@ -328,11 +328,10 @@ func computeGraph(applicationName string, applicationResources []generated.Gener
//id is the source from which the connections in connectionsBySource go out
if *direction == corerpv20231001preview.DirectionOutbound { // we are dealing with a relation formed by "connection"
connectionsBySource[id] = append(connectionsBySource[id], *connection)
dir := corerpv20231001preview.DirectionInbound
//otherID is the destination to the connections in connectionsByDestination
connectionInbound := corerpv20231001preview.ApplicationGraphConnection{
ID: &id,
Direction: &dir, //Direction is set with respect to Resource defining this connection
ID: to.Ptr(id),
Direction: to.Ptr(corerpv20231001preview.DirectionInbound), //Direction is set with respect to Resource defining this connection
Copy link
Author

Choose a reason for hiding this comment

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

Inbound connection should be Inbound not Outbound.

}
connectionsByDestination[otherID] = append(connectionsByDestination[otherID], connectionInbound)
} else {
Expand Down Expand Up @@ -493,7 +492,7 @@ 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.DirectionInbound
dir := corerpv20231001preview.DirectionOutbound
Copy link
Author

Choose a reason for hiding this comment

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

This is the root cause of this bug.

data := corerpv20231001preview.ConnectionProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
Expand Down Expand Up @@ -582,7 +581,7 @@ func providesFromAPIData(resource generated.GenericResource) []*corerpv20231001p
// If we encounter an error processing this data, just skip "invalid" connection entry.
entries := []*corerpv20231001preview.ApplicationGraphConnection{}
for _, connection := range connections {
dir := corerpv20231001preview.DirectionOutbound
dir := corerpv20231001preview.DirectionInbound
data := corerpv20231001preview.ContainerPortProperties{}
err := toStronglyTypedData(connection, &data)
if err == nil {
Expand Down
7 changes: 7 additions & 0 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 @@ -140,6 +140,13 @@ func Test_computeGraph(t *testing.T) {
envResourceDataFile: "",
expectedDataFile: "graph-app-httproute-out.json",
},
{
name: "using httproute 2",
Copy link
Author

Choose a reason for hiding this comment

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

I am going to change test case name to more meaningful one later.

applicationName: "myapp",
appResourceDataFile: "graph-app-httproute2-in.json",
envResourceDataFile: "",
expectedDataFile: "graph-app-httproute2-out.json",
},
{
name: "direct route",
applicationName: "myapp",
Expand Down
9 changes: 7 additions & 2 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-directroute-out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
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 carefully review these test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is right.

"connections": [
{
"direction": "Inbound",
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp"
}
],
Expand All @@ -13,7 +13,12 @@
"type": "Applications.Core/containers"
},
{
"connections": [],
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/frontend"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/backendapp",
"name": "backendapp",
"outputResources": [],
Expand Down
19 changes: 10 additions & 9 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute-out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
"connections": [
{
"direction": "Outbound",
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/sql-rte"
}
],
Expand All @@ -13,18 +13,19 @@
"type": "Applications.Core/containers"
},
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/sql-app-ctnr"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Datastores/sqlDatabases/sql-db",
"name": "sql-db",
"provisioningState": "Succeeded",
"type": "Applications.Datastores/sqlDatabases"
},
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/sql-ctnr"
}
],
"connections": [],
Copy link
Author

@youngbupark youngbupark Jan 25, 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 correct because no other container resources refers to http route.

Copy link
Contributor

@rynowak rynowak Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really going to comment on the correctness of the HTTP route scenario. It would take extra work specific to HTTP Routes to get a useful output for this case.

We shouldn't fix it, we should complete the removal of the HTTP Route type.

Copy link
Author

Choose a reason for hiding this comment

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

If we want to make it correct, then httproute should have outbound and inbound both.

"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/sql-rte",
"name": "sql-rte",
"outputResources": [],
Expand All @@ -34,7 +35,7 @@
{
"connections": [
{
"direction": "Inbound",
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Datastores/sqlDatabases/sql-db"
}
],
Expand All @@ -44,4 +45,4 @@
"provisioningState": "Succeeded",
"type": "Applications.Core/containers"
}
]
]
72 changes: 72 additions & 0 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute2-in.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
[
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1",
"name": "http-back-rte-simple1",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"provisioningState": "Succeeded"
},
"type": "Applications.Core/httpRoutes"
},
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/http-front-ctnr-simple1",
"name": "http-front-ctnr-simple1",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"containers": {
"image": "magpie:latest",
"ports": {
"web": {
"port": 8080
}
},
"readinessProbe": {
"kind": "httpGet",
"path": "/healthz",
"containerPort": 8080
}
},
"connections": {
"backend": {
"source": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1"
}
},
"provisioningState": "Succeeded",
"status": {
"outputResources": {
"id": "/some/thing/else",
"localId": "something"
}
}
},
"type": "Applications.Core/containers"
},
{
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/http-back-ctnr-simple1",
"name": "http-back-ctnr-simple1",
"properties": {
"application": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/Applications/myapp",
"container": {
"ports": {
"web": {
"port": 8080,
"provides": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1"
}
},
"readinessProbe": {
"kind": "httpGet",
"path": "/healthz",
"containerPort": 8080
}
},
"provisioningState": "Succeeded",
"status": {
"outputResources": {
"id": "/some/thing/else",
"localId": "something"
}
}
},
"type": "Applications.Core/containers"
}
]
41 changes: 41 additions & 0 deletions pkg/corerp/frontend/controller/applications/testdata/graph-app-httproute2-out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
[
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/http-front-ctnr-simple1"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1",
"name": "http-back-rte-simple1",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/httpRoutes"
},
{
"connections": [
{
"direction": "Outbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/http-front-ctnr-simple1",
"name": "http-front-ctnr-simple1",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/containers"
},
{
"connections": [
{
"direction": "Inbound",
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/httpRoutes/http-back-rte-simple1"
}
],
"id": "/planes/radius/local/resourcegroups/default/providers/Applications.Core/containers/http-back-ctnr-simple1",
"name": "http-back-ctnr-simple1",
"outputResources": [],
"provisioningState": "Succeeded",
"type": "Applications.Core/containers"
}
]
Loading
Loading