Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Adds Support for GatewayClass SupportedVersion Condition #10140

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Oct 1, 2024

Description

Previously, the gateway2 controller would unconditionally set the GatewayClass Accepted condition to True. This PR adds a watch for the Gateway API CRDs that triggers GatewayClass reconciliation. The ReconcileGatewayClasses() method was updated to set GatewayClass status conditions based on the Gateway API spec.

Fixes: #10092
Part Of: #10093

Testing steps

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves solo-io#10092

@solo-changelog-bot
Copy link

Issues linked to changelog:
solo-io#10092

@@ -32,6 +32,10 @@ const (
)

var (
// SupportedVersions specifies the supported versions of Gateway API.
// [TODO] Remove "v1.0.0-rc1" when https://github.com/solo-io/gloo/issues/10115 is fixed.
SupportedVersions = []string{"v1.1.0", "v1.0.0", "v1.0.0-rc1"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that v1.1.0 was added to this list due to solo-io#10121

Copy link
Member

Choose a reason for hiding this comment

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

cc @sam-heilbron - this is somewhat relevant to the conversation we've been having around ironing out our GW API support matrix.

@danehans
Copy link
Contributor Author

danehans commented Oct 3, 2024

CI is blocked by solo-io#10159.

@sam-heilbron
Copy link
Contributor

@danehans A potential fix for that test failure has merged, and should unblock this work

Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

LGTM - I think we can remove the 1.0.0-rc1 from the supported version list now that the issue you linked has been closed out. There's also some possible e2e test code refactoring that could be done here but I don't think it's worth pursuing if upstream decides to accept your conformance suite contribution.

@danehans
Copy link
Contributor Author

danehans commented Oct 4, 2024

@timflannagan I created solo-io#10174 to track the removal of rc1 from the list of approved CRD versions.

@danehans danehans force-pushed the issue_10092 branch 2 times, most recently from 5c489b5 to 08baa71 Compare October 7, 2024 15:57
Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

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

LGTM

func (r *controllerReconciler) ReconcileCRDs(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
log := log.FromContext(ctx).WithValues("CustomResourceDefinition", req.Name)

log.Info("reconciling CustomResourceDefinition")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do you think this log line is useful at the info level? I wonder if this could just as easily be placed at the debug level

Copy link
Member

Choose a reason for hiding this comment

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

Good nit. Feels consistent with how the majority of the c-r reconcilers in the ecosystem are wired up imo. Let’s start here and dial back over time?

var (
gwParametersManifestFile = filepath.Join(util.MustGetThisDir(), "testdata", "gatewayparameters.yaml")
gcManifestFile = filepath.Join(util.MustGetThisDir(), "testdata", "gatewayclass.yaml")
supportedCrdsManifestFile = filepath.Join(util.MustGetThisDir(), "../../../../../projects/gateway2/crds", "gateway-crds.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It feels like we could expose a method in the gateway2 project to return the absolute path to the gateway-crds file, and then just invoke it 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.

I symlinked testdata/gateway-crds.yaml to ../../../../../projects/gateway2/crds.

@sam-heilbron sam-heilbron added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Nov 13, 2024
nfuden and others added 2 commits November 13, 2024 15:13
Co-authored-by: Sam Heilbron <SamHeilbron@gmail.com>
Co-authored-by: Ian Rudie <ilrudie@gmail.com>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
Previously, the gateway2 controller would unconditionally set the GatewayClass Accepted
condition to `True`. This PR updates the CRD watcher to trigger GatewayClass reconciliation.
The `ReconcileGatewayClasses()` method was updated to set GatewayClass status conditionas
based on the Gateway API spec.

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

kubernetes-sigs/gateway-api#3368 adds the conformance test upstream and has sparked debate of the viability of the SupportedVersion feature. /hold until the discussion is resolved.

@danehans
Copy link
Contributor Author

danehans commented Jan 6, 2025

Converting to draft since a rebase is required due to b545ea8 and /hold due to kubernetes-sigs/gateway-api#3368.

@danehans danehans marked this pull request as draft January 6, 2025 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress signals bulldozer to keep pr open (don't auto-merge)
Projects
None yet
6 participants