Skip to content

Commit

Permalink
followup changes from #685 (#703)
Browse files Browse the repository at this point in the history 
* test cleanup

* addressing PR comments from #685

* fix error message style

* cleanup old comment

* import defaultsystem correctly

* refactor: condense lb lookup code

* fix logic

* earlier refactor messed up how things errored

* improve test coverage

* explain e2e test running serially

* refactor: split lb status code up

* fix seg violation in test

* refactor: assume single gateway

* code in config only allows a single Gateway, and if none are provided, it will use the defaults.

* address nits

* refactor: don't error if gateway not found

* don't want to trigger re-reconciling if it doesn't exist

* mark ingress as failed when cant find gateway

* marking as not ready was overwriting the failed status

* fix lint errors
  • Loading branch information
@KauzClay
KauzClay authored Apr 23, 2024
1 parent ad40832 commit 9e4ec34
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 140 deletions.
131 changes: 74 additions & 57 deletions pkg/reconciler/ingress/ingress.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,20 @@ import (
)

const (
notReconciledReason = "ReconcileIngressFailed"
notReconciledMessage = "Ingress reconciliation failed"
notReconciledReason = "ReconcileIngressFailed"
notReconciledMessage = "Ingress reconciliation failed"
gatewayNotFoundMessage = "could not find Gateway"
)

type GatewayNotFoundError struct {
NamespacedGatewayName string
Err error
}

func (gnfe *GatewayNotFoundError) Error() string {
return fmt.Sprintf("%s %s: %s", gatewayNotFoundMessage, gnfe.NamespacedGatewayName, gnfe.Err.Error())
}

// Reconciler implements controller.Reconciler for Route resources.
type Reconciler struct {
statusManager status.Manager
Expand Down Expand Up @@ -148,83 +158,90 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress

// TODO: check Gateway readiness before reporting Ingress ready
if routesReady {
var publicLbs, privateLbs []v1alpha1.LoadBalancerIngressStatus

externalGateway := pluginConfig.ExternalGateway()
localGateway := pluginConfig.LocalGateway()

publicLbs, err = c.determineLoadBalancerIngressStatus(externalGateway)
externalLBs, internalLBs, err := c.lookUpLoadBalancers(ing, pluginConfig)
if err != nil {
if apierrs.IsNotFound(err) {
ing.Status.MarkLoadBalancerFailed(
"GatewayDoesNotExist",
fmt.Sprintf(
"could not find Gateway %s/%s",
externalGateway.Namespace,
externalGateway.Name,
),
)
return fmt.Errorf("Gateway %s does not exist: %w", externalGateway.Name, err) //nolint:stylecheck
}
ing.Status.MarkLoadBalancerNotReady()
return err
}

privateLbs, err = c.determineLoadBalancerIngressStatus(localGateway)
if err != nil {
if apierrs.IsNotFound(err) {
ing.Status.MarkLoadBalancerFailed(
"GatewayDoesNotExist",
fmt.Sprintf(
"could not find Gateway %s/%s",
localGateway.Namespace,
localGateway.Name,
),
)
return fmt.Errorf("Gateway %s does not exist: %w", localGateway.Name, err) //nolint:stylecheck
// TODO: use errors.Is instead
if _, ok := err.(*GatewayNotFoundError); ok { //nolint
// if we can't find a Gateway, we mark it as failed, and
// return no error, since there is no point in retrying
return nil
}
ing.Status.MarkLoadBalancerNotReady()
return err
}

ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs)
ing.Status.MarkLoadBalancerReady(externalLBs, internalLBs)
} else {
ing.Status.MarkLoadBalancerNotReady()
}

return nil
}

// determineLoadBalancerIngressStatus will return the address for the Gateway.
// If a service is provided, it will return the address of the service.
// Otherwise, it will return the first address in the Gateway status.
func (c *Reconciler) determineLoadBalancerIngressStatus(gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) {
if gwc.Service != nil {
return []v1alpha1.LoadBalancerIngressStatus{
{DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace)},
}, nil
// lookUpLoadBalancers will return a map of visibilites to
// LoadBalancerIngressStatuses for the current Gateways in use.
func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.GatewayPlugin) ([]v1alpha1.LoadBalancerIngressStatus, []v1alpha1.LoadBalancerIngressStatus, error) {
externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateway())
if err != nil {
return nil, nil, err
}
gw, err := c.gatewayLister.Gateways(gwc.Namespace).Get(gwc.Name)

internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateway())
if err != nil {
return nil, err
return nil, nil, err
}

var lbis v1alpha1.LoadBalancerIngressStatus
return externalStatuses, internalStatuses, nil
}

// collectLBIngressStatus will return LoadBalancerIngressStatuses for the
// provided single Gateway config. If a service is available on a Gateway, it will
// return the address of the service. Otherwise, it will return the first
// address in the Gateway status.
func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) {
statuses := []v1alpha1.LoadBalancerIngressStatus{}

if len(gw.Status.Addresses) > 0 {
switch *gw.Status.Addresses[0].Type {
case gatewayapi.IPAddressType:
lbis = v1alpha1.LoadBalancerIngressStatus{IP: gw.Status.Addresses[0].Value}
default:
// Should this actually be under Domain? It seems like the rest of the code expects DomainInternal though...
lbis = v1alpha1.LoadBalancerIngressStatus{DomainInternal: gw.Status.Addresses[0].Value}
// TODO: currently only 1 gateway is supported. When the config is updated to
// support multiple, this code must change to find out which Gateway is
// appropriate for the given Ingress
if gwc.Service != nil {
statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{
DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace),
})
} else {
gw, err := c.gatewayLister.Gateways(gwc.Namespace).Get(gwc.Name)
if err != nil {
if apierrs.IsNotFound(err) {
ing.Status.MarkLoadBalancerFailed(
"GatewayDoesNotExist",
fmt.Sprintf(
"could not find Gateway %s/%s",
gwc.Namespace,
gwc.Name,
),
)
return nil, &GatewayNotFoundError{
NamespacedGatewayName: gwc.Namespace + "/" + gwc.Name,
Err: err,
}
}
return nil, fmt.Errorf("failed to get Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err)
}

return []v1alpha1.LoadBalancerIngressStatus{lbis}, nil
if len(gw.Status.Addresses) > 0 {
switch *gw.Status.Addresses[0].Type {
case gatewayapi.IPAddressType:
statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{IP: gw.Status.Addresses[0].Value})
default:
// Should this actually be under Domain? It seems like the rest of the code expects DomainInternal though...
statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{DomainInternal: gw.Status.Addresses[0].Value})
}
} else {
return nil, fmt.Errorf("no address found in status of Gateway %s/%s", gwc.Namespace, gwc.Name)
}
}

return nil, fmt.Errorf("Gateway %q does not have an address in status", gwc.NamespacedName) //nolint:stylecheck

return statuses, nil
}

// isHTTPRouteReady will check the status conditions of the ingress and return true if
Expand Down
89 changes: 78 additions & 11 deletions pkg/reconciler/ingress/ingress_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ var (
fakeStatusKey struct{}

publicGatewayAddress = "11.22.33.44"
publicGatewayHostname = "off.cluster.gateway"
privateGatewayAddress = "55.66.77.88"
)

Expand Down Expand Up @@ -2233,12 +2234,32 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) {
Objects: append([]runtime.Object{
ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
gw(defaultListener, setStatusPublicAddress),
gw(defaultListener, setStatusPublicAddressIP),
gw(privateGw, defaultListener, setStatusPrivateAddress),
}, servicesAndEndpoints...),
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGateway)},
},
}, {
Name: "gateway has hostname in address",
Key: "ns/name",
Ctx: withStatusManager(&fakeStatusManager{
FakeDoProbes: func(context.Context, status.Backends) (status.ProbeState, error) {
return status.ProbeState{Ready: true}, nil
},
FakeIsProbeActive: func(types.NamespacedName) (status.ProbeState, bool) {
return status.ProbeState{Ready: true}, true
},
}),
Objects: append([]runtime.Object{
ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
gw(defaultListener, setStatusPublicAddressHostname),
gw(privateGw, defaultListener, setStatusPrivateAddress),
}, servicesAndEndpoints...),
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGatewayHostname)},
},
}, {
Name: "gateway not ready",
Key: "ns/name",
Expand All @@ -2258,18 +2279,45 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) {
}, servicesAndEndpoints...),
WantErr: true,
WantStatusUpdates: []clientgotesting.UpdateActionImpl{
//{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGateway)},
{Object: ing(withBasicSpec, withGatewayAPIClass, withFinalizer, func(i *v1alpha1.Ingress) {
i.Status.InitializeConditions()
i.Status.MarkLoadBalancerNotReady()
i.Status.MarkNetworkConfigured()
i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed")
}),
},
{Object: ing(
withBasicSpec,
withGatewayAPIClass,
withFinalizer,
func(i *v1alpha1.Ingress) {
i.Status.InitializeConditions()
i.Status.MarkLoadBalancerNotReady()
i.Status.MarkNetworkConfigured()
i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed")
},
)},
},
WantEvents: []string{
Eventf(corev1.EventTypeWarning, "InternalError", `Gateway "istio-system/istio-gateway" does not have an address in status`),
Eventf(corev1.EventTypeWarning, "InternalError", `no address found in status of Gateway istio-system/istio-gateway`),
},
}, {
Name: "gateway doesn't exist",
Key: "ns/name",
Ctx: withStatusManager(&fakeStatusManager{
FakeDoProbes: func(context.Context, status.Backends) (status.ProbeState, error) {
return status.ProbeState{Ready: true}, nil
},
FakeIsProbeActive: func(types.NamespacedName) (status.ProbeState, bool) {
return status.ProbeState{Ready: true}, true
},
}),
Objects: append([]runtime.Object{
ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
}, servicesAndEndpoints...),
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{Object: ing(
withBasicSpec,
withGatewayAPIClass,
withFinalizer,
func(i *v1alpha1.Ingress) {
i.Status.InitializeConditions()
i.Status.MarkLoadBalancerFailed("GatewayDoesNotExist", "could not find Gateway istio-system/istio-gateway")
i.Status.MarkNetworkConfigured()
})}},
}}

table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
Expand Down Expand Up @@ -2302,6 +2350,18 @@ func makeItReadyOffClusterGateway(i *v1alpha1.Ingress) {
}})
}

func makeItReadyOffClusterGatewayHostname(i *v1alpha1.Ingress) {
i.Status.InitializeConditions()
i.Status.MarkNetworkConfigured()
i.Status.MarkLoadBalancerReady(
[]v1alpha1.LoadBalancerIngressStatus{{
DomainInternal: publicGatewayHostname,
}},
[]v1alpha1.LoadBalancerIngressStatus{{
IP: privateGatewayAddress,
}})
}

func makeItReady(i *v1alpha1.Ingress) {
i.Status.InitializeConditions()
i.Status.MarkNetworkConfigured()
Expand Down Expand Up @@ -2416,13 +2476,20 @@ func setStatusPrivateAddress(g *gatewayapi.Gateway) {
})
}

func setStatusPublicAddress(g *gatewayapi.Gateway) {
func setStatusPublicAddressIP(g *gatewayapi.Gateway) {
g.Status.Addresses = append(g.Status.Addresses, gatewayapi.GatewayStatusAddress{
Type: ptr.To[gatewayapi.AddressType](gatewayapi.IPAddressType),
Value: publicGatewayAddress,
})
}

func setStatusPublicAddressHostname(g *gatewayapi.Gateway) {
g.Status.Addresses = append(g.Status.Addresses, gatewayapi.GatewayStatusAddress{
Type: ptr.To[gatewayapi.AddressType](gatewayapi.HostnameAddressType),
Value: publicGatewayHostname,
})
}

func tlsListener(hostname, nsName, secretName string) GatewayOption {
return func(g *gatewayapi.Gateway) {
g.Spec.Listeners = append(g.Spec.Listeners, gatewayapi.Listener{
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/ingress/lister.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (l *gatewayPodTargetLister) BackendsToProbeTargets(ctx context.Context, bac
}

if len(gw.Status.Addresses) == 0 {
return nil, fmt.Errorf("no Addresses available in Status of Gateway %s/%s", gw.Namespace, gw.Name)
return nil, fmt.Errorf("no addresses available in status of Gateway %s/%s", gw.Namespace, gw.Name)
}

pt := status.ProbeTarget{
Expand Down
8 changes: 4 additions & 4 deletions pkg/reconciler/ingress/lister_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
},
},
objects: []runtime.Object{
gw(defaultListener, setStatusPublicAddress),
gw(defaultListener, setStatusPublicAddressIP),
},
ing: ing(withBasicSpec, withGatewayAPIClass),
want: []status.ProbeTarget{
Expand All @@ -337,7 +337,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
name: "gateway has tls listener (http enabled)",
objects: []runtime.Object{
// objects for secret and referenceGrant not needed in this test
gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddress),
gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddressIP),
},
backends: status.Backends{
URLs: map[v1alpha1.IngressVisibility]status.URLSet{
Expand All @@ -362,7 +362,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
name: "gateway has tls listener (https redirected)",
objects: []runtime.Object{
// objects for secret and referenceGrant not needed in this test
gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddress),
gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddressIP),
},
backends: status.Backends{
HTTPOption: v1alpha1.HTTPOptionRedirected,
Expand Down Expand Up @@ -399,7 +399,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
},
},
ing: ing(withBasicSpec, withGatewayAPIClass, withHTTPOption(v1alpha1.HTTPOptionRedirected)),
wantErr: fmt.Errorf("no Addresses available in Status of Gateway istio-system/istio-gateway"),
wantErr: fmt.Errorf("no addresses available in status of Gateway istio-system/istio-gateway"),
}}

for _, test := range tests {
Expand Down
2 changes: 2 additions & 0 deletions test/e2e-common.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ function gateway_conformance() {
}

function test_e2e() {
# TestGatewayWithNoService edits the ConfigMap, so running in parallel could cause
# unexpected problems when more tests are added.
local parallel_count="1"


Expand Down
Loading

0 comments on commit 9e4ec34

Please sign in to comment.