feat: implement Gateway API routing class for DevWorkspaceRouting - #1680
feat: implement Gateway API routing class for DevWorkspaceRouting#1680btjd wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: btjd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds Gateway API routing through configurable Gateway references, generated ChangesGateway API routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DevWorkspaceRouting
participant GatewayAPISolver
participant HTTPRoute
participant EndpointResolver
DevWorkspaceRouting->>GatewayAPISolver: Generate routing objects
GatewayAPISolver->>HTTPRoute: Create redirect and backend routes
DevWorkspaceRouting->>HTTPRoute: Synchronize routes
DevWorkspaceRouting->>EndpointResolver: Resolve exposed endpoints
EndpointResolver-->>DevWorkspaceRouting: Return URLs and readiness
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
deploy/deployment/openshift/combined.yaml (1)
27952-27957: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider scoping
httproutesverbs instead of wildcard.Granting
'*'onhttproutesis broader than the create/update/delete/watch lifecycle described for HTTPRoutes in this PR. This matches the existing convention for other resources in this ClusterRole (e.g.,ingresses,routes), so it's a minor, optional tightening rather than a new problem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/openshift/combined.yaml` around lines 27952 - 27957, In the ClusterRole rule for the gateway.networking.k8s.io httproutes resource, replace the wildcard verb permission with only the required create, update, delete, and watch verbs, matching the lifecycle described and the existing ingresses/routes convention.controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go (2)
17-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup third-party imports separately from project-local imports.
metav1andgwapiv1(third-party/Kubernetes) are combined with thegithub.com/devfile/devworkspace-operator/...(project-local) group instead of forming their own group.As per coding guidelines: "Organize imports into three groups separated by blank lines: standard library, third-party/Kubernetes, and project-local imports."
♻️ Proposed import grouping
import ( "fmt" - controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" - "github.com/devfile/devworkspace-operator/pkg/constants" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/config" + "github.com/devfile/devworkspace-operator/pkg/constants" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go` around lines 17 - 26, Reorganize the import block in gateway API solver so standard-library imports, third-party/Kubernetes imports such as metav1 and gwapiv1, and project-local devworkspace-operator imports are separated by blank lines.Source: Coding guidelines
71-121: 🩺 Stability & Availability | 🔵 TrivialHandle the Gateway's cross-namespace route allowance.
Both generated HTTPRoutes set
ParentRefs[0].Namespaceto the Gateway namespace, while the route namespace comes fromworkspaceMeta.Namespace. That cross-namespace attachment only succeeds if the target Gateway listener’sallowedRoutes.namespaces.fromincludes the workspace namespace. Confirm the pre-provisioned Gateway is documented to require this configuration; otherwise workspaces outside the Gateway namespace will remain unattached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go` around lines 71 - 121, The generated routes in getHTTPRoutesForSpec rely on cross-namespace attachment from workspaceMeta.Namespace to gatewayNamespace. Confirm and document that the pre-provisioned Gateway listeners set allowedRoutes.namespaces.from to permit the workspace namespace; if this configuration is not guaranteed, update the Gateway provisioning/configuration so both createHTTPRedirectRoute and createHTTPSBackendRoute routes can attach successfully.controllers/controller/devworkspacerouting/devworkspacerouting_controller.go (1)
72-72: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWildcard verbs on the new
httproutesRBAC grant. The kubebuilder marker grantsverbs=*forgateway.networking.k8s.io/httproutes, which controller-gen materializes into the ClusterRole. The controller's actual HTTPRoute lifecycle (list/get/create/update/patch/delete/watch, persync_httproutes.go) doesn't need the full wildcard. This mirrors the pre-existing'*'convention already used foringresses/routesin the same ClusterRole, so it's a consistency-preserving but still-broad grant worth tightening for least privilege.
controllers/controller/devworkspacerouting/devworkspacerouting_controller.go#L72-L72: scope the kubebuilder marker to the verbs actually used, e.g.// +kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes,verbs=get;list;watch;create;update;patch;delete, then runmake generate_allto regenerate the RBAC manifests.deploy/deployment/kubernetes/combined.yaml#L27952-L27957: this generated entry will update automatically once the marker above is scoped down and manifests are regenerated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller.go` at line 72, Restrict the httproutes RBAC marker in devworkspacerouting_controller.go to get, list, watch, create, update, patch, and delete, then run make generate_all to regenerate deploy/deployment/kubernetes/combined.yaml; update the generated ClusterRole entry at lines 27952-27957 accordingly.deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml (1)
179-184: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueWildcard verb on
httproutes. Duplicate of the same generated rule; see the consolidated note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml` around lines 179 - 184, Remove the duplicate generated RBAC rule granting wildcard verbs on httproutes in the ClusterRole manifest, while retaining the consolidated gateway.networking.k8s.io httproutes rule elsewhere.deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml (1)
179-184: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueWildcard verb on
httproutes. Duplicate of the same generated rule; see the consolidated note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml` around lines 179 - 184, Remove the duplicate generated RBAC rule granting wildcard verbs on the httproutes resource in the gateway.networking.k8s.io API group. Keep the consolidated httproutes rule unchanged and ensure only one equivalent rule remains.deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml (1)
259-264: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueWildcard verb on
httproutes. Same generated rule as the other three RBAC manifests; see the consolidated note anchored on the template.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml` around lines 259 - 264, Update the RBAC rule for gateway.networking.k8s.io httproutes in the devworkspace-operator ClusterServiceVersion manifest to replace the wildcard verb with only the required explicit verbs, matching the other generated RBAC manifests and the source template.controllers/controller/devworkspacerouting/util_test.go (2)
168-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the helper tolerant of an absent route, and prefer an explicit name over a boolean flag.
Waiting for existence before deleting turns "route was never created" into a 10s teardown hang plus a misleading failure. Cleanup helpers should be idempotent. Also,
deleteHTTPRoute(name, ns, true)at the call sites is opaque; taking the full route name (or exposing aredirectRouteName(endpoint)helper) reads better and removes the duplicated-http-redirectliteral.♻️ Proposed refactor
+func redirectRouteName(endpointName string) string { + return common.RouteName(testWorkspaceID, endpointName) + "-http-redirect" +} + func deleteHTTPRoute(endpointName string, namespace string, isRedirect bool) { createdHTTPRoute := gwapiv1.HTTPRoute{} routeName := common.RouteName(testWorkspaceID, endpointName) if isRedirect { - routeName = routeName + "-http-redirect" + routeName = redirectRouteName(endpointName) } httpRouteNamespacedName := namespacedName(routeName, namespace) - Eventually(func() bool { - err := k8sClient.Get(ctx, httpRouteNamespacedName, &createdHTTPRoute) - return err == nil - }, timeout, interval).Should(BeTrue(), "HTTPRoute should exist in cluster") - deleteObject(&createdHTTPRoute) + if err := k8sClient.Get(ctx, httpRouteNamespacedName, &createdHTTPRoute); err != nil { + Expect(k8sErrors.IsNotFound(err)).Should(BeTrue(), "unexpected error fetching HTTPRoute %s", routeName) + return + } + deleteObject(&createdHTTPRoute) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/util_test.go` around lines 168 - 180, Update deleteHTTPRoute to accept the explicit route name rather than the isRedirect boolean, with callers using the existing route-name helpers or a shared redirectRouteName helper instead of duplicating the "-http-redirect" suffix. Make cleanup idempotent by deleting directly and treating a not-found HTTPRoute as successful, removing the Eventually wait that requires prior existence.
168-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
-http-redirectsuffix is hardcoded in two test locations and (presumably) a third time in the solver. A rename in the solver would leave both tests asserting against a name that no longer exists, and the negative/cleanup paths would fail confusingly rather than pointing at the rename.
controllers/controller/devworkspacerouting/util_test.go#L168-L180: introduce aredirectRouteName(endpointName string) stringhelper (or reuse an exported name-builder from the solver package) and use it here instead of the inline concatenation.controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go#L541-L547: replacecommon.RouteName(testWorkspaceID, exposedEndPointName)+"-http-redirect"with that shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/util_test.go` around lines 168 - 180, The redirect route suffix is duplicated across test helpers and assertions, so rename changes can leave tests targeting stale names. In controllers/controller/devworkspacerouting/util_test.go:168-180, add or reuse a shared redirectRouteName(endpointName string) helper and use it in deleteHTTPRoute; in controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go:541-547, replace the inline route-name concatenation with that same helper, keeping both tests aligned with the solver’s naming logic.controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go (3)
570-588: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo assertion on
spec.hostnames.The context configures
ClusterHostSuffix: "test-environment-cluster-suffix", but nothing verifies the generated HTTPRoute'shostnames— yet that is exactly what makes the route match traffic for a workspace endpoint. A wrong or empty hostname would pass this whole suite. Consider adding a check thatcreatedHTTPRoute.Spec.Hostnamescontains the expected<endpoint-host>.test-environment-cluster-suffixvalue (and likewise for the redirect route).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` around lines 570 - 588, Extend the HTTPRoute assertions in the discoverable endpoint test to verify discoverableHTTPRoute.Spec.Hostnames contains the expected endpoint host combined with ClusterHostSuffix "test-environment-cluster-suffix". Add equivalent hostname validation for the redirect route using its expected endpoint host, while preserving the existing metadata and non-exposed route checks.
366-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the exported constant instead of the
"gateway-api"literal.Line 416 already uses
controllerv1alpha1.DevWorkspaceRoutingGatewayAPI; keeping a raw literal here means a change to the constant's value would silently leave this config pointing at an unknown routing class.♻️ Proposed tweak
- DefaultRoutingClass: "gateway-api", + DefaultRoutingClass: string(controllerv1alpha1.DevWorkspaceRoutingGatewayAPI),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` around lines 366 - 377, Replace the hardcoded "gateway-api" value assigned to RoutingConfig.DefaultRoutingClass in the test setup with the exported controllerv1alpha1.DevWorkspaceRoutingGatewayAPI constant, matching the existing usage elsewhere in the test.
455-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThis "Creates services" body is a near-verbatim third copy.
Lines 455-500 duplicate the Kubernetes (108-161) and OpenShift (244-295) service assertions almost exactly. Service generation is routing-class independent, so extracting a shared
expectConsolidatedServices(createdDWR)helper would keep the three contexts in sync when the service shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` around lines 455 - 500, Extract the duplicated service assertions from the “Creates services” tests into a shared expectConsolidatedServices(createdDWR) helper, including consolidated and discoverable Service lookup, labels, owner reference, selectors, ports, and annotations. Replace the near-identical assertion bodies in the Kubernetes, OpenShift, and routing-class contexts with calls to this helper while preserving each test’s setup and context-specific behavior.deploy/templates/components/rbac/role.yaml (2)
177-182: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueWildcard verb on
httproutes(root marker lives in the controller). See the consolidated note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/templates/components/rbac/role.yaml` around lines 177 - 182, Replace the wildcard verb in the httproutes rule with only the explicit verbs required by the controller’s httproute access. Update the rule under apiGroups gateway.networking.k8s.io and resources httproutes, leaving the controller-owned root marker unchanged.
177-182: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTighten the Gateway API RBAC verb wildcard to an explicit list.
controllers/controller/devworkspacerouting/devworkspacerouting_controller.go:72generates the fourgateway.networking.k8s.io/httproutesrules, andverbs=*grantsdeletecollectionplus any future Gateway API verbs via both the template and deployed CSV/ClusterRole manifests. Narrow the marker toverbs=get;list;watch;create;update;patch;delete, then regenerate the RBAC manifests; apply the same precedent to the existingingresses/routesverbs=*rules if the same least-privilege goal applies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/templates/components/rbac/role.yaml` around lines 177 - 182, The Gateway API httproutes RBAC rule uses an overly broad verb wildcard. In deploy/templates/components/rbac/role.yaml:177-182, replace verbs=* with get;list;watch;create;update;patch;delete, then regenerate the corresponding manifests in deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml:179-184, deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml:179-184, and deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml:259-264. Apply the same explicit verb list to existing ingresses/routes wildcard rules only if they are part of the same least-privilege change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go`:
- Around line 430-439: Update the AfterEach cleanup around deleteHTTPRoute so
teardown does not require the discoverable endpoint’s redirect HTTPRoute to
exist; use cleanup behavior that tolerates absent routes while still removing
any routes that were created. Keep deletion for the other resources and verified
route variants unchanged, and align this with the non-strict cleanup approach
used by deleteHTTPRoute/util_test.go.
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller.go`:
- Around line 246-262: Call syncHTTPRoutes unconditionally in the HTTPRoute
reconciliation block, including when httpRoutes is empty, so its
deletion-diffing cleanup removes stale cluster HTTPRoutes. Preserve the existing
error handling, requeue/status behavior, and assignment of
clusterRoutingObj.HTTPRoutes for successful synchronization.
In `@controllers/controller/devworkspacerouting/suite_test.go`:
- Around line 115-116: Replace the deprecated gwapiv1.AddToScheme call with
gwapiv1.Install in the scheme setup, matching the existing routev1.Install usage
while preserving the existing error assertion.
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 66-81: The Gateway reference fields gatewayClassName, name, and
namespace in the CRD schema currently accept unrestricted strings. Add
Kubernetes DNS-1123 resource-name validation, including the appropriate maximum
length, to each field while preserving the existing descriptions and defaults;
ensure name remains required and empty or malformed values are rejected at CRD
validation.
In `@main.go`:
- Line 83: Replace the deprecated gwapiv1.AddToScheme call in the
scheme-registration setup with gwapiv1.Install(scheme), preserving the existing
utilruntime.Must handling and matching the nearby Gateway API registrations.
In `@pkg/provision/sync/diffopts.go`:
- Around line 96-101: Update httpRouteDiffOpts so cmpopts.IgnoreFields for
gwapiv1.ParentReference omits only Group and Kind, keeping Namespace included in
comparisons; leave the other HTTPRoute and reference-field ignore rules
unchanged.
---
Nitpick comments:
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go`:
- Around line 570-588: Extend the HTTPRoute assertions in the discoverable
endpoint test to verify discoverableHTTPRoute.Spec.Hostnames contains the
expected endpoint host combined with ClusterHostSuffix
"test-environment-cluster-suffix". Add equivalent hostname validation for the
redirect route using its expected endpoint host, while preserving the existing
metadata and non-exposed route checks.
- Around line 366-377: Replace the hardcoded "gateway-api" value assigned to
RoutingConfig.DefaultRoutingClass in the test setup with the exported
controllerv1alpha1.DevWorkspaceRoutingGatewayAPI constant, matching the existing
usage elsewhere in the test.
- Around line 455-500: Extract the duplicated service assertions from the
“Creates services” tests into a shared expectConsolidatedServices(createdDWR)
helper, including consolidated and discoverable Service lookup, labels, owner
reference, selectors, ports, and annotations. Replace the near-identical
assertion bodies in the Kubernetes, OpenShift, and routing-class contexts with
calls to this helper while preserving each test’s setup and context-specific
behavior.
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller.go`:
- Line 72: Restrict the httproutes RBAC marker in
devworkspacerouting_controller.go to get, list, watch, create, update, patch,
and delete, then run make generate_all to regenerate
deploy/deployment/kubernetes/combined.yaml; update the generated ClusterRole
entry at lines 27952-27957 accordingly.
In `@controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go`:
- Around line 17-26: Reorganize the import block in gateway API solver so
standard-library imports, third-party/Kubernetes imports such as metav1 and
gwapiv1, and project-local devworkspace-operator imports are separated by blank
lines.
- Around line 71-121: The generated routes in getHTTPRoutesForSpec rely on
cross-namespace attachment from workspaceMeta.Namespace to gatewayNamespace.
Confirm and document that the pre-provisioned Gateway listeners set
allowedRoutes.namespaces.from to permit the workspace namespace; if this
configuration is not guaranteed, update the Gateway provisioning/configuration
so both createHTTPRedirectRoute and createHTTPSBackendRoute routes can attach
successfully.
In `@controllers/controller/devworkspacerouting/util_test.go`:
- Around line 168-180: Update deleteHTTPRoute to accept the explicit route name
rather than the isRedirect boolean, with callers using the existing route-name
helpers or a shared redirectRouteName helper instead of duplicating the
"-http-redirect" suffix. Make cleanup idempotent by deleting directly and
treating a not-found HTTPRoute as successful, removing the Eventually wait that
requires prior existence.
- Around line 168-180: The redirect route suffix is duplicated across test
helpers and assertions, so rename changes can leave tests targeting stale names.
In controllers/controller/devworkspacerouting/util_test.go:168-180, add or reuse
a shared redirectRouteName(endpointName string) helper and use it in
deleteHTTPRoute; in
controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go:541-547,
replace the inline route-name concatenation with that same helper, keeping both
tests aligned with the solver’s naming logic.
In `@deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml`:
- Around line 259-264: Update the RBAC rule for gateway.networking.k8s.io
httproutes in the devworkspace-operator ClusterServiceVersion manifest to
replace the wildcard verb with only the required explicit verbs, matching the
other generated RBAC manifests and the source template.
In
`@deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml`:
- Around line 179-184: Remove the duplicate generated RBAC rule granting
wildcard verbs on httproutes in the ClusterRole manifest, while retaining the
consolidated gateway.networking.k8s.io httproutes rule elsewhere.
In `@deploy/deployment/openshift/combined.yaml`:
- Around line 27952-27957: In the ClusterRole rule for the
gateway.networking.k8s.io httproutes resource, replace the wildcard verb
permission with only the required create, update, delete, and watch verbs,
matching the lifecycle described and the existing ingresses/routes convention.
In
`@deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml`:
- Around line 179-184: Remove the duplicate generated RBAC rule granting
wildcard verbs on the httproutes resource in the gateway.networking.k8s.io API
group. Keep the consolidated httproutes rule unchanged and ensure only one
equivalent rule remains.
In `@deploy/templates/components/rbac/role.yaml`:
- Around line 177-182: Replace the wildcard verb in the httproutes rule with
only the explicit verbs required by the controller’s httproute access. Update
the rule under apiGroups gateway.networking.k8s.io and resources httproutes,
leaving the controller-owned root marker unchanged.
- Around line 177-182: The Gateway API httproutes RBAC rule uses an overly broad
verb wildcard. In deploy/templates/components/rbac/role.yaml:177-182, replace
verbs=* with get;list;watch;create;update;patch;delete, then regenerate the
corresponding manifests in
deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml:179-184,
deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml:179-184,
and
deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml:259-264.
Apply the same explicit verb list to existing ingresses/routes wildcard rules
only if they are part of the same least-privilege change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da5d02c0-ae32-40bb-af72-8715f4d81c15
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/devworkspacerouting_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller_test.gocontrollers/controller/devworkspacerouting/solvers/gateway_api_solver.gocontrollers/controller/devworkspacerouting/solvers/solver.gocontrollers/controller/devworkspacerouting/suite_test.gocontrollers/controller/devworkspacerouting/sync_httproutes.gocontrollers/controller/devworkspacerouting/testdata/gateway.networking.k8s.io_httproutes.yamlcontrollers/controller/devworkspacerouting/util_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/components/rbac/role.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlgo.modmain.gopkg/config/sync.gopkg/provision/sync/diff.gopkg/provision/sync/diffopts.go
| err = gwapiv1.AddToScheme(scheme.Scheme) | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t d -p 'gateway-api/apis/v1$' | head
rg -nP '\b(Install|AddToScheme)\b\s*=' --glob '**/gateway-api/apis/v1/*.go'Repository: devfile/devworkspace-operator
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find suite_test and references:"
fd -a 'suite_test.go$' . | sed 's#^\./##' | rg 'controllers/controller/devworkspacerouting|devworkspacerouting' || true
rg -n 'gwapiv1|routev1\.(Install|AddToScheme)|AddToScheme\(scheme\.Scheme\)' controllers/controller/devworkspacerouting go.mod go.sum 2>/dev/null || true
echo
echo "Go module gateway-api deps:"
python3 - <<'PY'
from pathlib import Path
for p in ['go.mod','go.sum']:
if Path(p).exists():
lines=[ln for ln in Path(p).read_text().splitlines() if 'gateway-api' in ln]
print(f'--- {p}')
for ln in lines[:40]:
print(ln)
PYRepository: devfile/devworkspace-operator
Length of output: 10022
🌐 Web query:
sigs.k8s.io/gateway-api v1.2.1 apis v1 Install AddToScheme deprecated
💡 Result:
In the Gateway API Go packages, the AddToScheme function is deprecated in favor of Install [1][2][3]. This deprecation is a common pattern in Kubernetes-related projects to align with updated conventions for registering API types into a runtime.Scheme. Both AddToScheme and Install typically resolve to the same underlying functionality provided by the SchemeBuilder [1][2][3], but Install is the currently supported and recommended idiom [2][4]. If you encounter code using AddToScheme (such as sigs.k8s.io/gateway-api/apis/v1.AddToScheme), you should update it to use the Install function from the same package (e.g., gwv1.Install) [2][4]. This ensures compatibility with current standards and avoids potential warnings or future removal of the deprecated alias [5][1][3]. It is also important to ensure that this registration happens exactly once (typically during the initialization or client setup phase) to avoid concurrency issues like "concurrent map writes" when multiple controllers or analyzers attempt to register types into the same shared runtime.Scheme simultaneously [2].
Citations:
- 1: https://github.com/istio/gateway-api/blob/e60ed053/apis/v1alpha2/zz_generated.register.go
- 2: https://github.com/k8sgpt-ai/k8sgpt/pull/1705/files
- 3: https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/v1.5.0/apix/config/v1alpha1/zz_generated.register.go
- 4: https://apache.googlesource.com/camel-k/+/25c3513fd12c52a386aa0f94148e8ae18eb225ca/pkg/apis/addtoscheme_gateway.go
- 5: https://gateway-api.sigs.k8s.io/guides/crd-management/
Replace the deprecated gwapiv1.AddToScheme call with gwapiv1.Install.
This path is flagged by staticcheck SA1019; Install is the non-deprecated gateway-api v1 scheme-builder replacement and already matches this file’s routev1.Install usage.
🐛 Proposed fix
- err = gwapiv1.AddToScheme(scheme.Scheme)
+ err = gwapiv1.Install(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = gwapiv1.AddToScheme(scheme.Scheme) | |
| Expect(err).NotTo(HaveOccurred()) | |
| err = gwapiv1.Install(scheme.Scheme) | |
| Expect(err).NotTo(HaveOccurred()) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 115-115: SA1019: gwapiv1.AddToScheme is deprecated: use Install instead
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/controller/devworkspacerouting/suite_test.go` around lines 115 -
116, Replace the deprecated gwapiv1.AddToScheme call with gwapiv1.Install in the
scheme setup, matching the existing routev1.Install usage while preserving the
existing error assertion.
Source: Linters/SAST tools
| gatewayClassName: | ||
| default: nginx | ||
| description: |- | ||
| GatewayClassName is the name of the GatewayClass to use. | ||
| This is used for validation and informational purposes. | ||
| Defaults to "nginx" if not specified. | ||
| type: string | ||
| name: | ||
| description: Name is the name of the Gateway resource | ||
| type: string | ||
| namespace: | ||
| description: |- | ||
| Namespace is the namespace of the Gateway resource. | ||
| If not specified, HTTPRoutes will reference a Gateway in the same namespace | ||
| as the DevWorkspace. | ||
| type: string |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate Gateway reference names as Kubernetes resource names.
name, namespace, and gatewayClassName are unrestricted strings; required only checks that name is present and still permits an empty or malformed value. Add DNS-1123/max-length validation so invalid references are rejected at the CRD boundary instead of failing during routing reconciliation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`
around lines 66 - 81, The Gateway reference fields gatewayClassName, name, and
namespace in the CRD schema currently accept unrestricted strings. Add
Kubernetes DNS-1123 resource-name validation, including the appropriate maximum
length, to each field while preserving the existing descriptions and defaults;
ensure name remains required and empty or malformed values are rejected at CRD
validation.
| utilruntime.Must(controllerv1alpha1.AddToScheme(scheme)) | ||
| utilruntime.Must(dwv1.AddToScheme(scheme)) | ||
| utilruntime.Must(dwv2.AddToScheme(scheme)) | ||
| utilruntime.Must(gwapiv1.AddToScheme(scheme)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | sed -n '1,120p'
echo
echo "Search gwapiv1 usage:"
rg -n "gwapiv1|Install\\(|AddToScheme\\(" -S . || true
echo
echo "Go module deps references:"
for f in go.mod go.sum; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "gateway|gwapiv1|sigs.k8s.io/gateway-api|AddToScheme|Install" "$f" || true
fi
doneRepository: devfile/devworkspace-operator
Length of output: 20904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read main.go around the referenced line without executing repository code.
if [ -f main.go ]; then
echo "--- main.go outline ---"
ast-grep outline main.go --view compact || true
echo "--- main.go lines 60-105 ---"
sed -n '60,105p' main.go | nl -ba -v60
fiRepository: devfile/devworkspace-operator
Length of output: 411
🌐 Web query:
gateway-api v1.2.1 apis/v1 AddToScheme deprecated Install
💡 Result:
In the context of the Kubernetes Gateway API, the function AddToScheme has been deprecated in favor of Install [1]. This transition is part of a broader move within the Kubernetes ecosystem to standardize how API types are registered with a runtime scheme. If you are maintaining code that previously used AddToScheme (e.g., v1alpha2.AddToScheme), you should update it to use the corresponding Install function (e.g., v1alpha2.Install) [1]. This pattern applies across the various API versions of the Gateway API (e.g., v1alpha2, v1beta1, v1) [1][2][3]. Regarding Gateway API v1.2.1 specifically, this was a patch release focused on addressing backward compatibility issues related to the SupportedFeatures field in the GatewayClass API [4][5]. It did not introduce the deprecation of AddToScheme itself, as this pattern is a general evolution of the project's Go client libraries [1]. If you are encountering issues related to installing CRDs or upgrading to v1.2 or later, please ensure that any stored versions of deprecated resources (such as v1alpha2 versions of GRPCRoute or ReferenceGrant) are properly migrated and removed from the storedVersions field in your custom resource definitions [4][6].
Citations:
- 1: remove deprecated v1alpha2.AddToScheme and use v1alpha2.Install instead kubernetes-sigs/gateway-api-inference-extension#649
- 2: https://pkg.go.dev/sigs.k8s.io/gateway-api/apis/v1alpha2
- 3: https://pkg.go.dev/sigs.k8s.io/gateway-api/apis/v1beta1
- 4: https://github.com/kubernetes-sigs/gateway-api/blob/main/CHANGELOG/1.2-CHANGELOG.md
- 5: https://github.com/kubernetes-sigs/gateway-api/releases/tag/v1.2.1
- 6: https://gateway-api.sigs.k8s.io/guides/getting-started/introduction/
Use gwapiv1.Install(scheme) instead of the deprecated AddToScheme.
AddToScheme is kept only for compatibility in Gateway API Go types; this entry is the only scheme registration still using it here, so switch it to Install to align with the nearby route/template/oauth/security registrations and avoid staticcheck SA1019.
🐛 Proposed fix
- utilruntime.Must(gwapiv1.AddToScheme(scheme))
+ utilruntime.Must(gwapiv1.Install(scheme))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| utilruntime.Must(gwapiv1.AddToScheme(scheme)) | |
| utilruntime.Must(gwapiv1.Install(scheme)) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 83-83: SA1019: gwapiv1.AddToScheme is deprecated: use Install instead
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.go` at line 83, Replace the deprecated gwapiv1.AddToScheme call in the
scheme-registration setup with gwapiv1.Install(scheme), preserving the existing
utilruntime.Must handling and matching the nearby Gateway API registrations.
Source: Linters/SAST tools
| var httpRouteDiffOpts = cmp.Options{ | ||
| cmpopts.IgnoreFields(gwapiv1.HTTPRoute{}, "TypeMeta", "ObjectMeta", "Status"), | ||
| cmpopts.IgnoreFields(gwapiv1.BackendRef{}, "Weight"), | ||
| cmpopts.IgnoreFields(gwapiv1.BackendObjectReference{}, "Group", "Kind", "Namespace"), | ||
| cmpopts.IgnoreFields(gwapiv1.ParentReference{}, "Group", "Kind", "Namespace"), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'gateway_api_solver.go' --exec cat -n {}Repository: devfile/devworkspace-operator
Length of output: 12597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the diff options file and usages, then inspect the relevant sections.
ast-grep outline pkg/provision/sync/diffopts.go --view expanded || true
printf '\n--- usages ---\n'
rg -n "httpRouteDiffOpts|ParentReference|BackendObjectReference|HTTPRoute" pkg/provision/sync -g '!**/*_test.go'
printf '\n--- diffopts excerpt ---\n'
sed -n '1,180p' pkg/provision/sync/diffopts.goRepository: devfile/devworkspace-operator
Length of output: 5214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the solver tests around gateway namespace assertions.
rg -n "Namespace|gatewayRef|ParentReference|BackendObjectReference|HTTPRoute" pkg/provision/sync -g '*_test.go'
printf '\n--- solver tests excerpt ---\n'
sed -n '1,240p' pkg/provision/sync/solvers/*gateway*test.go 2>/dev/null || trueRepository: devfile/devworkspace-operator
Length of output: 167
Keep ParentReference.Namespace in the comparison.
Group/Kind can stay ignored, but Namespace comes from GatewayRef.Namespace (or the routing namespace fallback), so ignoring it hides routing.gatewayRef.namespace changes and leaves existing HTTPRoutes attached to the old Gateway namespace.
Proposed fix
var httpRouteDiffOpts = cmp.Options{
cmpopts.IgnoreFields(gwapiv1.HTTPRoute{}, "TypeMeta", "ObjectMeta", "Status"),
cmpopts.IgnoreFields(gwapiv1.BackendRef{}, "Weight"),
cmpopts.IgnoreFields(gwapiv1.BackendObjectReference{}, "Group", "Kind", "Namespace"),
- cmpopts.IgnoreFields(gwapiv1.ParentReference{}, "Group", "Kind", "Namespace"),
+ cmpopts.IgnoreFields(gwapiv1.ParentReference{}, "Group", "Kind"),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var httpRouteDiffOpts = cmp.Options{ | |
| cmpopts.IgnoreFields(gwapiv1.HTTPRoute{}, "TypeMeta", "ObjectMeta", "Status"), | |
| cmpopts.IgnoreFields(gwapiv1.BackendRef{}, "Weight"), | |
| cmpopts.IgnoreFields(gwapiv1.BackendObjectReference{}, "Group", "Kind", "Namespace"), | |
| cmpopts.IgnoreFields(gwapiv1.ParentReference{}, "Group", "Kind", "Namespace"), | |
| } | |
| var httpRouteDiffOpts = cmp.Options{ | |
| cmpopts.IgnoreFields(gwapiv1.HTTPRoute{}, "TypeMeta", "ObjectMeta", "Status"), | |
| cmpopts.IgnoreFields(gwapiv1.BackendRef{}, "Weight"), | |
| cmpopts.IgnoreFields(gwapiv1.BackendObjectReference{}, "Group", "Kind", "Namespace"), | |
| cmpopts.IgnoreFields(gwapiv1.ParentReference{}, "Group", "Kind"), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/provision/sync/diffopts.go` around lines 96 - 101, Update
httpRouteDiffOpts so cmpopts.IgnoreFields for gwapiv1.ParentReference omits only
Group and Kind, keeping Namespace included in comparisons; leave the other
HTTPRoute and reference-field ignore rules unchanged.
| // that HTTPRoutes should attach to via parentRefs. | ||
| type GatewayReference struct { | ||
| // Name is the name of the Gateway resource | ||
| // +kubebuilder:validation:Required |
There was a problem hiding this comment.
Required only checks presence; maybe add length bounds?
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=253 |
There was a problem hiding this comment.
Good catch, I will add the length bounds.
| // Namespace is the namespace of the Gateway resource. | ||
| // If not specified, HTTPRoutes will reference a Gateway in the same namespace | ||
| // as the DevWorkspace. | ||
| // +kubebuilder:validation:Optional |
There was a problem hiding this comment.
nit, Perhaps add DNS-1123 label bounds here too
| // +kubebuilder:validation:Optional | |
| // +kubebuilder:validation:Optional | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=63 |
There was a problem hiding this comment.
Agreed, adding bounds.
| // Defaults to "nginx" if not specified. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default:="nginx" | ||
| GatewayClassName string `json:"gatewayClassName,omitempty"` |
There was a problem hiding this comment.
+kubebuilder:default always populates this, would omitempty be needed?
There was a problem hiding this comment.
On second thought, I am not sure why this field is needed. It is not referenced in gateway solver. It only consumes Name and Namespace. So generated HTTPRoutes are identical regardless of what this is set to.
There was a problem hiding this comment.
You're right, this field isn't used anywhere in the solver. I'll remove it to keep the API surface minimal — we can always add it back if we find a concrete use case.
| Owns(&corev1.Service{}). | ||
| Owns(&networkingv1.Ingress{}) | ||
| Owns(&networkingv1.Ingress{}). | ||
| Owns(&gwapiv1.HTTPRoute{}) |
There was a problem hiding this comment.
We currently register it unconditionally. Gateway API CRDs aren't installed by default on Kubernetes or OpenShift. Could we gate this the same way the Route watch is gated ?
There was a problem hiding this comment.
Good point, this will cause informer failures on clusters without Gateway API CRDs installed. I'll gate this similarly to how Route watches are gated. I'm thinking we can check for the HTTPRoute CRD presence at startup and only register the watch if it exists.
| resources: | ||
| - httproutes | ||
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
nit, scope httproutes verbs instead of *
There was a problem hiding this comment.
Agreed, will scope to the specific verbs needed: get, list, watch, create, update, patch, delete.
| // 10 hour timeout (matching ingress2gateway output and long-running workspace sessions) | ||
| requestTimeout := gwapiv1.Duration("10h") |
There was a problem hiding this comment.
nit, Perhaps introduce these timeouts as constants with links to ingress2gateway spec?
There was a problem hiding this comment.
Sure, will extract to a constant.
| if len(httpRoute.Spec.Hostnames) > 0 { | ||
| hostname := string(httpRoute.Spec.Hostnames[0]) | ||
| // Use HTTPS scheme | ||
| url = fmt.Sprintf("https://%s%s", hostname, endpoint.Path) |
There was a problem hiding this comment.
Could we reuse getURLForEndpoint (or mirror its logic) here so protocol, path-joining, and query/fragment handling stay consistent with the other solvers?
There was a problem hiding this comment.
Good call, I will refactor to use getURLForEndpoint to get consistent path/query/fragment handling.
| routeName := fmt.Sprintf("%s-http-redirect", common.RouteName(workspaceMeta.DevWorkspaceId, endpointName)) | ||
|
|
||
| httpsScheme := "https" | ||
| statusCode := 301 |
There was a problem hiding this comment.
nit, maybe use 308, which always preserves the request method?
There was a problem hiding this comment.
301 is the conventional choice for HTTP→HTTPS redirects (matches ingress-nginx, Traefik defaults, and ingress2gateway output). Since workspace endpoints are primarily browser-accessed, 301 is appropriate. Happy to discuss further if there's a specific use case for method-preserving redirects.
| ) gwapiv1.HTTPRoute { | ||
| routeName := fmt.Sprintf("%s-http-redirect", common.RouteName(workspaceMeta.DevWorkspaceId, endpointName)) | ||
|
|
||
| httpsScheme := "https" |
There was a problem hiding this comment.
A bit naive question: why are we always redirecting to HTTPS and routing over 443? I haven't checked but how would TLS be handled in this setup? Just want to make sure I understand the intended flow.
There was a problem hiding this comment.
TLS termination is handled at the Gateway level, not in the solver. The Gateway is configured with an HTTPS listener (port 443) that has a TLS certificate. The solver creates two HTTPRoutes per endpoint: one on port 80 that redirects to HTTPS, and one on port 443 that routes to the workspace service. Traffic is encrypted between the client and Gateway; traffic from Gateway to the pod is cluster-internal plaintext (same model as Ingress with TLS termination).
| httpsPort := gwapiv1.PortNumber(443) | ||
| servicePort := gwapiv1.PortNumber(endpoint.TargetPort) | ||
|
|
||
| return gwapiv1.HTTPRoute{ |
There was a problem hiding this comment.
nit, this builder and the one is createHTTPRedirectRoute share very similar fields. Maybe we can create a shared helper to reduce duplication?
There was a problem hiding this comment.
The two builders share ObjectMeta/ParentRefs setup but diverge significantly in their Rules (redirect filter vs backendRef + timeout). I lean toward keeping them separate for readability, but happy to extract the common ObjectMeta/ParentRef setup into a helper if you feel strongly.
|
I tested this PR on OpenShift with the built-in OpenShift Gateway API and can confirm the Environment: OCP 4.20 (AWS CI), DWO built from this PR. Steps I followed1. Enable OpenShift Gateway APIoc apply -f - <<'EOF'
apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
name: openshift-default
spec:
controllerName: openshift.io/gateway-controller/v1
EOF
oc get gatewayclass openshift-default -o jsonpath='{range .status.conditions[*]}{.type}={.status} {.message}{"\n"}{end}'
oc -n openshift-ingress get podsExample output: 2. Create a shared Gateway (80 + 443, cross-namespace routes)oc -n openshift-ingress get secrets | grep tls
DOMAIN=$(oc get ingresses.config.openshift.io cluster -o jsonpath='{.spec.domain}')
echo "$DOMAIN"
oc apply -f - <<'EOF'
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: shared-gateway
namespace: openshift-ingress
spec:
gatewayClassName: openshift-default
listeners:
- name: http
protocol: HTTP
port: 80
allowedRoutes:
namespaces:
from: All
- name: https
protocol: HTTPS
port: 443
tls:
mode: Terminate
certificateRefs:
- name: router-certs-default
allowedRoutes:
namespaces:
from: All
EOF
oc -n openshift-ingress get gateway shared-gateway
oc -n openshift-ingress get svc | grep shared-gateway3. Install DWO from this PR and configure DWOCDOMAIN=$(oc get ingresses.config.openshift.io cluster -o jsonpath='{.spec.domain}')
oc apply -f - <<EOF
apiVersion: controller.devfile.io/v1alpha1
kind: DevWorkspaceOperatorConfig
metadata:
name: devworkspace-operator-config
namespace: openshift-operators
config:
routing:
clusterHostSuffix: ${DOMAIN}
defaultRoutingClass: gateway-api
gatewayRef:
name: shared-gateway
namespace: openshift-ingress
gatewayClassName: openshift-default
EOF
oc -n openshift-operators get dwoc devworkspace-operator-config -o yaml
oc -n openshift-operators get deploy | grep -i workspace4. Create a test DevWorkspaceUse oc new-project gw-api-test 2>/dev/null || oc project gw-api-test
oc apply -f - <<'EOF'
apiVersion: workspace.devfile.io/v1alpha2
kind: DevWorkspace
metadata:
name: gw-test
namespace: gw-api-test
spec:
started: true
routingClass: gateway-api
template:
components:
- name: tools
container:
image: traefik/whoami:v1.10.3
args: ["--port", "8080"]
memoryLimit: 128Mi
endpoints:
- name: http-server
targetPort: 8080
exposure: public
protocol: http
EOF
oc -n gw-api-test get dw,podExample output: 5. Verify routing objectsoc -n gw-api-test get dw,dwr,svc,httproute
oc -n gw-api-test get dwr -o jsonpath='{.items[0].status.phase}{"\n"}{.items[0].status.exposedEndpoints.tools[0].url}{"\n"}'
oc -n gw-api-test get httproute -o yamlExample output: HTTPRoute (HTTPS) — key fields: spec:
parentRefs:
- name: shared-gateway
namespace: openshift-ingress
port: 443
rules:
- backendRefs:
- name: workspace1f219d6e267842ba-service
port: 8080
status:
parents:
- controllerName: openshift.io/gateway-controller/v1
conditions:
- type: Accepted
status: "True"
- type: ResolvedRefs
status: "True"6. Verify traffic through the GatewayHOST=$(oc -n gw-api-test get dwr -o jsonpath='{.items[0].status.exposedEndpoints.tools[0].url}' | sed 's|https://||')
GATEWAY_LB=$(oc -n openshift-ingress get svc shared-gateway-openshift-default -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')
GW_IP=$(dig +short "$GATEWAY_LB" | head -1)
echo "HOST=$HOST"
echo "GATEWAY_LB=$GATEWAY_LB"
echo "GW_IP=$GW_IP"
curl -vk --resolve "${HOST}:443:${GW_IP}" "https://${HOST}/"Example output: 7. Cleanupoc delete dw gw-test -n gw-api-test
oc get httproute -n gw-api-test
oc delete project gw-api-testResult
I can confirm the |
|
Also tested this setup on minikube with traefic following these steps https://gist.github.com/btjd/28e07c0f3b07ef0b35fa85ce7bc4f17c I can confirm these steps also work ✔️ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
218-222: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExit when conversion webhook registration fails.
If either
Complete()call returns an error, this code only logs the error and starts the manager. The operator can then run without a required conversion webhook. Terminate startup after logging either error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.go` around lines 218 - 222, Update both conversion webhook registration checks around ctrl.NewWebhookManagedBy(...).Complete() to terminate startup after logging an error, rather than continuing to start the manager. Preserve the existing error messages and apply the same exit behavior to both DevWorkspace v1alpha1 and v1alpha2 registrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@main.go`:
- Around line 218-222: Update both conversion webhook registration checks around
ctrl.NewWebhookManagedBy(...).Complete() to terminate startup after logging an
error, rather than continuing to start the manager. Preserve the existing error
messages and apply the same exit behavior to both DevWorkspace v1alpha1 and
v1alpha2 registrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 724b099b-9cca-4c51-817e-26d3cee0c6e5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/devworkspacerouting_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller_test.gocontrollers/controller/devworkspacerouting/solvers/gateway_api_solver.gocontrollers/controller/devworkspacerouting/solvers/solver.gocontrollers/controller/devworkspacerouting/suite_test.gocontrollers/controller/devworkspacerouting/sync_httproutes.gocontrollers/controller/devworkspacerouting/testdata/gateway.networking.k8s.io_httproutes.yamlcontrollers/controller/devworkspacerouting/util_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/components/rbac/role.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlgo.modmain.gopkg/config/sync.gopkg/provision/sync/diff.gopkg/provision/sync/diffopts.go
🚧 Files skipped from review as they are similar to previous changes (22)
- deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml
- deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml
- deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- controllers/controller/devworkspacerouting/util_test.go
- deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml
- pkg/config/sync.go
- deploy/templates/components/rbac/role.yaml
- pkg/provision/sync/diff.go
- apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
- pkg/provision/sync/diffopts.go
- controllers/controller/devworkspacerouting/solvers/solver.go
- deploy/deployment/openshift/combined.yaml
- deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
- apis/controller/v1alpha1/devworkspacerouting_types.go
- controllers/controller/devworkspacerouting/testdata/gateway.networking.k8s.io_httproutes.yaml
- controllers/controller/devworkspacerouting/devworkspacerouting_controller.go
- deploy/deployment/kubernetes/combined.yaml
- controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go
- deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
- controllers/controller/devworkspacerouting/sync_httproutes.go
- apis/controller/v1alpha1/zz_generated.deepcopy.go
Signed-off-by: Badre Tejado-Imam <btejado@redhat.com>
…t and watches Signed-off-by: Badre Tejado-Imam <btejado@redhat.com>
…s stalls Signed-off-by: Badre Tejado-Imam <btejado@redhat.com>
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new comments every 10m0s, so there may be a short delay before I respond. Available commands:
|
|
/retest |
- Remove unused GatewayClassName field from GatewayReference struct - Add MinLength/MaxLength validation markers on GatewayReference fields - Gate HTTPRoute watch and cache behind Gateway API CRD availability using infrastructure detection pattern (IsGatewayAPIInstalled) - Scope httproutes RBAC from wildcard to explicit verbs - Make endpoint readiness log/status messages generic - Extract httpRouteRequestTimeout constant for HTTPRoute timeout - Reuse getURLForEndpoint for HTTPRoute URL construction Assisted-by: Claude Opus 4.6 Signed-off-by: Badre Tejado-Imam <btejado@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Badre Tejado-Imam <btejado@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go (3)
454-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared service assertions into a helper.
This block repeats the service assertions from the Kubernetes context (Lines 108-161) and the OpenShift context (Lines 244-295) almost verbatim. Move the consolidated-service and discoverable-service checks into a helper in
util_test.goand call it from all three contexts. The Gateway API context also omits the "service is not created for non-exposed endpoint" check that the other two contexts perform; a shared helper closes that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` around lines 454 - 499, Extract the consolidated-service, discoverable-service, and non-exposed-endpoint assertions from the test blocks into a shared helper in util_test.go, using parameters for the workspace, exposed endpoints, namespace, and Kubernetes client context as needed. Replace the duplicated checks in the Kubernetes and OpenShift contexts with calls to this helper, and add the same call to the Gateway API context so it also verifies that no service is created for non-exposed endpoints.
519-523: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert the optional pointers are non-nil before dereferencing them.
ParentReference.Namespace,ParentReference.Port,BackendObjectReference.Port,HTTPRequestRedirectFilter.Scheme, andHTTPRequestRedirectFilter.StatusCodeare all optional pointers ingwapiv1. If the solver omits any of them, the spec panics on a nil dereference instead of reporting the missing field. Add aShouldNot(BeNil())assertion before each dereference.♻️ Proposed hardening for the parent reference assertions
parentRef := createdHTTPRoute.Spec.ParentRefs[0] Expect(string(parentRef.Name)).Should(Equal(gatewayName), "HTTPRoute should reference the configured Gateway") + Expect(parentRef.Namespace).ShouldNot(BeNil(), "HTTPRoute parent reference should set the Gateway namespace") Expect(*parentRef.Namespace).Should(Equal(gwapiv1.Namespace(gatewayNamespace)), "HTTPRoute should reference Gateway in correct namespace") + Expect(parentRef.Port).ShouldNot(BeNil(), "HTTPRoute parent reference should set the Gateway listener port") Expect(*parentRef.Port).Should(Equal(gwapiv1.PortNumber(443)), "HTTPS HTTPRoute should reference port 443")Apply the same pattern at Line 538, Line 552, and Lines 563-564.
Also applies to: 550-552, 562-564
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` around lines 519 - 523, Harden the Gateway API assertions in the test by adding ShouldNot(BeNil()) checks before dereferencing optional pointers in ParentReference.Namespace and Port, BackendObjectReference.Port, and HTTPRequestRedirectFilter.Scheme and StatusCode. Apply the checks at the existing assertion blocks around createdHTTPRoute and the corresponding lines near 538, 550-552, and 562-564, preserving the current value assertions afterward.
370-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the production routing constants in the test expectations.
Replace the
"gateway-api"literal withstring(controllerv1alpha1.DevWorkspaceRoutingGatewayAPI), and replace the"-http-redirect"route suffix literals with the value generated by the solver. Thegateway-apirouting class constant exists, but the redirect suffix is currently only produced as `"%s-http-redirect"; export it or reuse the solver helper for the test lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go` at line 370, Update the test expectations around DefaultRoutingClass to use string(controllerv1alpha1.DevWorkspaceRoutingGatewayAPI) instead of the hard-coded "gateway-api" literal, and replace "-http-redirect" suffix literals with the solver-generated value. Export or reuse the existing solver helper that produces the "%s-http-redirect" suffix so the test lookup stays aligned with production behavior.apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go (1)
122-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding RFC 1123 pattern validation for
NameandNamespace.
NameandNamespacehave length bounds but no character-format validation. Kubernetes object names follow the RFC 1123 subdomain pattern (^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$), and namespace names follow the RFC 1123 label pattern (^[a-z0-9]([-a-z0-9]*[a-z0-9])?$). Without a pattern, an invalid Gateway name (for example, one with uppercase characters) passes CRD admission and only fails later, when the solver buildsHTTPRouteparent references or when the Gateway API implementation rejects the route. Add a+kubebuilder:validation:Patternfor each field so invalid names are rejected at admission time.♻️ Suggested pattern additions
type GatewayReference struct { // Name is the name of the Gateway resource // +kubebuilder:validation:Required // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` Name string `json:"name"` // Namespace is the namespace of the Gateway resource. // If not specified, HTTPRoutes will reference a Gateway in the same namespace // as the DevWorkspace. // +kubebuilder:validation:Optional // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` Namespace *string `json:"namespace,omitempty"` }Run
make generate_allafter this change to regenerate the CRD manifests. As per coding guidelines, "After modifying API types, struct fields in API types, or kubebuilder markers inapis/, runmake generate_all."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go` around lines 122 - 137, Add RFC 1123 pattern validation markers to the Name and Namespace fields of GatewayReference, using the subdomain pattern for Name and the label pattern for Namespace while preserving their existing length and optionality markers. Run make generate_all to regenerate the CRD manifests after updating the API markers.Source: Coding guidelines
deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml (1)
61-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd RFC1123 pattern validation for
gatewayRef.name/gatewayRef.namespace(generated in two manifests).Both generated manifests define the same
gatewayRefschema with onlymaxLength/minLengthonnameandnamespace, no character-set pattern. Other Kubernetes object name/namespace fields in these files (for examplepvcName,serviceAccountName) enforce^[a-z0-9]([-a-z0-9]*[a-z0-9])?$. The root cause is the shared source struct; fix it once there.
deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml#L61-L83: regenerate after adding+kubebuilder:validation:PatterntoGatewayReference.Name/GatewayReference.Namespaceinapis/controller/v1alpha1/devworkspaceoperatorconfig_types.go.deploy/deployment/openshift/combined.yaml#L61-L83: no separate edit needed; runningmake generate_allafter the struct fix regenerates this file too.As per coding guidelines: "After modifying API types, struct fields in API types, or kubebuilder markers in
apis/, runmake generate_all."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml` around lines 61 - 83, Update GatewayReference.Name and GatewayReference.Namespace in apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go with the RFC1123 kubebuilder pattern validation matching other Kubernetes object names, then run make generate_all. This regenerates the schema in deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml#L61-L83 and deploy/deployment/openshift/combined.yaml#L61-L83; no separate manifest edits are needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/infrastructure/cluster.go`:
- Around line 46-54: The Initialize flow around detect and isGatewayAPIInstalled
currently enables Gateway API based only on the group name; update discovery to
inspect gateway.networking.k8s.io/v1 resources and set the flag only when the
HTTPRoute resource is served. Preserve the existing error propagation and
unsupported-cluster handling, and use ServerGroupsAndResources or
ServerResourcesForGroupVersion for resource-level discovery.
---
Nitpick comments:
In `@apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go`:
- Around line 122-137: Add RFC 1123 pattern validation markers to the Name and
Namespace fields of GatewayReference, using the subdomain pattern for Name and
the label pattern for Namespace while preserving their existing length and
optionality markers. Run make generate_all to regenerate the CRD manifests after
updating the API markers.
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller_test.go`:
- Around line 454-499: Extract the consolidated-service, discoverable-service,
and non-exposed-endpoint assertions from the test blocks into a shared helper in
util_test.go, using parameters for the workspace, exposed endpoints, namespace,
and Kubernetes client context as needed. Replace the duplicated checks in the
Kubernetes and OpenShift contexts with calls to this helper, and add the same
call to the Gateway API context so it also verifies that no service is created
for non-exposed endpoints.
- Around line 519-523: Harden the Gateway API assertions in the test by adding
ShouldNot(BeNil()) checks before dereferencing optional pointers in
ParentReference.Namespace and Port, BackendObjectReference.Port, and
HTTPRequestRedirectFilter.Scheme and StatusCode. Apply the checks at the
existing assertion blocks around createdHTTPRoute and the corresponding lines
near 538, 550-552, and 562-564, preserving the current value assertions
afterward.
- Line 370: Update the test expectations around DefaultRoutingClass to use
string(controllerv1alpha1.DevWorkspaceRoutingGatewayAPI) instead of the
hard-coded "gateway-api" literal, and replace "-http-redirect" suffix literals
with the solver-generated value. Export or reuse the existing solver helper that
produces the "%s-http-redirect" suffix so the test lookup stays aligned with
production behavior.
In
`@deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 61-83: Update GatewayReference.Name and GatewayReference.Namespace
in apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go with the RFC1123
kubebuilder pattern validation matching other Kubernetes object names, then run
make generate_all. This regenerates the schema in
deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml#L61-L83
and deploy/deployment/openshift/combined.yaml#L61-L83; no separate manifest
edits are needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c451e81f-e1cf-4369-9c56-c2739888369e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/devworkspacerouting_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller.gocontrollers/controller/devworkspacerouting/devworkspacerouting_controller_test.gocontrollers/controller/devworkspacerouting/solvers/gateway_api_solver.gocontrollers/controller/devworkspacerouting/solvers/solver.gocontrollers/controller/devworkspacerouting/suite_test.gocontrollers/controller/devworkspacerouting/sync_httproutes.gocontrollers/controller/devworkspacerouting/testdata/gateway.networking.k8s.io_httproutes.yamlcontrollers/controller/devworkspacerouting/util_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/components/rbac/role.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlgo.modmain.gopkg/cache/cache.gopkg/config/sync.gopkg/infrastructure/cluster.gopkg/provision/sync/diff.gopkg/provision/sync/diffopts.go
🚧 Files skipped from review as they are similar to previous changes (20)
- apis/controller/v1alpha1/zz_generated.deepcopy.go
- pkg/provision/sync/diff.go
- pkg/config/sync.go
- deploy/templates/components/rbac/role.yaml
- deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml
- main.go
- controllers/controller/devworkspacerouting/sync_httproutes.go
- deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml
- apis/controller/v1alpha1/devworkspacerouting_types.go
- controllers/controller/devworkspacerouting/suite_test.go
- go.mod
- deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
- deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml
- controllers/controller/devworkspacerouting/solvers/solver.go
- controllers/controller/devworkspacerouting/devworkspacerouting_controller.go
- deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- controllers/controller/devworkspacerouting/solvers/gateway_api_solver.go
- pkg/provision/sync/diffopts.go
- controllers/controller/devworkspacerouting/util_test.go
- controllers/controller/devworkspacerouting/testdata/gateway.networking.k8s.io_httproutes.yaml
| infraType, apiGroups, err := detect() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if current == Unsupported { | ||
| if infraType == Unsupported { | ||
| return fmt.Errorf("running on unsupported cluster") | ||
| } | ||
| current = infraType | ||
| isGatewayAPIInstalled = findAPIGroup(apiGroups, "gateway.networking.k8s.io") != nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
kubectl get --raw /apis/gateway.networking.k8s.io/v1 |
jq -e '.resources[] | select(.name == "httproutes")'Repository: devfile/devworkspace-operator
Length of output: 208
🏁 Script executed:
#!/bin/sh
set -eu
printf 'Files:\n'
git ls-files 'pkg/infrastructure/*.go' 'pkg/cache/cache.go' 'pkg/controllers/*' | sed -n '1,200p'
printf '\ncluster.go outline:\n'
ast-grep outline pkg/infrastructure/cluster.go || true
printf '\ncluster.go relevant lines:\n'
cat -n pkg/infrastructure/cluster.go | sed -n '1,140p'
printf '\nRouter/Gateway API source references:\n'
rg -n "HTTPRoute|GatewayRoute|gateway.networking.k8s.io|isGatewayAPIInstalled|GatewayNetwork|Supported|Unsupported|ServerGroups|ServerResources|ServerPreferredResources|findAPIGroup" pkg || trueRepository: devfile/devworkspace-operator
Length of output: 26074
Check the HTTPRoute resource before enabling Gateway API.
detect() only reads API groups via ServerGroups(), and Initialize() sets isGatewayAPIInstalled from the gateway.networking.k8s.io group name alone. Use resource discovery, such as ServerGroupsAndResources() or ServerResourcesForGroupVersion("gateway.networking.k8s.io/v1"), and set the flag only if v1 serves httproutes. This prevents false-positive Gateway API support when a group exists but the required resource is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/infrastructure/cluster.go` around lines 46 - 54, The Initialize flow
around detect and isGatewayAPIInstalled currently enables Gateway API based only
on the group name; update discovery to inspect gateway.networking.k8s.io/v1
resources and set the flag only when the HTTPRoute resource is served. Preserve
the existing error propagation and unsupported-cluster handling, and use
ServerGroupsAndResources or ServerResourcesForGroupVersion for resource-level
discovery.
|
/retest |
1 similar comment
|
/retest |
|
@btjd: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
What does this PR do?
Adds a new
gateway-apirouting class to the DevWorkspace Operator that exposes workspace endpoints via Kubernetes Gateway APIHTTPRoutes instead of Ingress/Route resources.
GatewayAPISolverthat creates HTTPRoutes attached to a pre-provisioned Gateway referenced in the operator config (config.routing.gatewayRef)GatewayReferencetype toDevWorkspaceOperatorConfigAPI to configure the target Gateway name, namespace, and GatewayClassDevWorkspaceRoutingGatewayAPI("gateway-api") routing class constantsigs.k8s.io/gateway-apiadded as a dependency; Gateway API HTTPRoute CRD bundled indeploy/What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-23675
Is it tested? How?
Unit tests added for the
GatewayAPISolverand routing controller covering:publicendpoints get HTTPRoutes)Manual verification:
DevWorkspaceOperatorConfigwithrouting.gatewayRefpointing to a pre-provisioned GatewayroutingClass: gateway-apiPR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes