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

Set GatewayClass status for ignored GatewayClasses #804

Merged
merged 5 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions cmd/gateway/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ func createProvisionerModeCommand() *cobra.Command {
return provisioner.StartManager(provisioner.Config{
Logger: logger,
GatewayClassName: gatewayClassName.value,
GatewayCtlrName: gatewayCtlrName.value,
})
},
}
Expand Down
2 changes: 1 addition & 1 deletion conformance/Makefile
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
NKG_TAG = edge
NKG_PREFIX = nginx-kubernetes-gateway
GATEWAY_CLASS = nginx
SUPPORTED_FEATURES = HTTPRoute,HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect
SUPPORTED_FEATURES = HTTPRoute,HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,GatewayClassObservedGenerationBump
KIND_KUBE_CONFIG=$${HOME}/.kube/kind/config
TAG = latest
PREFIX = conformance-test-runner
Expand Down
5 changes: 4 additions & 1 deletion docs/gateway-api-compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ Fields:
* `parametersRef` - not supported.
* `description` - supported.
* `status`
* `conditions` - partially supported.
* `conditions` - supported (Condition/Status/Reason):
* `Accepted/True/Accepted`
* `Accepted/False/InvalidParameters`
* `Accepted/False/GatewayClassConflict`: Custom reason for when the GatewayClass references this controller, but a different GatewayClass name is provided to the controller via the command-line argument.

### Gateway

Expand Down
9 changes: 8 additions & 1 deletion internal/events/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/go-logr/logr"
apiv1 "k8s.io/api/core/v1"
discoveryV1 "k8s.io/api/discovery/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/gateway-api/apis/v1beta1"

"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config"
Expand Down Expand Up @@ -48,6 +49,8 @@ type EventHandlerConfig struct {
StatusUpdater status.Updater
// Logger is the logger to be used by the EventHandler.
Logger logr.Logger
// ControllerName is the name of this controller.
ControllerName string
}

// EventHandlerImpl implements EventHandler.
Expand Down Expand Up @@ -121,7 +124,11 @@ func (h *EventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Confi
func (h *EventHandlerImpl) propagateUpsert(e *UpsertEvent) {
switch r := e.Resource.(type) {
case *v1beta1.GatewayClass:
h.cfg.Processor.CaptureUpsertChange(r)
if string(r.Spec.ControllerName) != h.cfg.ControllerName {
sjberman marked this conversation as resolved.
Show resolved Hide resolved
h.cfg.Processor.CaptureDeleteChange(r, client.ObjectKeyFromObject(r))
} else {
h.cfg.Processor.CaptureUpsertChange(r)
}
case *v1beta1.Gateway:
h.cfg.Processor.CaptureUpsertChange(r)
case *v1beta1.HTTPRoute:
Expand Down
5 changes: 2 additions & 3 deletions internal/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,7 @@ func Start(cfg config.Config) error {
{
objectType: &gatewayv1beta1.GatewayClass{},
options: []controller.Option{
controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(
types.NamespacedName{Name: cfg.GatewayClassName},
)),
controller.WithK8sPredicate(predicate.GatewayClassPredicate{ControllerName: cfg.GatewayCtlrName}),
},
},
{
Expand Down Expand Up @@ -177,6 +175,7 @@ func Start(cfg config.Config) error {
NginxFileMgr: nginxFileMgr,
NginxRuntimeMgr: nginxRuntimeMgr,
StatusUpdater: statusUpdater,
ControllerName: cfg.GatewayCtlrName,
})

objects, objectLists := prepareFirstEventBatchPreparerArgs(cfg.GatewayClassName, cfg.GatewayNsName)
Expand Down
47 changes: 47 additions & 0 deletions internal/manager/predicate/gatewayclass.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package predicate

import (
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/gateway-api/apis/v1beta1"
)

// GatewayClassPredicate implements a predicate function based on the controllerName of a GatewayClass.
// This predicate will skip events for GatewayClasses that don't reference this controller.
type GatewayClassPredicate struct {
sjberman marked this conversation as resolved.
Show resolved Hide resolved
predicate.Funcs
ControllerName string
}

// Create implements default CreateEvent filter for validating a GatewayClass controllerName.
func (gcp GatewayClassPredicate) Create(e event.CreateEvent) bool {
if e.Object == nil {
return false
}

gc, ok := e.Object.(*v1beta1.GatewayClass)
if !ok {
return false
}

return string(gc.Spec.ControllerName) == gcp.ControllerName
}

// Update implements default UpdateEvent filter for validating a GatewayClass controllerName.
func (gcp GatewayClassPredicate) Update(e event.UpdateEvent) bool {
if e.ObjectOld != nil {
gcOld, ok := e.ObjectOld.(*v1beta1.GatewayClass)
if ok {
return string(gcOld.Spec.ControllerName) == gcp.ControllerName
}
}

if e.ObjectNew != nil {
gcNew, ok := e.ObjectNew.(*v1beta1.GatewayClass)
if ok {
return string(gcNew.Spec.ControllerName) == gcp.ControllerName
}
}

return false
sjberman marked this conversation as resolved.
Show resolved Hide resolved
}
32 changes: 32 additions & 0 deletions internal/manager/predicate/gatewayclass_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package predicate

import (
"testing"

. "github.com/onsi/gomega"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/gateway-api/apis/v1beta1"
)

func TestGatewayClassPredicate(t *testing.T) {
g := NewGomegaWithT(t)

p := GatewayClassPredicate{ControllerName: "nginx-ctlr"}

gc := &v1beta1.GatewayClass{
Spec: v1beta1.GatewayClassSpec{
ControllerName: "nginx-ctlr",
},
}

g.Expect(p.Create(event.CreateEvent{Object: gc})).To(BeTrue())
g.Expect(p.Update(event.UpdateEvent{ObjectNew: gc})).To(BeTrue())

gc2 := &v1beta1.GatewayClass{
Spec: v1beta1.GatewayClassSpec{
ControllerName: "unknown",
},
}
g.Expect(p.Create(event.CreateEvent{Object: gc2})).To(BeFalse())
g.Expect(p.Update(event.UpdateEvent{ObjectOld: gc, ObjectNew: gc2})).To(BeTrue())
}
37 changes: 14 additions & 23 deletions internal/manager/predicate/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package predicate
import (
"testing"

. "github.com/onsi/gomega"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -224,34 +225,24 @@ func TestServicePortsChangedPredicate_Update(t *testing.T) {
p := ServicePortsChangedPredicate{}

for _, tc := range testcases {
update := p.Update(event.UpdateEvent{
ObjectOld: tc.objectOld,
ObjectNew: tc.objectNew,
})
t.Run(tc.msg, func(t *testing.T) {
g := NewGomegaWithT(t)
update := p.Update(event.UpdateEvent{
ObjectOld: tc.objectOld,
ObjectNew: tc.objectNew,
})

if update != tc.expUpdate {
t.Errorf(
"ServicePortsChangedPredicate.Update() mismatch for %q; got %t, expected %t",
tc.msg,
update,
tc.expUpdate,
)
}
g.Expect(update).To(Equal(tc.expUpdate))
})
}
}

func TestServicePortsChangedPredicate(t *testing.T) {
p := ServicePortsChangedPredicate{}

if !p.Delete(event.DeleteEvent{Object: &v1.Service{}}) {
t.Errorf("ServicePortsChangedPredicate.Delete() returned false; expected true")
}
g := NewGomegaWithT(t)

if !p.Create(event.CreateEvent{Object: &v1.Service{}}) {
t.Errorf("ServicePortsChangedPredicate.Create() returned false; expected true")
}
p := ServicePortsChangedPredicate{}

if !p.Generic(event.GenericEvent{Object: &v1.Service{}}) {
t.Errorf("ServicePortsChangedPredicate.Generic() returned false; expected true")
}
g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue())
g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue())
g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeTrue())
}
29 changes: 20 additions & 9 deletions internal/provisioner/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,28 @@ func newEventHandler(
}
}

func (h *eventHandler) ensureGatewayClassAccepted(ctx context.Context) {
gc, exist := h.store.gatewayClasses[types.NamespacedName{Name: h.gcName}]
if !exist {
panic(fmt.Errorf("GatewayClass %s must exist", h.gcName))
func (h *eventHandler) setGatewayClassStatuses(ctx context.Context) {
statuses := status.Statuses{
GatewayClassStatuses: make(status.GatewayClassStatuses),
}

statuses := status.Statuses{
GatewayClassStatus: &status.GatewayClassStatus{
Conditions: conditions.NewDefaultGatewayClassConditions(),
var gcExists bool
for nsname, gc := range h.store.gatewayClasses {
var conds []conditions.Condition
if gc.Name == h.gcName {
gcExists = true
conds = conditions.NewDefaultGatewayClassConditions()
} else {
conds = []conditions.Condition{conditions.NewGatewayClassConflict()}
}

statuses.GatewayClassStatuses[nsname] = status.GatewayClassStatus{
Conditions: conds,
ObservedGeneration: gc.Generation,
},
}
}
if !gcExists {
panic(fmt.Errorf("GatewayClass %s must exist", h.gcName))
}

h.statusUpdater.Update(ctx, statuses)
Expand Down Expand Up @@ -133,7 +144,7 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {

func (h *eventHandler) HandleEventBatch(ctx context.Context, batch events.EventBatch) {
h.store.update(batch)
h.ensureGatewayClassAccepted(ctx)
h.setGatewayClassStatuses(ctx)
h.ensureDeploymentsMatchGateways(ctx)
}

Expand Down
65 changes: 53 additions & 12 deletions internal/provisioner/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
embeddedfiles "github.com/nginxinc/nginx-kubernetes-gateway"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/status"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes"
)
Expand Down Expand Up @@ -272,6 +273,46 @@ var _ = Describe("handler", func() {
Expect(deps.Items).To(HaveLen(0))
})
})

When("upserting GatewayClass that is not set in command-line argument", func() {
It("should set the proper status if this controller is referenced", func() {
gc := &v1beta1.GatewayClass{
ObjectMeta: metav1.ObjectMeta{
Name: "unknown-gc",
},
Spec: v1beta1.GatewayClassSpec{
ControllerName: "test.example.com",
},
}
err := k8sclient.Create(context.Background(), gc)
Expect(err).ShouldNot(HaveOccurred())

batch := []interface{}{
&events.UpsertEvent{
Resource: gc,
},
}

handler.HandleEventBatch(context.Background(), batch)

unknownGC := &v1beta1.GatewayClass{}
err = k8sclient.Get(context.Background(), client.ObjectKeyFromObject(gc), unknownGC)
Expect(err).ShouldNot(HaveOccurred())

expectedConditions := []metav1.Condition{
{
Type: string(v1beta1.GatewayClassConditionStatusAccepted),
Status: metav1.ConditionFalse,
ObservedGeneration: 0,
LastTransitionTime: fakeClockTime,
Reason: string(conditions.GatewayClassReasonGatewayClassConflict),
Message: string(conditions.GatewayClassMessageGatewayClassConflict),
},
}

Expect(unknownGC.Status.Conditions).To(Equal(expectedConditions))
})
})
})

Describe("Edge cases", func() {
Expand Down Expand Up @@ -392,20 +433,20 @@ var _ = Describe("handler", func() {
Expect(handle).Should(Panic())
})
})
})

When("upserting Gateway with broken static Deployment YAML", func() {
It("it should panic", func() {
handler = newEventHandler(
gcName,
statusUpdater,
k8sclient,
zap.New(),
[]byte("broken YAML"),
)
When("upserting Gateway with broken static Deployment YAML", func() {
It("it should panic", func() {
handler = newEventHandler(
gcName,
statusUpdater,
k8sclient,
zap.New(),
[]byte("broken YAML"),
)

itShouldUpsertGatewayClass()
itShouldPanicWhenUpsertingGateway(types.NamespacedName{Namespace: "test-ns", Name: "test-gw"})
itShouldUpsertGatewayClass()
itShouldPanicWhenUpsertingGateway(types.NamespacedName{Namespace: "test-ns", Name: "test-gw"})
})
})
})
})
8 changes: 3 additions & 5 deletions internal/provisioner/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
v1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
ctlr "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -17,14 +16,15 @@ import (
embeddedfiles "github.com/nginxinc/nginx-kubernetes-gateway"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/controller"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/filter"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/status"
)

// Config is configuration for the provisioner mode.
type Config struct {
Logger logr.Logger
GatewayClassName string
GatewayCtlrName string
}

// StartManager starts a Manager for the provisioner mode, which provisions
Expand Down Expand Up @@ -60,9 +60,7 @@ func StartManager(cfg Config) error {
{
objectType: &gatewayv1beta1.GatewayClass{},
options: []controller.Option{
controller.WithNamespacedNameFilter(
filter.CreateSingleResourceFilter(types.NamespacedName{Name: cfg.GatewayClassName}),
),
controller.WithK8sPredicate(predicate.GatewayClassPredicate{ControllerName: cfg.GatewayCtlrName}),
},
},
{
Expand Down
Loading
Loading