-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[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.
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
[feat gw api] Implement Gateway Status + Some bug fixes for Gateway #4189
Conversation
d91565a
to
d2b5899
Compare
d2b5899
to
030a637
Compare
1812d4b
to
459977e
Compare
@@ -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(err, "Unable to update gateway status on failure to retrieve attached config") |
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.
err or statusErr?
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.
should be statusErr. thanks :)
pkg/gateway/routeutils/backend.go
Outdated
@@ -31,15 +31,17 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci | |||
|
|||
// We only support references of type service. | |||
if backendRef.Kind != nil && *backendRef.Kind != "Service" { | |||
return nil, nil | |||
initialErrorMessage := "Backend Ref must be of kind 'Service'" | |||
return nil, wrapError(errors.Errorf("%s", generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier)), gwv1.GatewayReasonListenersNotValid) |
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 reuse some of the existing error constant? e.g. gwv1.RouteReasonInvalidKind
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.
Where do you want that introduced? Sorry it was not clear to me.
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.
i was thinking this can be the returned reason. but now realize that gateway status has a different set of error reason (gwv1.GatewayReasonListenersNotValid). in this case, we might wanna think about how to handle both route and gateway status here, cuz in the same place, route status is supposed to have gwv1.RouteReasonInvalidKind. i can take gwv1.GatewayReasonListenersNotValid and map it to invalidKind if this is the only place ListenersNotValid is thrown
@@ -72,9 +75,17 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci | |||
svc := &corev1.Service{} | |||
err := k8sClient.Get(ctx, svcIdentifier, svc) | |||
if err != nil { | |||
|
|||
convertToNotFoundError := client.IgnoreNotFound(err) |
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.
gwv1.RouteReasonBackendNotFound
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.
I think that should be applied to the route condition, however the Gateway condition has it's own set of conditions to set.
7c5ad7e
to
283c5bf
Compare
@@ -86,6 +94,7 @@ func newGatewayReconciler(controllerName string, lbType elbv2model.LoadBalancerT | |||
reconcileTracker: reconcileTracker, | |||
cfgResolver: cfgResolver, | |||
routeReconciler: routeReconciler, | |||
gatewayConditionUpdater: prepareGatewayConditionUpdate, |
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.
Looks like you are calling prepareGatewayConditionUpdate
directly everywhere where you are using it. Do you want to remove it from the struct
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.
nice find, thanks. i've replaced the direct usage with calls from the struct.
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{ | ||
{ |
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.
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.
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.
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?
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.
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:
- The address value is empty OR
- 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
}
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.
🤦 wow -- thanks for catching that and the explanation.
// 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 |
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.
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
}
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.
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.
283c5bf
to
8f7a43f
Compare
8f7a43f
to
cecfe5b
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wweiwei-li, zac-nixon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Description
Implements Gateway conditions for both Programmed and Accepted. I had to refactor the route util in order to propagate the exact problem back to the gateway controller in order to properly implement the various failure causes on the Accepted condition.
I had to refactor the deployer to get back the LB status to correctly implement requeuing the Gateway in order to update the programmed condition when the LB finishes provisioning.
Added support infrastructure annotations, they get propagated to the TGB created by the Gateway.
Tests
Checklist
README.md
, or thedocs
directory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯