Skip to content

feat: Add ReferenceGrant support for HTTPRoute #149

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

Merged
merged 14 commits into from
May 21, 2025

Conversation

dspo
Copy link
Contributor

@dspo dspo commented May 21, 2025

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

feat: Add ReferenceGrant support for HTTPRoute

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

dspo added 11 commits May 20, 2025 22:43
This commit introduces functionality to handle the ReferenceGrant resource in the Gateway API. It updates the Gateway controller logic, adds necessary permissions in RBAC manifests, and integrates condition handling for cross-namespace references. Additionally, skipped conformance tests related to ReferenceGrants are reinstated.
This commit introduces support for Gateway API ReferenceGrant CRD, enabling cross-namespace references for HTTPRoutes. It refactors backend reference handling to validate Service references and check ReferenceGrants. Also includes minor code cleanups, added cluster role permissions for ReferenceGrants, and adjustments to e2e manifests.
…reference-grant-for-route

# Conflicts:
#	test/conformance/conformance_test.go
Reorganized and simplified the predicate logic for ReferenceGrant handling across gateway and HTTPRoute controllers. Consolidated duplicate code into reusable functions, reducing redundancy and improving maintainability. This centralization ensures consistent behavior and clearer code structure.
…nce-grant-for-route

# Conflicts:
#	internal/controller/gateway_controller.go
#	internal/controller/utils.go
#	test/conformance/conformance_test.go
@dspo dspo changed the title feat: enable reference grant feat: check enable reference grant May 21, 2025
Copy link

github-actions bot commented May 21, 2025

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2025-05-21T10:49:13Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: partial
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 0
      Passed: 32
      Skipped: 1
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 1 test skips.

@dspo dspo requested a review from Copilot May 21, 2025 09:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a feature to detect and enable cross-namespace ReferenceGrant support, refactors reference grant checks and condition handling, and updates controllers to watch ReferenceGrant events.

  • Introduce a global enableReferenceGrant flag and detect the ReferenceGrant CRD in manager/run.go
  • Refactor utilities to unify reference grant logic (checkReferenceGrant, predicates) and simplify condition construction
  • Update both HTTPRoute and Gateway controllers to watch and reconcile based on ReferenceGrant changes
  • Remove skipped conformance tests for traditional routes, replacing the skip list with a TODO

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/conformance/conformance_test.go Cleared out skipped tests list for traditional routes
internal/manager/run.go Added ReferenceGrant CRD detection and SetEnableReferenceGrant
internal/controller/utils.go Refactored SetRouteConditionResolvedRefs, added grant predicates
internal/controller/httproute_controller.go Wired in cross-namespace grant checks and new predicates
internal/controller/gateway_controller.go Replaced custom predicates with shared grant predicate logic
Comments suppressed due to low confidence (5)

test/conformance/conformance_test.go:27

  • With all traditional route skips removed, add or update test cases to cover the new cross-namespace ReferenceGrant behaviors or adjust the skip list to reflect expected conformance changes.
var skippedTestsForTraditionalRoutes []string // TODO: HTTPRoute hostname intersection and listener hostname matching

internal/controller/utils.go:16

  • The import "cmp" has no path and will fail to compile. Please replace it with the correct module path (e.g., "github.com/google/go-cmp/cmp" or appropriate Go 1.21 import).
"cmp"

internal/controller/utils.go:961

  • The code uses slices.Contains but the slices package is not imported. Add import "slices" (Go 1.21) or the appropriate module path.
return slices.Contains(reasons, Reason(re.Reason))

internal/controller/utils.go:273

  • When err is not a ReasonError, the condition’s Reason remains the default ResolvedRefs, which may misrepresent the actual failure. Consider setting a generic reason or mapping other error types to an appropriate Reason.
if errors.As(err, &re) {

internal/manager/run.go:187

  • [nitpick] Mixing setupLog and logger for logging here may be confusing. Use setupLog.Error consistently for controller startup logs.
logger.Error(err, "CRD ReferenceGrants is not installed")

@dspo dspo changed the title feat: check enable reference grant feat: Add ReferenceGrant support for HTTPRoute May 21, 2025

// TODO: HTTPRoute hostname intersection and listener hostname matching
}
var skippedTestsForTraditionalRoutes []string // TODO: HTTPRoute hostname intersection and listener hostname matching
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

Resource: "referencegrants",
})
if err != nil {
logger.Error(err, "CRD ReferenceGrants is not installed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not need to print logs, or should it not be error-level logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

Copy link
Contributor

@ronething ronething left a comment

Choose a reason for hiding this comment

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

others lgtm.

@@ -101,6 +102,10 @@ func (r *HTTPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error {
Watches(&v1alpha1.GatewayProxy{},
handler.EnqueueRequestsFromMapFunc(r.listHTTPRoutesForGatewayProxy),
).
Watches(&v1beta1.ReferenceGrant{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we allow not installing RefereGrant?
If not installed, an error will occur here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is allowed to not install ReferenceGrant and an error will occur here if not installed ReferenceGrant.
Let me fix it.

@dspo dspo merged commit bf07565 into release-v2-dev May 21, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants