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.

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

Jump to bottom

fix: Return LB status for each gateway #1218

Closed
wants to merge 1 commit into from
Closed

fix: Return LB status for each gateway #1218

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
61 changes: 37 additions & 24 deletions pkg/reconciler/ingress/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,16 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
}

if ready {
publicLbs := getLBStatus(publicGatewayServiceURLFromContext(ctx))
privateLbs := getLBStatus(privateGatewayServiceURLFromContext(ctx))
publicLbs, err := getLBStatuses(ctx, v1alpha1.IngressVisibilityExternalIP)
if err != nil {
return fmt.Errorf("failed to get external LB status: %w", err)
}

privateLbs, err := getLBStatuses(ctx, v1alpha1.IngressVisibilityClusterLocal)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding, this will set multiple gateways to the LB status when users configured multiple gateways. Did that work with serving? For example, here is the code which disallows it -
https://github.com/knative/serving/blob/9b0d1851055e91ced3965dc3cb8d9edb08f833ee/pkg/reconciler/route/resources/service.go#L87-L91

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I wasn't aware of such limitation at serving side.

Small remark tho, we can have multiple istio gateways that point to the same istio ingress gateway (and therefore LB)

if err != nil {
return fmt.Errorf("failed to get local LB status: %w", err)
}

ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs)
} else {
ing.Status.MarkLoadBalancerNotReady()
Expand Down Expand Up @@ -511,34 +519,39 @@ func qualifiedGatewayNamesFromContext(ctx context.Context) map[v1alpha1.IngressV
}
}

func publicGatewayServiceURLFromContext(ctx context.Context) string {
cfg := config.FromContext(ctx).Istio
if len(cfg.IngressGateways) > 0 {
return cfg.IngressGateways[0].ServiceURL
}
return ""
}
// getLBStatuses gets the LB Statuses.
func getLBStatuses(ctx context.Context, visibility v1alpha1.IngressVisibility) ([]v1alpha1.LoadBalancerIngressStatus, error) {
ret := make([]v1alpha1.LoadBalancerIngressStatus, 0)

func privateGatewayServiceURLFromContext(ctx context.Context) string {
cfg := config.FromContext(ctx).Istio
if len(cfg.LocalGateways) > 0 {
return cfg.LocalGateways[0].ServiceURL
var gateways []config.Gateway

switch visibility {
case v1alpha1.IngressVisibilityClusterLocal:
gateways = cfg.LocalGateways
case v1alpha1.IngressVisibilityExternalIP:
gateways = cfg.IngressGateways
default:
return nil, fmt.Errorf("unknown visibility: %s", visibility)
}
return ""
}

// getLBStatus gets the LB Status.
func getLBStatus(gatewayServiceURL string) []v1alpha1.LoadBalancerIngressStatus {
// The Ingress isn't load-balanced by any particular
// Service, but through a Service mesh.
if gatewayServiceURL == "" {
return []v1alpha1.LoadBalancerIngressStatus{
{MeshOnly: true},
for _, gateway := range gateways {
// The Ingress isn't load-balanced by any particular
// Service, but through a Service mesh.
if gateway.ServiceURL == "" {
ret = append(ret, v1alpha1.LoadBalancerIngressStatus{
MeshOnly: true,
})

continue
}

ret = append(ret, v1alpha1.LoadBalancerIngressStatus{
DomainInternal: gateway.ServiceURL,
})
}
return []v1alpha1.LoadBalancerIngressStatus{
{DomainInternal: gatewayServiceURL},
}

return ret, nil
}

func shouldReconcileTLS(ing *v1alpha1.Ingress) bool {
Expand Down
Loading