Skip to content

Commit

Permalink
feat: set GWC accepted condition properly (#1021)
Browse files Browse the repository at this point in the history
* feat: set GWC accepted condition properly

The GWC accepted condition is now properly set to False with reason
InvalidParameters in case the ParametersRef field of the GatewayClass
spec is not a valid GatewayConfiguration.

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

* Apply suggestions from code review

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>

* Apply suggestions from code review

Co-authored-by: Jakub Warczarek <jakub.warczarek@konghq.com>

* chore: address review comments

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

* chore: add godocs

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

---------

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Co-authored-by: Jakub Warczarek <jakub.warczarek@konghq.com>
  • Loading branch information
3 people authored Jan 20, 2025
1 parent 8d5072c commit ac21d78
Show file tree
Hide file tree
Showing 12 changed files with 339 additions and 40 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@
- Move `DataPlane` promotion in progress validation to CRD CEL validation expressions.
This is relevant for `DataPlane`s with BlueGreen rollouts enabled only.
[#1054](https://github.com/Kong/gateway-operator/pull/1054)
- The `GatewayClass` Accepted Condition is set to `False` with reason `InvalidParameters`
in case the `.spec.parametersRef` field is not a valid reference to an existing
`GatewayConfiguration` object.
[#1021](https://github.com/Kong/gateway-operator/pull/1021)

[kubebuilder_3907]: https://github.com/kubernetes-sigs/kubebuilder/discussions/3907

Expand Down
9 changes: 8 additions & 1 deletion controller/gateway/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
log.Trace(logger, "checking gatewayclass")
gwc, err := gatewayclass.Get(ctx, r.Client, string(gateway.Spec.GatewayClassName))
if err != nil {
if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
switch {
case errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}):
log.Debug(logger, "resource not supported, ignoring",
"expectedGatewayClass", vars.ControllerName(),
"gatewayClass", gateway.Spec.GatewayClassName,
"reason", err.Error(),
)
return ctrl.Result{}, nil
case errors.As(err, &operatorerrors.ErrNotAcceptedGatewayClass{}):
log.Debug(logger, "GatewayClass not accepted, ignoring",
"gatewayClass", gateway.Spec.GatewayClassName,
"reason", err.Error(),
)
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
}
Expand Down
10 changes: 7 additions & 3 deletions controller/gateway/controller_watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ func (r *Reconciler) gatewayHasMatchingGatewayClass(obj client.Object) bool {
// class as well. If we fail here it's most likely because of some failure
// of the Kubernetes API and it's technically better to enqueue the object
// than to drop it for eventual consistency during cluster outages.
return !errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{})
return !errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) &&
!errors.As(err, &operatorerrors.ErrNotAcceptedGatewayClass{})
}

return true
Expand Down Expand Up @@ -229,9 +230,12 @@ func (r *Reconciler) listManagedGatewaysInNamespace(ctx context.Context, obj cli
objKey := client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}

if _, err := gatewayclass.Get(ctx, r.Client, string(gateway.Spec.GatewayClassName)); err != nil {
if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
switch {
case errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}):
log.Debug(logger, "gateway class not supported, ignoring")
} else {
case errors.As(err, &operatorerrors.ErrNotAcceptedGatewayClass{}):
log.Debug(logger, "gateway class not accepted, ignoring")
default:
log.Error(logger, err, "failed to get Gateway's GatewayClass",
"gatewayClass", objKey.Name,
"gateway", gateway.Name,
Expand Down
38 changes: 15 additions & 23 deletions controller/gatewayclass/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"

k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
Expand All @@ -16,7 +15,6 @@ import (
"github.com/kong/gateway-operator/controller"
"github.com/kong/gateway-operator/controller/pkg/log"
"github.com/kong/gateway-operator/internal/utils/gatewayclass"
"github.com/kong/gateway-operator/pkg/consts"
k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
)

Expand Down Expand Up @@ -55,29 +53,23 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, nil
}

if !gwc.IsAccepted() {
oldGwc := gwc.DeepCopy()
oldGwc := gwc.DeepCopy()

k8sutils.SetCondition(
k8sutils.NewConditionWithGeneration(
consts.ConditionType(gatewayv1.GatewayClassConditionStatusAccepted),
metav1.ConditionTrue,
consts.ConditionReason(gatewayv1.GatewayClassReasonAccepted),
"the gatewayclass has been accepted by the operator",
gwc.GetGeneration(),
),
gwc,
)
if err := r.Status().Patch(ctx, gwc.GatewayClass, client.MergeFrom(oldGwc)); err != nil {
if k8serrors.IsConflict(err) {
log.Debug(logger, "conflict found when updating GatewayClass, retrying")
return ctrl.Result{
Requeue: true,
RequeueAfter: controller.RequeueWithoutBackoff,
}, nil
}
return ctrl.Result{}, fmt.Errorf("failed patching GatewayClass: %w", err)
condition, err := getAcceptedCondition(ctx, r.Client, gwc.GatewayClass)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to get accepted condition: %w", err)
}
k8sutils.SetCondition(*condition, gwc)

if err := r.Status().Patch(ctx, gwc.GatewayClass, client.MergeFrom(oldGwc)); err != nil {
if k8serrors.IsConflict(err) {
log.Debug(logger, "conflict found when updating GatewayClass, retrying")
return ctrl.Result{
Requeue: true,
RequeueAfter: controller.RequeueWithoutBackoff,
}, nil
}
return ctrl.Result{}, fmt.Errorf("failed patching GatewayClass: %w", err)
}

return ctrl.Result{}, nil
Expand Down
65 changes: 65 additions & 0 deletions controller/gatewayclass/controller_reconciler_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package gatewayclass

import (
"context"
"strings"

k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"

operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1"
"github.com/kong/gateway-operator/pkg/consts"
k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
)

// getAcceptedCondition returns the accepted condition for the GatewayClass, with
// the proper status, reason and message.
func getAcceptedCondition(ctx context.Context, cl client.Client, gwc *gatewayv1.GatewayClass) (*metav1.Condition, error) {
reason := gatewayv1.GatewayClassReasonAccepted
messages := []string{}
status := metav1.ConditionFalse

if gwc.Spec.ParametersRef != nil {
validRef := true
if gwc.Spec.ParametersRef.Group != gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group) ||
gwc.Spec.ParametersRef.Kind != "GatewayConfiguration" {
reason = gatewayv1.GatewayClassReasonInvalidParameters
messages = append(messages, "ParametersRef must reference a gateway-operator.konghq.com/GatewayConfiguration")
validRef = false
}

if gwc.Spec.ParametersRef.Namespace == nil {
reason = gatewayv1.GatewayClassReasonInvalidParameters
messages = append(messages, "ParametersRef must reference a namespaced resource")
validRef = false
}

if validRef {
gatewayConfig := operatorv1beta1.GatewayConfiguration{}
err := cl.Get(ctx, client.ObjectKey{Name: gwc.Spec.ParametersRef.Name, Namespace: string(*gwc.Spec.ParametersRef.Namespace)}, &gatewayConfig)
if client.IgnoreNotFound(err) != nil {
return nil, err
}
if k8serrors.IsNotFound(err) {
reason = gatewayv1.GatewayClassReasonInvalidParameters
messages = append(messages, "The referenced GatewayConfiguration does not exist")
}
}
}
if reason == gatewayv1.GatewayClassReasonAccepted {
status = metav1.ConditionTrue
messages = []string{"GatewayClass is accepted"}
}

acceptedCondition := k8sutils.NewConditionWithGeneration(
consts.ConditionType(gatewayv1.GatewayClassConditionStatusAccepted),
status,
consts.ConditionReason(reason),
strings.Join(messages, ". "),
gwc.GetGeneration(),
)

return &acceptedCondition, nil
}
131 changes: 131 additions & 0 deletions controller/gatewayclass/controller_reconciler_utils_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package gatewayclass

import (
"context"
"testing"

"github.com/samber/lo"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"

operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1"
)

func TestGetAcceptedCondition(t *testing.T) {
scheme := runtime.NewScheme()
assert.NoError(t, gatewayv1.Install(scheme))
assert.NoError(t, operatorv1beta1.AddToScheme(scheme))

tests := []struct {
name string
gwc *gatewayv1.GatewayClass
existingObjs []runtime.Object
expectedStatus metav1.ConditionStatus
expectedReason string
expectedMsg string
}{
{
name: "ParametersRef is nil",
gwc: &gatewayv1.GatewayClass{
Spec: gatewayv1.GatewayClassSpec{
ParametersRef: nil,
},
},
expectedStatus: metav1.ConditionTrue,
expectedReason: string(gatewayv1.GatewayClassReasonAccepted),
expectedMsg: "GatewayClass is accepted",
},
{
name: "Invalid ParametersRef Group and kind",
gwc: &gatewayv1.GatewayClass{
Spec: gatewayv1.GatewayClassSpec{

ParametersRef: &gatewayv1.ParametersReference{
Group: "invalid.group",
Kind: "InvalidKind",
Namespace: lo.ToPtr(gatewayv1.Namespace("default")),
Name: "invalid",
},
},
},
expectedStatus: metav1.ConditionFalse,
expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters),
expectedMsg: "ParametersRef must reference a gateway-operator.konghq.com/GatewayConfiguration",
},
{
name: "ParametersRef Namespace is nil",
gwc: &gatewayv1.GatewayClass{
Spec: gatewayv1.GatewayClassSpec{
ParametersRef: &gatewayv1.ParametersReference{
Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group),
Kind: "GatewayConfiguration",
Name: "no-namespace",
},
},
},
expectedStatus: metav1.ConditionFalse,
expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters),
expectedMsg: "ParametersRef must reference a namespaced resource",
},
{
name: "GatewayConfiguration does not exist",
gwc: &gatewayv1.GatewayClass{
Spec: gatewayv1.GatewayClassSpec{
ParametersRef: &gatewayv1.ParametersReference{
Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group),
Kind: "GatewayConfiguration",
Name: "nonexistent",
Namespace: lo.ToPtr(gatewayv1.Namespace("default")),
},
},
},
expectedStatus: metav1.ConditionFalse,
expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters),
expectedMsg: "The referenced GatewayConfiguration does not exist",
},
{
name: "Valid ParametersRef",
gwc: &gatewayv1.GatewayClass{
Spec: gatewayv1.GatewayClassSpec{
ParametersRef: &gatewayv1.ParametersReference{
Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group),
Kind: "GatewayConfiguration",
Name: "valid-config",
Namespace: lo.ToPtr(gatewayv1.Namespace("default")),
},
},
},
existingObjs: []runtime.Object{
&operatorv1beta1.GatewayConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid-config",
Namespace: "default",
},
},
},
expectedStatus: metav1.ConditionTrue,
expectedReason: string(gatewayv1.GatewayClassReasonAccepted),
expectedMsg: "GatewayClass is accepted",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()
cl := fake.NewClientBuilder().
WithScheme(scheme).
WithRuntimeObjects(tt.existingObjs...).
Build()

condition, err := getAcceptedCondition(ctx, cl, tt.gwc)
assert.NoError(t, err)
assert.NotNil(t, condition)
assert.Equal(t, tt.expectedStatus, condition.Status)
assert.Equal(t, tt.expectedReason, condition.Reason)
assert.Equal(t, tt.expectedMsg, condition.Message)
})
}
}
15 changes: 13 additions & 2 deletions controller/specialized/aigateway_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,19 @@ func (r *AIGatewayReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// See: https://github.com/kubernetes-sigs/controller-runtime/issues/1996
gwc, err := gatewayclass.Get(ctx, r.Client, aigateway.Spec.GatewayClassName)
if err != nil {
if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
log.Debug(logger, "resource not supported, ignoring", "ExpectedGatewayClass", vars.ControllerName())
switch {
case errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}):
log.Debug(logger, "resource not supported, ignoring",
"expectedGatewayClass", vars.ControllerName(),
"gatewayClass", aigateway.Spec.GatewayClassName,
"reason", err.Error(),
)
return ctrl.Result{}, nil
case errors.As(err, &operatorerrors.ErrNotAcceptedGatewayClass{}):
log.Debug(logger, "GatewayClass not accepted, ignoring",
"gatewayClass", aigateway.Spec.GatewayClassName,
"reason", err.Error(),
)
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
Expand Down
3 changes: 2 additions & 1 deletion controller/specialized/aigateway_controller_watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ func (r *AIGatewayReconciler) aiGatewayHasMatchingGatewayClass(obj client.Object
// class as well. If we fail here it's most likely because of some failure
// of the Kubernetes API and it's technically better to enqueue the object
// than to drop it for eventual consistency during cluster outages.
return !errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{})
return !errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) &&
!errors.As(err, &operatorerrors.ErrNotAcceptedGatewayClass{})
}

return true
Expand Down
21 changes: 21 additions & 0 deletions internal/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package errors
import (
"errors"
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// -----------------------------------------------------------------------------
Expand All @@ -23,14 +25,33 @@ type ErrUnsupportedGatewayClass struct {
reason string
}

// NewErrUnsupportedGateway creates a new ErrUnsupportedGatewayClass error
func NewErrUnsupportedGateway(reason string) ErrUnsupportedGatewayClass {
return ErrUnsupportedGatewayClass{reason: reason}
}

// Error returns the error message for the ErrUnsupportedGatewayClass error
func (e ErrUnsupportedGatewayClass) Error() string {
return fmt.Sprintf("unsupported gateway class: %s", e.reason)
}

// ErrNotAcceptedGatewayClass is an error which indicates that a provided GatewayClass
// is not accepted.
type ErrNotAcceptedGatewayClass struct {
gatewayClass string
condition metav1.Condition
}

// NewErrNotAcceptedGatewayClass creates a new ErrNotAcceptedGatewayClass error
func NewErrNotAcceptedGatewayClass(gatewayClass string, condition metav1.Condition) ErrNotAcceptedGatewayClass {
return ErrNotAcceptedGatewayClass{gatewayClass: gatewayClass, condition: condition}
}

// Error returns the error message for the ErrNotAcceptedGatewayClass error
func (e ErrNotAcceptedGatewayClass) Error() string {
return fmt.Sprintf("gateway class %s not accepted; reason: %s, message: %s", e.gatewayClass, e.condition.Reason, e.condition.Message)
}

// -----------------------------------------------------------------------------
// GatewayClass - Errors
// -----------------------------------------------------------------------------
Expand Down
Loading

0 comments on commit ac21d78

Please sign in to comment.