Skip to content

[feat gw api] Implement Gateway Status + Some bug fixes for Gateway #4189

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 29, 2025
+1,136 −65
Merged

[feat gw api] Implement Gateway Status + Some bug fixes for Gateway #4189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions controllers/gateway/config_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ func newGatewayConfigResolver() gatewayConfigResolver {
func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error) {

// If the Gateway Class isn't accepted, we shouldn't try to reconcile this Gateway.
derivedStatus, _ := deriveGatewayClassAcceptedStatus(gwClass)
derivedStatusIndx, ok := deriveAcceptedConditionIndex(gwClass)

if derivedStatus != metav1.ConditionTrue {
return elbv2gw.LoadBalancerConfiguration{}, errors.Errorf("Unable to materialize gateway when gateway class [%s] is not accepted. GatewayClass status is %s", gwClass.Name, derivedStatus)
if !ok || gwClass.Status.Conditions[derivedStatusIndx].Status != metav1.ConditionTrue {
return elbv2gw.LoadBalancerConfiguration{}, errors.Errorf("Unable to materialize gateway when gateway class [%s] is not accepted", gwClass.Name)
}

gatewayClassLBConfig, err := resolver.configResolverFn(ctx, k8sClient, gwClass.Spec.ParametersRef)
Expand Down
84 changes: 70 additions & 14 deletions controllers/gateway/gateway_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package gateway
import (
"context"
"fmt"
elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/tools/record"
Expand Down Expand Up @@ -35,6 +37,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/source"
gwv1 "sigs.k8s.io/gateway-api/apis/v1"
gwalpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
"time"
)

const (
requeueMessage = "Monitoring provisioning state"
statusUpdateRequeueTime = 2 * time.Minute
)

var _ Reconciler = &gatewayReconciler{}
Expand Down Expand Up @@ -86,6 +94,7 @@ func newGatewayReconciler(controllerName string, lbType elbv2model.LoadBalancerT
reconcileTracker: reconcileTracker,
cfgResolver: cfgResolver,
routeReconciler: routeReconciler,
gatewayConditionUpdater: prepareGatewayConditionUpdate,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you are calling prepareGatewayConditionUpdate directly everywhere where you are using it. Do you want to remove it from the struct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice find, thanks. i've replaced the direct usage with calls from the struct.

}
}

Expand All @@ -107,6 +116,7 @@ type gatewayReconciler struct {
logger logr.Logger
metricsCollector lbcmetrics.MetricCollector
reconcileTracker func(namespaceName types.NamespacedName)
gatewayConditionUpdater func(gw *gwv1.Gateway, targetConditionType string, newStatus metav1.ConditionStatus, reason string, message string) bool

cfgResolver gatewayConfigResolver
routeReconciler routeutils.RouteReconciler
Expand Down Expand Up @@ -186,12 +196,23 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R
mergedLbConfig, err := r.cfgResolver.getLoadBalancerConfigForGateway(ctx, r.k8sClient, gw, gwClass)

if err != nil {
statusErr := r.updateGatewayStatusFailure(ctx, gw, gwv1.GatewayReasonInvalid, err)
if statusErr != nil {
r.logger.Error(statusErr, "Unable to update gateway status on failure to retrieve attached config")
}
return err
}

allRoutes, err := r.gatewayLoader.LoadRoutesForGateway(ctx, *gw, r.routeFilter, r.routeReconciler)

if err != nil {
var loaderErr routeutils.LoaderError
if errors.As(err, &loaderErr) {
statusErr := r.updateGatewayStatusFailure(ctx, gw, loaderErr.GetGatewayReason(), loaderErr)
if statusErr != nil {
r.logger.Error(statusErr, "Unable to update gateway status on failure to build routes")
}
}
return err
}

Expand Down Expand Up @@ -248,18 +269,14 @@ func (r *gatewayReconciler) reconcileUpdate(ctx context.Context, gw *gwv1.Gatewa
if err != nil {
return err
}
lbDNS, err := lb.DNSName().Resolve(ctx)
if err != nil {
return err
}

if !backendSGRequired {
if err := r.backendSGProvider.Release(ctx, networking.ResourceTypeGateway, []types.NamespacedName{k8s.NamespacedName(gw)}); err != nil {
return err
}
}

if err = r.updateGatewayStatus(ctx, lbDNS, gw); err != nil {
if err = r.updateGatewayStatusSuccess(ctx, lb.Status, gw); err != nil {
r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.GatewayEventReasonFailedUpdateStatus, fmt.Sprintf("Failed update status due to %v", err))
return err
}
Expand Down Expand Up @@ -295,28 +312,58 @@ func (r *gatewayReconciler) buildModel(ctx context.Context, gw *gwv1.Gateway, cf
return stack, lb, backendSGRequired, nil
}

func (r *gatewayReconciler) updateGatewayStatus(ctx context.Context, lbDNS string, gw *gwv1.Gateway) error {
// TODO Consider LB ARN.
func (r *gatewayReconciler) updateGatewayStatusSuccess(ctx context.Context, lbStatus *elbv2model.LoadBalancerStatus, gw *gwv1.Gateway) error {
// LB Status should always be set, if it's not, we need to prevent NPE
if lbStatus == nil {
r.logger.Info("Unable to update Gateway Status due to null LB status")
return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

if lbStatus == nil, the updateGatewayStatusSuccess return nil (no error).
There's a potential issue with this part in reconcileUpdate. it will success reconcile. Should this not be expected ? How about return an error ?

if err = r.updateGatewayStatusSuccess(ctx, lb.Status, gw); err != nil {
    r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.GatewayEventReasonFailedUpdateStatus, fmt.Sprintf("Failed update status due to %v", err))
    return err
}
r.eventRecorder.Event(gw, corev1.EventTypeNormal, k8s.GatewayEventReasonSuccessfullyReconciled, "Successfully reconciled")
	return nil
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, i wasn't exactly sure what to do there. My thinking is that the lbstatus being null is a weird and unexpected condition. I'd prefer to silently fail (with a log message) rather than keep performing reconciles when the LBStatus is busted.

}
gwOld := gw.DeepCopy()

var needPatch bool
var requeueNeeded bool
if isGatewayProgrammed(*lbStatus) {
needPatch = r.gatewayConditionUpdater(gw, string(gwv1.GatewayConditionProgrammed), metav1.ConditionTrue, string(gwv1.GatewayConditionProgrammed), lbStatus.LoadBalancerARN)
} else {
requeueNeeded = true
}

// Gateway Address Status
needPatch = r.gatewayConditionUpdater(gw, string(gwv1.GatewayConditionAccepted), metav1.ConditionTrue, string(gwv1.GatewayConditionAccepted), "") || needPatch
if len(gw.Status.Addresses) != 1 ||
gw.Status.Addresses[0].Value != "" ||
gw.Status.Addresses[0].Value != lbDNS {
gwOld := gw.DeepCopy()
gw.Status.Addresses[0].Value != lbStatus.DNSName {
ipAddressType := gwv1.HostnameAddressType
gw.Status.Addresses = []gwv1.GatewayStatusAddress{
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm. Is this correct ? `gw.Status.Addresses[0].Value != "" The second condition will always be true if the address value is not empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't follow this comment, sorry.

Are you saying that if the address is not empty, then it must equal the DNS name of the LB?

Copy link
Collaborator

@wweiwei-li wweiwei-li May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I may need to understand the intent here better. I was thinking gw.Status.Addresses[0].Value cannot be simultaneously equal to both an empty string and lbStatus.DNSName. One of these conditions will always be true. Therefore, gw.Status.Addresses[0].Value != "" || gw.Status.Addresses[0].Value != lbStatus.DNSName will always be true.

Was your intent here to update the address only when necessary. More specifically, only when either:

  1. The address value is empty OR
  2. The address does not match the load balancer's DNS name
if len(gw.Status.Addresses) != 1 ||
    gw.Status.Addresses[0].Value == "" || gw.Status.Addresses[0].Value != lbStatus.DNSName) {
    ipAddressType := gwv1.HostnameAddressType
    gw.Status.Addresses = []gwv1.GatewayStatusAddress{
        {
            Type:  &ipAddressType,
            Value: lbStatus.DNSName,
        },
    }
    needPatch = true
}

Else, simply remove gw.Status.Addresses[0].Value != ""

if len(gw.Status.Addresses) != 1 ||
    gw.Status.Addresses[0].Value != lbStatus.DNSName {
    ipAddressType := gwv1.HostnameAddressType
    gw.Status.Addresses = []gwv1.GatewayStatusAddress{
        {
            Type:  &ipAddressType,
            Value: lbStatus.DNSName,
        },
    }
    needPatch = true
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦 wow -- thanks for catching that and the explanation.

Type: &ipAddressType,
Value: lbDNS,
Value: lbStatus.DNSName,
},
}
needPatch = true
}

if needPatch {
if err := r.k8sClient.Status().Patch(ctx, gw, client.MergeFrom(gwOld)); err != nil {
return errors.Wrapf(err, "failed to update gw status: %v", k8s.NamespacedName(gw))
}
}

// TODO: Listener status ListenerStatus
// https://github.com/aws/aws-application-networking-k8s/blob/main/pkg/controllers/gateway_controller.go#L350
if requeueNeeded {
return runtime.NewRequeueNeededAfter(requeueMessage, statusUpdateRequeueTime)
}

return nil
}

func (r *gatewayReconciler) updateGatewayStatusFailure(ctx context.Context, gw *gwv1.Gateway, reason gwv1.GatewayConditionReason, err error) error {
gwOld := gw.DeepCopy()

needPatch := r.gatewayConditionUpdater(gw, string(gwv1.GatewayConditionAccepted), metav1.ConditionFalse, string(reason), err.Error())

if needPatch {
if err := r.k8sClient.Status().Patch(ctx, gw, client.MergeFrom(gwOld)); err != nil {
return errors.Wrapf(err, "failed to update gw status: %v", k8s.NamespacedName(gw))
}
}

return nil
}
Expand Down Expand Up @@ -475,3 +522,12 @@ func (r *gatewayReconciler) setupNLBGatewayControllerWatches(ctrl controller.Con
return nil

}

func isGatewayProgrammed(lbStatus elbv2model.LoadBalancerStatus) bool {
if lbStatus.ProvisioningState == nil {
return false
}

return lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActive || lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActiveImpaired

}
72 changes: 66 additions & 6 deletions controllers/gateway/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,21 @@ import (
"strconv"
"strings"
"time"
"unicode/utf8"
)

const (
gatewayClassAnnotationLastProcessedConfig = "elbv2.k8s.aws/last-processed-config"
gatewayClassAnnotationLastProcessedConfigTimestamp = gatewayClassAnnotationLastProcessedConfig + "-timestamp"

// The max message that can be stored in a condition
maxMessageLength = 32700
)

// updateGatewayClassLastProcessedConfig updates the gateway class annotations with the last processed lb config resource version or "" if no lb config is attached to the gatewayclass
func updateGatewayClassLastProcessedConfig(ctx context.Context, k8sClient client.Client, gwClass *gwv1.GatewayClass, lbConf *elbv2gw.LoadBalancerConfiguration) error {

calculatedVersion := gatewayClassAnnotationLastProcessedConfig
calculatedVersion := ""

if lbConf != nil {
calculatedVersion = lbConf.ResourceVersion
Expand All @@ -36,12 +41,16 @@ func updateGatewayClassLastProcessedConfig(ctx context.Context, k8sClient client
}

gwClassOld := gwClass.DeepCopy()
if gwClass.Annotations == nil {
gwClass.Annotations = make(map[string]string)
}
gwClass.Annotations[gatewayClassAnnotationLastProcessedConfig] = calculatedVersion
gwClass.Annotations[gatewayClassAnnotationLastProcessedConfigTimestamp] = strconv.FormatInt(time.Now().Unix(), 10)

return k8sClient.Patch(ctx, gwClass, client.MergeFrom(gwClassOld))
}

// getStoredProcessedConfig retrieves the resource version attached to the lb config referenced by the gateway class or nil if no such mapping exists.
func getStoredProcessedConfig(gwClass *gwv1.GatewayClass) *string {
var storedVersion *string

Expand All @@ -54,10 +63,20 @@ func getStoredProcessedConfig(gwClass *gwv1.GatewayClass) *string {
return storedVersion
}

// updateGatewayClassAcceptedCondition updates the 'accepted' condition on the gateway class to the passed in parameters. if no 'Accepted' condition exists, do nothing.
func updateGatewayClassAcceptedCondition(ctx context.Context, k8sClient client.Client, gwClass *gwv1.GatewayClass, newStatus metav1.ConditionStatus, reason string, message string) error {
derivedStatus, indxToUpdate := deriveGatewayClassAcceptedStatus(gwClass)
indxToUpdate, ok := deriveAcceptedConditionIndex(gwClass)

if ok {

storedStatus := gwClass.Status.Conditions[indxToUpdate].Status
storedMessage := gwClass.Status.Conditions[indxToUpdate].Message
storedReason := gwClass.Status.Conditions[indxToUpdate].Reason

if storedStatus == newStatus && storedMessage == message && storedReason == reason {
return nil
}

if indxToUpdate != -1 && derivedStatus != newStatus {
gwClassOld := gwClass.DeepCopy()
gwClass.Status.Conditions[indxToUpdate].LastTransitionTime = metav1.NewTime(time.Now())
gwClass.Status.Conditions[indxToUpdate].ObservedGeneration = gwClass.Generation
Expand All @@ -71,15 +90,56 @@ func updateGatewayClassAcceptedCondition(ctx context.Context, k8sClient client.C
return nil
}

func deriveGatewayClassAcceptedStatus(gwClass *gwv1.GatewayClass) (metav1.ConditionStatus, int) {
// prepareGatewayConditionUpdate inserts the necessary data into the condition field of the gateway. The caller should patch the corresponding gateway. Returns false when no change was performed.
func prepareGatewayConditionUpdate(gw *gwv1.Gateway, targetConditionType string, newStatus metav1.ConditionStatus, reason string, message string) bool {

indxToUpdate := -1
var derivedCondition metav1.Condition
for i, condition := range gw.Status.Conditions {
if condition.Type == targetConditionType {
indxToUpdate = i
derivedCondition = condition
break
}
}

// 32768 is the max message limit
truncatedMessage := truncateMessage(message)

if indxToUpdate != -1 {
if derivedCondition.Status != newStatus || derivedCondition.Message != truncatedMessage || derivedCondition.Reason != reason {
gw.Status.Conditions[indxToUpdate].LastTransitionTime = metav1.NewTime(time.Now())
gw.Status.Conditions[indxToUpdate].ObservedGeneration = gw.Generation
gw.Status.Conditions[indxToUpdate].Status = newStatus
gw.Status.Conditions[indxToUpdate].Message = truncatedMessage
gw.Status.Conditions[indxToUpdate].Reason = reason
return true
}
}
return false
}

func truncateMessage(s string) string {
if utf8.RuneCountInString(s) <= maxMessageLength {
return s
}

runes := []rune(s)
return string(runes[:maxMessageLength]) + "..."
}

// deriveAcceptedConditionIndex returns the index of the condition pertaining to the accepted condition.
// -1 if the condition doesn't exist
func deriveAcceptedConditionIndex(gwClass *gwv1.GatewayClass) (int, bool) {
for i, v := range gwClass.Status.Conditions {
if v.Type == string(gwv1.GatewayClassReasonAccepted) {
return v.Status, i
return i, true
}
}
return metav1.ConditionFalse, -1
return -1, false
}

// resolveLoadBalancerConfig returns the lb config referenced in the ParametersReference.
func resolveLoadBalancerConfig(ctx context.Context, k8sClient client.Client, reference *gwv1.ParametersReference) (*elbv2gw.LoadBalancerConfiguration, error) {
var lbConf *elbv2gw.LoadBalancerConfiguration

Expand Down
Loading
Loading