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
Assert observedGeneration is incremented in Status.Conditions. #1586
Changes from 18 commits
d5aebb9
6873420
0e5ed4c
5658f1d
3fbe62e
b58b45f
009e835
de2d9c5
db554da
6582754
c8f4c5d
546ad16
0b9d064
ec9c9ca
9441114
f87815b
3e97ba4
46fb65d
41f9802
e2ca2e9
c73eadb
759598d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/* | ||
Copyright 2022 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" | ||
"time" | ||
|
||
"github.com/stretchr/testify/require" | ||
"k8s.io/apimachinery/pkg/types" | ||
|
||
"sigs.k8s.io/gateway-api/apis/v1beta1" | ||
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes" | ||
"sigs.k8s.io/gateway-api/conformance/utils/suite" | ||
) | ||
|
||
func init() { | ||
ConformanceTests = append(ConformanceTests, GatewayObservedGenerationBump) | ||
} | ||
|
||
var GatewayObservedGenerationBump = suite.ConformanceTest{ | ||
ShortName: "GatewayObservedGenerationBump", | ||
Description: "A Gateway in the gateway-conformance-infra namespace should update the observedGeneration in all of it's Status.Conditions after an update to the spec", | ||
Manifests: []string{"tests/gateway-observed-generation-bump.yaml"}, | ||
Test: func(t *testing.T, s *suite.ConformanceTestSuite) { | ||
|
||
gwNN := types.NamespacedName{Name: "gateway-observed-generation-bump", Namespace: "gateway-conformance-infra"} | ||
|
||
t.Run("observedGeneration should increment", func(t *testing.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Minute) | ||
defer cancel() | ||
|
||
namespaces := []string{"gateway-conformance-infra"} | ||
kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) | ||
|
||
original := &v1beta1.Gateway{} | ||
err := s.Client.Get(ctx, gwNN, original) | ||
require.NoErrorf(t, err, "error getting Gateway: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.GatewayMustHaveLatestConditions(t, original) | ||
|
||
all := v1beta1.NamespacesFromAll | ||
|
||
mutate := original.DeepCopy() | ||
|
||
// mutate the Gateway Spec | ||
mutate.Spec.Listeners = append(mutate.Spec.Listeners, v1beta1.Listener{ | ||
Name: "alternate", | ||
Port: 8080, | ||
Protocol: v1beta1.HTTPProtocolType, | ||
AllowedRoutes: &v1beta1.AllowedRoutes{ | ||
Namespaces: &v1beta1.RouteNamespaces{From: &all}, | ||
}, | ||
}) | ||
|
||
err = s.Client.Update(ctx, mutate) | ||
require.NoErrorf(t, err, "error updating the Gateway: %v", err) | ||
|
||
// Ensure the generation and observedGeneration sync up | ||
kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) | ||
|
||
updated := &v1beta1.Gateway{} | ||
err = s.Client.Get(ctx, gwNN, updated) | ||
require.NoErrorf(t, err, "error getting Gateway: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.GatewayMustHaveLatestConditions(t, updated) | ||
|
||
if original.Generation == updated.Generation { | ||
dprotaso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) | ||
} | ||
}) | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
apiVersion: gateway.networking.k8s.io/v1beta1 | ||
kind: Gateway | ||
metadata: | ||
name: gateway-observed-generation-bump | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any way to use an existing Gateway for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to keep tests isolated - I don't want to change the base gateway if other tests assume it's static. |
||
namespace: gateway-conformance-infra | ||
spec: | ||
gatewayClassName: "{GATEWAY_CLASS_NAME}" | ||
listeners: | ||
- name: http | ||
port: 80 | ||
protocol: HTTP | ||
allowedRoutes: | ||
namespaces: | ||
from: All |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
/* | ||
Copyright 2022 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" | ||
"time" | ||
|
||
"github.com/stretchr/testify/require" | ||
"k8s.io/apimachinery/pkg/types" | ||
|
||
"sigs.k8s.io/gateway-api/apis/v1beta1" | ||
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes" | ||
"sigs.k8s.io/gateway-api/conformance/utils/suite" | ||
) | ||
|
||
func init() { | ||
ConformanceTests = append(ConformanceTests, GatewayClassObservedGenerationBump) | ||
} | ||
|
||
var GatewayClassObservedGenerationBump = suite.ConformanceTest{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The GatewayClass is part of the environment provided by the person running the conformance tests. I personally don't think we should modify it. Maybe we can ensure that ObservedGeneration in status of the provided GatewayClass matches the generation of the resource though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We talked about this in the community sync: @youngnick made the point that we might as well do this now as it's something we'll need in the future if/when I'm personally +1 on Nick's point, I think we should keep this. However if this is a sticking point, I can live with us dropping it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We do this already - this test is to ensure the observedGeneration changes when the |
||
ShortName: "GatewayClassObservedGenerationBump", | ||
Description: "A GatewayClass should update the observedGeneration in all of it's Status.Conditions after an update to the spec", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currently Contour is not reconciling changes to GatewayClasses, just taking their state when they are first created/observed by the Contour Gateway API provisioner due to: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.GatewayClass
We haven't yet finished our thinking/implementation on this area so this fails for us There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah nvm, found where we were short circuiting the status update logic, this should work with Contour we have a small fix to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Manifests: []string{"tests/gatewayclass-observed-generation-bump.yaml"}, | ||
Test: func(t *testing.T, s *suite.ConformanceTestSuite) { | ||
gwc := types.NamespacedName{Name: "gatewayclass-observed-generation-bump"} | ||
|
||
t.Run("observedGeneration should increment", func(t *testing.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Minute) | ||
defer cancel() | ||
|
||
kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) | ||
|
||
original := &v1beta1.GatewayClass{} | ||
err := s.Client.Get(ctx, gwc, original) | ||
require.NoErrorf(t, err, "error getting GatewayClass: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.GatewayClassMustHaveLatestConditions(t, original) | ||
|
||
mutate := original.DeepCopy() | ||
desc := "new" | ||
mutate.Spec.Description = &desc | ||
|
||
err = s.Client.Update(ctx, mutate) | ||
require.NoErrorf(t, err, "error updating the GatewayClass: %v", err) | ||
|
||
// Ensure the generation and observedGeneration sync up | ||
kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) | ||
|
||
updated := &v1beta1.GatewayClass{} | ||
err = s.Client.Get(ctx, gwc, updated) | ||
require.NoErrorf(t, err, "error getting GatewayClass: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.GatewayClassMustHaveLatestConditions(t, updated) | ||
|
||
if original.Generation == updated.Generation { | ||
t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) | ||
} | ||
}) | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
apiVersion: gateway.networking.k8s.io/v1beta1 | ||
kind: GatewayClass | ||
metadata: | ||
name: gatewayclass-observed-generation-bump | ||
spec: | ||
controllerName: "{GATEWAY_CONTROLLER_NAME}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a significant change. As I understand it, this requires implementations to watch GatewayClasses matching their controller name and then update status on those. I think in some cases GatewayClass is part of the deployment model and each controller supports exactly one class. I could be wrong, but need to get some more input from other implementations before moving forward with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it behind a support feature flag for now (so that the manifests don't get applied) |
||
description: "old" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/* | ||
Copyright 2022 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" | ||
"time" | ||
|
||
"github.com/stretchr/testify/require" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/types" | ||
|
||
"sigs.k8s.io/gateway-api/apis/v1beta1" | ||
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes" | ||
"sigs.k8s.io/gateway-api/conformance/utils/suite" | ||
) | ||
|
||
func init() { | ||
ConformanceTests = append(ConformanceTests, HTTPRouteObservedGenerationBump) | ||
} | ||
|
||
var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this sufficiently generic that the included code can work for every route type? This can be a follow up, just would be nice to have consistency across all Route types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do as a follow up |
||
ShortName: "HTTPRouteObservedGenerationBump", | ||
Description: "A HTTPRoute in the gateway-conformance-infra namespace should update the observedGeneration in all of it's Status.Conditions after an update to the spec", | ||
Manifests: []string{"tests/httproute-observed-generation-bump.yaml"}, | ||
Test: func(t *testing.T, s *suite.ConformanceTestSuite) { | ||
|
||
routeNN := types.NamespacedName{Name: "observed-generation-bump", Namespace: "gateway-conformance-infra"} | ||
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: "gateway-conformance-infra"} | ||
|
||
acceptedCondition := metav1.Condition{ | ||
Type: string(v1beta1.RouteConditionAccepted), | ||
Status: metav1.ConditionTrue, | ||
Reason: "", // any reason | ||
} | ||
|
||
t.Run("observedGeneration should increment", func(t *testing.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Minute) | ||
defer cancel() | ||
|
||
namespaces := []string{"gateway-conformance-infra"} | ||
kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) | ||
|
||
original := &v1beta1.HTTPRoute{} | ||
err := s.Client.Get(ctx, routeNN, original) | ||
require.NoErrorf(t, err, "error getting HTTPRoute: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.HTTPRouteMustHaveLatestConditions(t, original) | ||
|
||
mutate := original.DeepCopy() | ||
mutate.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-v2" | ||
err = s.Client.Update(ctx, mutate) | ||
require.NoErrorf(t, err, "error updating the HTTPRoute: %v", err) | ||
|
||
kubernetes.HTTPRouteMustHaveCondition(t, s.Client, s.TimeoutConfig, routeNN, gwNN, acceptedCondition) | ||
|
||
updated := &v1beta1.HTTPRoute{} | ||
err = s.Client.Get(ctx, routeNN, updated) | ||
require.NoErrorf(t, err, "error getting Gateway: %v", err) | ||
|
||
// Sanity check | ||
kubernetes.HTTPRouteMustHaveLatestConditions(t, updated) | ||
|
||
if original.Generation == updated.Generation { | ||
t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) | ||
} | ||
}) | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
apiVersion: gateway.networking.k8s.io/v1beta1 | ||
kind: HTTPRoute | ||
metadata: | ||
name: observed-generation-bump | ||
namespace: gateway-conformance-infra | ||
spec: | ||
parentRefs: | ||
- name: same-namespace | ||
rules: | ||
- backendRefs: | ||
- name: infra-backend-v1 | ||
port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the moment, contour will not mark this update as accepted, we don't support a port other than 80 or 443
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do something trivial here like you've done with gw class, or add an annotation etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe change an existing listeners allowedroutes section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This restriction is odd - the API doesn't specify any constraints on the port numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I may be misspeaking a little, but at the very least Contour doesn't let you configure multiple HTTP or HTTPS Listeners, only one port is allowed for each at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue to move this discussion off the PR - #1607
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is passing now on Contour since observed generation is set correctly, the status we get is:
The Gateway as a whole should still be
Accepted=true
since some part of the config is valid, per: https://gateway-api.sigs.k8s.io/geps/gep-1364/#acceptedobservedGenerations are all correct, though the status/validity isn't all there