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

youngbupark · 2024-01-26T04:31:02Z

Description

This is to support routes.*.destination property for App graph. It enables to show the connections between Gateway and the other resources in App Graph

Type of change

This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

Fixes: #7038

Signed-off-by: Young Bu Park <[email protected]>

youngbupark · 2024-01-26T04:36:36Z

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

-// 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 {


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

youngbupark · 2024-01-26T04:37:51Z

pkg/corerp/frontend/controller/applications/testdata/graph-app-gw-out.json

@@ -0,0 +1,53 @@
+[


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

Looks right to me 👍

youngbupark · 2024-01-26T04:40:22Z

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

@@ -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 {


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

Great catch!

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.

radius-functional-tests · 2024-01-26T04:41:57Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	youngbupark/radius
Commit ref	`c940f16`
Unique ID	81a82df449
Image tag	pr-81a82df449

Click here to see the list of tools in the current test run

gotestsum 1.10.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/functional/shared/recipes/<name>:pr-81a82df449
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-81a82df449
controller test image location: ghcr.io/radius-project/dev/controller:pr-81a82df449
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-81a82df449
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting daprrp functional tests...
⌛ Starting msgrp functional tests...
⌛ Starting shared functional tests...
⌛ Starting samples functional tests...
⌛ Starting datastoresrp functional tests...
⌛ Starting kubernetes functional tests...
⌛ Starting ucp functional tests...
✅ datastoresrp functional tests succeeded
✅ msgrp functional tests succeeded
✅ samples functional tests succeeded
✅ daprrp functional tests succeeded
✅ kubernetes functional tests succeeded
✅ ucp functional tests succeeded
✅ shared functional tests succeeded

ytimocin · 2024-01-26T16:48:42Z

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

@@ -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 {


Great catch!

ytimocin · 2024-01-26T16:50:19Z

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

-		// 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) {


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

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.

Signed-off-by: Young Bu Park <[email protected]>

radius-functional-tests · 2024-01-26T17:12:10Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	youngbupark/radius
Commit ref	`15b2478`
Unique ID	2364f63704
Image tag	pr-2364f63704

Click here to see the list of tools in the current test run

gotestsum 1.10.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/functional/shared/recipes/<name>:pr-2364f63704
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-2364f63704
controller test image location: ghcr.io/radius-project/dev/controller:pr-2364f63704
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-2364f63704
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting shared functional tests...
⌛ Starting msgrp functional tests...
⌛ Starting samples functional tests...
⌛ Starting datastoresrp functional tests...
⌛ Starting kubernetes functional tests...
⌛ Starting ucp functional tests...
⌛ Starting daprrp functional tests...
✅ datastoresrp functional tests succeeded
✅ msgrp functional tests succeeded
✅ kubernetes functional tests succeeded
✅ samples functional tests succeeded
✅ ucp functional tests succeeded
✅ daprrp functional tests succeeded
✅ shared functional tests succeeded

rynowak · 2024-01-26T17:37:51Z

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

+// 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 {


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.

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

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.

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.

rynowak · 2024-01-26T18:00:03Z

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

-		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))


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.

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.

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

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

…#7079) # Description This is to support `routes.*.destination` property for App graph. It enables to show the connections between Gateway and the other resources in App Graph ## Type of change - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Fixes: radius-project#7038 --------- Signed-off-by: Young Bu Park <[email protected]>

…#7079) # Description This is to support `routes.*.destination` property for App graph. It enables to show the connections between Gateway and the other resources in App Graph ## Type of change - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Fixes: radius-project#7038 --------- Signed-off-by: Young Bu Park <[email protected]> Signed-off-by: sk593 <[email protected]>

…#7079) # Description This is to support `routes.*.destination` property for App graph. It enables to show the connections between Gateway and the other resources in App Graph ## Type of change - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Fixes: radius-project#7038 --------- Signed-off-by: Young Bu Park <[email protected]>

youngbupark added 3 commits January 25, 2024 16:36

wip

63b8f8b

Signed-off-by: Young Bu Park <[email protected]>

support gw

afbd67f

Signed-off-by: Young Bu Park <[email protected]>

support gw

0acfc19

Signed-off-by: Young Bu Park <[email protected]>

youngbupark requested review from a team as code owners January 26, 2024 04:31

youngbupark added 2 commits January 25, 2024 20:33

clean up

9e9d0de

Signed-off-by: Young Bu Park <[email protected]>

Add comment

c940f16

Signed-off-by: Young Bu Park <[email protected]>

youngbupark commented Jan 26, 2024

View reviewed changes

youngbupark temporarily deployed to functional-tests January 26, 2024 04:41 — with GitHub Actions Inactive

ytimocin previously approved these changes Jan 26, 2024

View reviewed changes

more clean up

15b2478

Signed-off-by: Young Bu Park <[email protected]>

youngbupark dismissed ytimocin’s stale review via 15b2478 January 26, 2024 17:08

youngbupark temporarily deployed to functional-tests January 26, 2024 17:11 — with GitHub Actions Inactive

rynowak reviewed Jan 26, 2024

View reviewed changes

rynowak approved these changes Jan 26, 2024

View reviewed changes

youngbupark merged commit c262df7 into radius-project:main Jan 26, 2024
16 checks passed

youngbupark deleted the youngp/support-gateway branch January 26, 2024 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

youngbupark commented Jan 26, 2024

youngbupark Jan 26, 2024 •

edited

Loading

youngbupark Jan 26, 2024

rynowak Jan 26, 2024

youngbupark Jan 26, 2024 •

edited

Loading

ytimocin Jan 26, 2024

rynowak Jan 26, 2024

radius-functional-tests bot commented Jan 26, 2024 •

edited

Loading

ytimocin Jan 26, 2024

ytimocin Jan 26, 2024

youngbupark Jan 26, 2024 •

edited

Loading

radius-functional-tests bot commented Jan 26, 2024 •

edited

Loading

rynowak Jan 26, 2024

rynowak Jan 26, 2024

youngbupark Jan 26, 2024 •

edited

Loading

rynowak Jan 26, 2024

rynowak Jan 26, 2024 •

edited

Loading

rynowak Jan 26, 2024

youngbupark Jan 26, 2024

rynowak Jan 26, 2024

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

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

Conversation

youngbupark commented Jan 26, 2024

Description

Type of change

youngbupark Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radius-functional-tests bot commented Jan 26, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

radius-functional-tests bot commented Jan 26, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark Jan 26, 2024 •

edited

Loading

youngbupark Jan 26, 2024 •

edited

Loading

radius-functional-tests bot commented Jan 26, 2024 •

edited

Loading

youngbupark Jan 26, 2024 •

edited

Loading

radius-functional-tests bot commented Jan 26, 2024 •

edited

Loading

youngbupark Jan 26, 2024 •

edited

Loading

rynowak Jan 26, 2024 •

edited

Loading