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

[Conformance]: Adds GatewayClass SupportedVersion Test #3368

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions conformance/tests/gatewayclass-supported-version-condition.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
Copyright 2024 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package tests

import (
"context"
"testing"

"github.com/stretchr/testify/require"
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
"sigs.k8s.io/gateway-api/conformance/utils/suite"
"sigs.k8s.io/gateway-api/pkg/consts"
"sigs.k8s.io/gateway-api/pkg/features"
)

func init() {
ConformanceTests = append(ConformanceTests, GatewayClassSupportedVersionCondition)
}

var GatewayClassSupportedVersionCondition = suite.ConformanceTest{
ShortName: "GatewayClassSupportedVersionCondition",
Features: []features.FeatureName{
features.SupportGateway,
},
Description: "A GatewayClass should set the SupportedVersion condition based on the presence and version of Gateway API CRDs in the cluster",
danehans marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this makes sense.

Consider I build an implementation that supports v1.1.

v1.2 comes out. Per the spec, even if I am willing to support it, I must set the condition to "false"

This means I cannot pass this conformance test for 1.2 until I upgrade to 1.2 which doesn't actually seem desired?

Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
gwc := types.NamespacedName{Name: s.GatewayClassName}

t.Run("SupportedVersion condition should be set correctly", func(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), s.TimeoutConfig.DefaultTestTimeout)
defer cancel()

// Ensure GatewayClass conditions are as expected before proceeding
kubernetes.GWCMustHaveAcceptedConditionTrue(t, s.Client, s.TimeoutConfig, gwc.Name)
kubernetes.GWCMustHaveSupportedVersionConditionAny(t, s.Client, s.TimeoutConfig, gwc.Name)

// Retrieve the GatewayClass CRD
crd := &apiextv1.CustomResourceDefinition{}
crdName := types.NamespacedName{Name: "gatewayclasses.gateway.networking.k8s.io"}
err := s.Client.Get(ctx, crdName, crd)
require.NoErrorf(t, err, "error getting GatewayClass CRD: %v", err)

if crd.Annotations != nil {
// Remove the bundle version annotation if it exists
if _, exists := crd.Annotations[consts.BundleVersionAnnotation]; !exists {
t.Fatalf("Annotation %q does not exist on CRD %s", consts.BundleVersionAnnotation, crdName)
}
delete(crd.Annotations, consts.BundleVersionAnnotation)
if err := s.Client.Update(ctx, crd); err != nil {
t.Fatalf("Failed to update CRD %s: %v", crdName, err)
}
}

// Ensure the SupportedVersion status condition is false
kubernetes.GWCMustHaveSupportedVersionConditionFalse(t, s.Client, s.TimeoutConfig, gwc.Name)
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 really expect implementations to have to watch CRDs and constantly reconcile the Gatewayclass? I get how we got to this point, but it feels like we are building APIs in isolation from considerations about user needs and implementation complexity.

Users are unlikely to ever have an incompatibility and somehow realize to check a random object (that they don't even create, so why would they know to look at it?).

The cost to implement this, however, is extremely high...

Copy link
Contributor

@howardjohn howardjohn Nov 19, 2024

Choose a reason for hiding this comment

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

FWIW the api-machinery folks have, historically, advised against an implementation reading CRDs at all or even assuming an API is defined by a CRD (vs another mechanism).

I realize this has not much to do with the test... but I missed the PR adding the condition


// Add the bundle version annotation
crd.Annotations[consts.BundleVersionAnnotation] = consts.BundleVersion
if err := s.Client.Update(ctx, crd); err != nil {
t.Fatalf("Failed to update CRD %s: %v", crdName, err)
}

// Ensure the SupportedVersion status condition is true
kubernetes.GWCMustHaveSupportedVersionConditionTrue(t, s.Client, s.TimeoutConfig, gwc.Name)

// Set the bundle version annotation to an unsupported version
crd.Annotations[consts.BundleVersionAnnotation] = "v0.0.0"
if err := s.Client.Update(ctx, crd); err != nil {
t.Fatalf("Failed to update CRD %s: %v", crdName, err)
}

// Ensure the SupportedVersion is false
kubernetes.GWCMustHaveSupportedVersionConditionFalse(t, s.Client, s.TimeoutConfig, gwc.Name)
Copy link
Member

Choose a reason for hiding this comment

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

Some implementations like GKE will ~immediately overwrite this CRD with the original version, can you add that as an acceptable outcome of this test?


// Add the bundle version annotation back
crd.Annotations[consts.BundleVersionAnnotation] = consts.BundleVersion
if err := s.Client.Update(ctx, crd); err != nil {
t.Fatalf("Failed to update CRD %s: %v", crdName, err)
}

// Ensure the SupportedVersion is true
kubernetes.GWCMustHaveSupportedVersionConditionTrue(t, s.Client, s.TimeoutConfig, gwc.Name)
})
},
}
8 changes: 8 additions & 0 deletions conformance/utils/config/timeout.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ type TimeoutConfig struct {
// Max value for conformant implementation: None
GWCMustBeAccepted time.Duration

// GWCMustBeSupportedVersion represents the maximum time for a GatewayClass to have a SupportedVersion condition set to true.
// Max value for conformant implementation: None
GWCMustBeSupportedVersion time.Duration

// HTTPRouteMustNotHaveParents represents the maximum time for an HTTPRoute to have either no parents or a single parent that is not accepted.
// Max value for conformant implementation: None
HTTPRouteMustNotHaveParents time.Duration
Expand Down Expand Up @@ -115,6 +119,7 @@ func DefaultTimeoutConfig() TimeoutConfig {
GatewayStatusMustHaveListeners: 60 * time.Second,
GatewayListenersMustHaveConditions: 60 * time.Second,
GWCMustBeAccepted: 180 * time.Second,
GWCMustBeSupportedVersion: 180 * time.Second,
HTTPRouteMustNotHaveParents: 60 * time.Second,
HTTPRouteMustHaveCondition: 60 * time.Second,
TLSRouteMustHaveCondition: 60 * time.Second,
Expand Down Expand Up @@ -155,6 +160,9 @@ func SetupTimeoutConfig(timeoutConfig *TimeoutConfig) {
if timeoutConfig.GWCMustBeAccepted == 0 {
timeoutConfig.GWCMustBeAccepted = defaultTimeoutConfig.GWCMustBeAccepted
}
if timeoutConfig.GWCMustBeSupportedVersion == 0 {
timeoutConfig.GWCMustBeSupportedVersion = defaultTimeoutConfig.GWCMustBeSupportedVersion
}
if timeoutConfig.HTTPRouteMustNotHaveParents == 0 {
timeoutConfig.HTTPRouteMustNotHaveParents = defaultTimeoutConfig.HTTPRouteMustNotHaveParents
}
Expand Down
90 changes: 90 additions & 0 deletions conformance/utils/kubernetes/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,26 @@ func GWCMustHaveAcceptedConditionTrue(t *testing.T, c client.Client, timeoutConf
return gwcMustBeAccepted(t, c, timeoutConfig, gwcName, string(metav1.ConditionTrue))
}

// GWCMustHaveSupportedVersionConditionTrue waits until the specified GatewayClass has a SupportedVersion condition set with a status value equal to True.
func GWCMustHaveSupportedVersionConditionTrue(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwcName string) string {
return gwcMustBeSupportedVersion(t, c, timeoutConfig, gwcName, string(metav1.ConditionTrue))
}

// GWCMustHaveSupportedVersionConditionFalse waits until the specified GatewayClass has a SupportedVersion condition set with a status value equal to False.
func GWCMustHaveSupportedVersionConditionFalse(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwcName string) string {
return gwcMustBeSupportedVersion(t, c, timeoutConfig, gwcName, string(metav1.ConditionFalse))
}

// GWCMustHaveAcceptedConditionAny waits until the specified GatewayClass has an Accepted condition set with a status set to any value.
func GWCMustHaveAcceptedConditionAny(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwcName string) string {
return gwcMustBeAccepted(t, c, timeoutConfig, gwcName, "")
}

// GWCMustHaveSupportedVersionConditionAny waits until the specified GatewayClass has a SupportedVersion condition set to any value.
func GWCMustHaveSupportedVersionConditionAny(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwcName string) string {
return gwcMustBeSupportedVersion(t, c, timeoutConfig, gwcName, "")
}

// gwcMustBeAccepted waits until the specified GatewayClass has an Accepted
// condition set. Passing an empty status string means that any value
// will be accepted. It also returns the ControllerName for the GatewayClass.
Expand Down Expand Up @@ -112,6 +127,35 @@ func gwcMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo
return controllerName
}

// gwcMustBeSupportedVersion waits until the specified GatewayClass has a SupportedVersion condition set.
// Passing an empty status string means that any value will be accepted. It also returns the ControllerName
// for the GatewayClass. This will cause the test to halt if the specified timeout is exceeded.
func gwcMustBeSupportedVersion(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwcName, expectedStatus string) string {
t.Helper()

var controllerName string
waitErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, timeoutConfig.GWCMustBeSupportedVersion, true, func(ctx context.Context) (bool, error) {
gwc := &gatewayv1.GatewayClass{}
err := c.Get(ctx, types.NamespacedName{Name: gwcName}, gwc)
if err != nil {
return false, fmt.Errorf("error fetching GatewayClass: %w", err)
}

controllerName = string(gwc.Spec.ControllerName)

if err := ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions); err != nil {
tlog.Log(t, "GatewayClass", err)
return false, nil
}

// Passing an empty string as the Reason means that any Reason will do.
return findConditionInList(t, gwc.Status.Conditions, "SupportedVersion", expectedStatus, ""), nil
})
require.NoErrorf(t, waitErr, "error waiting for %s GatewayClass to have SupportedVersion condition to be set: %v", gwcName, waitErr)

return controllerName
}

// GatewayMustHaveLatestConditions waits until the specified Gateway has
// all conditions updated with the latest observed generation.
func GatewayMustHaveLatestConditions(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, gwNN types.NamespacedName) {
Expand Down Expand Up @@ -249,6 +293,52 @@ func NamespacesMustBeReady(t *testing.T, c client.Client, timeoutConfig config.T
require.NoErrorf(t, waitErr, "error waiting for %s namespaces to be ready", strings.Join(namespaces, ", "))
}

// GatewayClassMustHaveCondition checks that the supplied GatewayClass has the supplied Condition,
// halting after the specified timeout is exceeded.
func GatewayClassMustHaveCondition(
t *testing.T,
client client.Client,
timeoutConfig config.TimeoutConfig,
gcName string,
expectedCondition metav1.Condition,
) {
t.Helper()

waitErr := wait.PollUntilContextTimeout(
context.Background(),
1*time.Second,
timeoutConfig.GWCMustBeSupportedVersion,
true,
func(ctx context.Context) (bool, error) {
gc := &gatewayv1.GatewayClass{}
gcNN := types.NamespacedName{
Name: gcName,
}
err := client.Get(ctx, gcNN, gc)
if err != nil {
return false, fmt.Errorf("error fetching GatewayClass: %w", err)
}

if err := ConditionsHaveLatestObservedGeneration(gc, gc.Status.Conditions); err != nil {
return false, err
}

if findConditionInList(t,
gc.Status.Conditions,
expectedCondition.Type,
string(expectedCondition.Status),
expectedCondition.Reason,
) {
return true, nil
}

return false, nil
},
)

require.NoErrorf(t, waitErr, "error waiting for GatewayClass status to have a Condition matching expectations")
}

// GatewayMustHaveCondition checks that the supplied Gateway has the supplied Condition,
// halting after the specified timeout is exceeded.
func GatewayMustHaveCondition(
Expand Down