-
Notifications
You must be signed in to change notification settings - Fork 11
Add routes recipes #50
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.
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
base: main
Are you sure you want to change the base?
Conversation
92aa70e to
de46894
Compare
| # Define common configuration | ||
| setup_config() { | ||
| resource_folders=("Security") | ||
| # Define specific resource types to test instead of entire folders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating the workflow to allow testing to be added for individual resources instead of a namespace
Compute/routes/recipes/kubernetes/bicep/kubernetes-routes.bicep
Outdated
Show resolved
Hide resolved
26f04ab to
801d78a
Compare
| gateway_name = "gateway-${local.resource_id_hash}" | ||
| } | ||
|
|
||
| variable "context" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem right/necessary. context will be populated into tf module
we can set
variable "context" {
description = "This variable contains Radius recipe context."
type = any
}
| } | ||
| } | ||
|
|
||
| resource myContainer 'Radius.Compute/containers@2025-08-01-preview' = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this application template as an example of how the routes resource can be deployed. I'm not sure that we'd want this to be a test scenario for the repo since it's more of an integration/E2E test and it'd require the Gateway CRD to be installed on the test cluster (I set up the CRDs and necessary RBAC to deploy this when manually testing to verify that it'd work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to suggestions on if this should be included in this repo's test framework
| } | ||
|
|
||
| # Create Gateway for routing | ||
| resource "kubernetes_manifest" "gateway" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have a Gateway and a listener resource in the recipe, plus a reference to Contour. Which makes sense since there are pointers like Route → Gateway → Contour and listener. This is problematic since most clusters don't use Contour and we also don't know what listener the platform engineer wants/needs.
The way I imagined this is:
• Platform engineer installs a Gateway Controller like NGINX. As part of that, the platform engineer creates the Gateway resource that has the listener and the GatewayClass. NGINX actually creates this as part of the install.
• Then the platform engineer sets the parentRefs as a parameter on the Recipe.
The pros of this approach is we enforce separation of concerns. HTTPRoutes is the application part. Gateway and listener are platform engineering tasks. The con is that in order for a Routes resource to work, the Gateway Controller and the parameter must be set correct.
I recommend we remove the Gateway resource from the Recipe. Do you have other ideas about how this workflow should work between platform engineer and developers?
| namespace: context.runtime.kubernetes.namespace | ||
| } | ||
| ] | ||
| hostnames: length(hostnames) > 0 ? hostnames : ['localhost'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than defaulting to localhost, I think this should be not specified if it is not provided. From the docs:
If no hostname is specified, traffic is routed based on HTTPRoute rules and filters (optional).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| backendRefs: [ | ||
| { | ||
| name: '${toLower(last(split(rule.destinationContainer.resourceId, '/')))}-${rule.destinationContainer.containerName}' | ||
| port: 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs reference rule.destinationContainer.containerPortName. We cannot assume a port number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. We'll update the containerPortName variable to expect an integer so it can be passed as an accessible input variable
c60f4df to
20f2eb0
Compare
|
But it's not required in the recipe. Pls check if we need to update schema to align with recipe. |
|
@zachcasper , @sk593 perhaps this was discussed already but should include gRPC as another route_kind in the yaml and update the recipe for it? |
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
20f2eb0 to
00cacb0
Compare
I don't think this was discussed but we can add gRPC. thoughts @zachcasper ? |
|
Will this support WebSocket connections, including HTTPS-to-WSS protocol switching, especially in the context of route-based promotion capabilities? |
This PR adds Recipes for the Routes resource type. Since Routes need a container for the connection to work, this PR also adds a simple container Recipe