Skip to content

Commit

Permalink
Review feedbacks
Browse files Browse the repository at this point in the history 
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
  • Loading branch information
@mlavacca
mlavacca committed Aug 12, 2022
1 parent 92a38f5 commit c22f7fb
Show file tree
Hide file tree
Showing 14 changed files with 141 additions and 83 deletions.
6 changes: 4 additions & 2 deletions examples/gateway-httproute.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,10 @@ spec:
value: /httproute-testing
backendRefs:
- name: httpbin
port: 80
kind: Service
port: 80
weight: 75
- name: nginx
port: 8080
kind: Service
port: 8080
weight: 25
9 changes: 5 additions & 4 deletions internal/controllers/gateway/gateway_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/kong/kubernetes-ingress-controller/v2/internal/annotations"
"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)

func Test_readyConditionExistsForObservedGeneration(t *testing.T) {
Expand Down Expand Up @@ -523,7 +524,7 @@ func Test_getReferenceGrantConditionReason(t *testing.T) {
name: "no need for reference",
gatewayNamespace: "test",
certRef: gatewayv1alpha2.SecretObjectReference{
Kind: (*gatewayv1alpha2.Kind)(pointer.StringPtr("secret")),
Kind: util.StringToGatewayAPIKindPtr("Secret"),
Name: "testSecret",
},
expectedReason: string(gatewayv1alpha2.ListenerReasonResolvedRefs),
Expand All @@ -532,7 +533,7 @@ func Test_getReferenceGrantConditionReason(t *testing.T) {
name: "reference not granted",
gatewayNamespace: "test",
certRef: gatewayv1alpha2.SecretObjectReference{
Kind: (*gatewayv1alpha2.Kind)(pointer.StringPtr("secret")),
Kind: util.StringToGatewayAPIKindPtr("Secret"),
Name: "testSecret",
Namespace: (*gatewayv1alpha2.Namespace)(pointer.StringPtr("otherNamespace")),
},
Expand Down Expand Up @@ -565,7 +566,7 @@ func Test_getReferenceGrantConditionReason(t *testing.T) {
name: "reference granted, secret name not specified",
gatewayNamespace: "test",
certRef: gatewayv1alpha2.SecretObjectReference{
Kind: (*gatewayv1alpha2.Kind)(pointer.StringPtr("secret")),
Kind: util.StringToGatewayAPIKindPtr("Secret"),
Name: "testSecret",
Namespace: (*gatewayv1alpha2.Namespace)(pointer.StringPtr("otherNamespace")),
},
Expand Down Expand Up @@ -604,7 +605,7 @@ func Test_getReferenceGrantConditionReason(t *testing.T) {
name: "reference granted, secret name specified",
gatewayNamespace: "test",
certRef: gatewayv1alpha2.SecretObjectReference{
Kind: (*gatewayv1alpha2.Kind)(pointer.StringPtr("secret")),
Kind: util.StringToGatewayAPIKindPtr("Secret"),
Name: "testSecret",
Namespace: (*gatewayv1alpha2.Namespace)(pointer.StringPtr("otherNamespace")),
},
Expand Down
66 changes: 43 additions & 23 deletions internal/controllers/gateway/httproute_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont
gatewayParentStatus := &gatewayv1alpha2.RouteParentStatus{
ParentRef: gatewayv1alpha2.ParentReference{
Group: (*gatewayv1alpha2.Group)(&gatewayv1alpha2.GroupVersion.Group),
Kind: (*gatewayv1alpha2.Kind)(&httprouteParentKind),
Kind: util.StringToGatewayAPIKindPtr(httprouteParentKind),
Namespace: (*gatewayv1alpha2.Namespace)(&gateway.gateway.Namespace),
Name: gatewayv1alpha2.ObjectName(gateway.gateway.Name),
},
Expand Down Expand Up @@ -422,17 +422,16 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont
statusChangesWereMade = true
}

/// TODO: move this condition BELOW
// if we didn't have to actually make any changes, no status update is needed
if !statusChangesWereMade {
return false, nil
}

parentStatuses, err := r.setRouteConditionResolvedRefsCondition(ctx, httproute, parentStatuses)
parentStatuses, resolvedRefsChanged, err := r.setRouteConditionResolvedRefsCondition(ctx, httproute, parentStatuses)
if err != nil {
return false, err
}

// if we didn't have to actually make any changes, no status update is needed
if !statusChangesWereMade && !resolvedRefsChanged {
return false, nil
}

// update the httproute status with the new status references
httproute.Status.Parents = make([]gatewayv1alpha2.RouteParentStatus, 0, len(parentStatuses))
for _, parent := range parentStatuses {
Expand Down Expand Up @@ -477,26 +476,42 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusRemoved(ctx context.Co
}

// setRouteConditionResolvedRefsCondition sets a condition of type ResolvedRefs on the route status.
func (r *HTTPRouteReconciler) setRouteConditionResolvedRefsCondition(ctx context.Context, httpRoute *gatewayv1alpha2.HTTPRoute, parentStatuses map[string]*gatewayv1alpha2.RouteParentStatus) (map[string]*gatewayv1alpha2.RouteParentStatus, error) {
func (r *HTTPRouteReconciler) setRouteConditionResolvedRefsCondition(ctx context.Context, httpRoute *gatewayv1alpha2.HTTPRoute, parentStatuses map[string]*gatewayv1alpha2.RouteParentStatus) (map[string]*gatewayv1alpha2.RouteParentStatus, bool, error) {
var changed bool
resolvedRefsStatus := metav1.ConditionFalse
reason, err := r.getHTTPRouteRuleReason(ctx, *httpRoute)
if err != nil {
return nil, err
return nil, false, err
}
if reason == "" {
return parentStatuses, nil
if reason == gatewayv1alpha2.RouteReasonResolvedRefs {
resolvedRefsStatus = metav1.ConditionTrue
}

// iterate over all the parentStatuses conditions, and if no RouteConditionResolvedRefs is found,
// or if the condition is found but has to be changed, update the status and mark it to be updated
for _, parentStatus := range parentStatuses {
parentStatus.Conditions = append(parentStatus.Conditions, metav1.Condition{
Type: string(gatewayv1alpha2.RouteConditionResolvedRefs),
Status: metav1.ConditionFalse,
ObservedGeneration: httpRoute.Generation,
LastTransitionTime: metav1.Now(),
Reason: string(reason),
})
var conditionFound bool
for _, cond := range parentStatus.Conditions {
if cond.Type == string(gatewayv1alpha2.RouteConditionResolvedRefs) &&
cond.Status == resolvedRefsStatus &&
cond.Reason == string(reason) {
conditionFound = true
break
}
}
if !conditionFound {
parentStatus.Conditions = append(parentStatus.Conditions, metav1.Condition{
Type: string(gatewayv1alpha2.RouteConditionResolvedRefs),
Status: resolvedRefsStatus,
ObservedGeneration: httpRoute.Generation,
LastTransitionTime: metav1.Now(),
Reason: string(reason),
})
changed = true
}
}

return parentStatuses, nil
return parentStatuses, changed, nil
}

func (r *HTTPRouteReconciler) getHTTPRouteRuleReason(ctx context.Context, httpRoute gatewayv1alpha2.HTTPRoute) (gatewayv1alpha2.RouteConditionReason, error) {
Expand Down Expand Up @@ -533,13 +548,18 @@ func (r *HTTPRouteReconciler) getHTTPRouteRuleReason(ctx context.Context, httpRo
if len(referenceGrantList.Items) == 0 {
return gatewayv1alpha2.RouteReasonRefNotPermitted, nil
}
var isGranted bool
for _, grant := range referenceGrantList.Items {
if !isHTTPReferenceGranted(grant.Spec, backendRef, httpRoute.Namespace) {
return gatewayv1alpha2.RouteReasonRefNotPermitted, nil
if isHTTPReferenceGranted(grant.Spec, backendRef, httpRoute.Namespace) {
isGranted = true
break
}
}
if !isGranted {
return gatewayv1alpha2.RouteReasonRefNotPermitted, nil
}
}
}
}
return "", nil
return gatewayv1alpha2.RouteReasonResolvedRefs, nil
}
11 changes: 6 additions & 5 deletions internal/controllers/gateway/route_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ func listenerHostnameIntersectWithRouteHostnames(listener gatewayv1alpha2.Listen
return false
}

// filterHostnames accepts a HTTPRoute and returns a version of the same object with only a subset of the
// hostnames, the ones matching with the listeners' hostname.
func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewayv1alpha2.HTTPRoute) *gatewayv1alpha2.HTTPRoute {
filteredHostnames := make([]gatewayv1alpha2.Hostname, 0)

Expand All @@ -224,8 +226,7 @@ func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewa
if len(httpRoute.Spec.Hostnames) == 0 {
for _, gateway := range gateways {
for _, listener := range gateway.gateway.Spec.Listeners {
if gatewayv1alpha2.SectionName(gateway.listenerName) == "" ||
gatewayv1alpha2.SectionName(gateway.listenerName) == listener.Name {
if listenerName := gatewayv1alpha2.SectionName(gateway.listenerName); listenerName == "" || listenerName == listener.Name {
if listener.Hostname != nil {
filteredHostnames = append(filteredHostnames, *listener.Hostname)
}
Expand All @@ -246,10 +247,10 @@ func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewa

// getMinimumHostnameIntersection returns the minimum intersecting namespace, in the sense that:
//
// - if the listener hostname is empty, returns the httpRoute hostname
// - if the listener hostname is empty, return the httpRoute hostname
// - if the listener hostname acts as a wildcard for the httpRoute hostname, return the httpRoute hostname
// - if the httpRoute hostname acts as a wildcard for the listener hostname, return the listener hostname
// - if the httpRoute hostname is the same of the listener hostname, returns it
// - if the httpRoute hostname is the same of the listener hostname, return it
// - if none of the above is true, return an empty string.
func getMinimumHostnameIntersection(gateways []supportedGatewayWithCondition, hostname gatewayv1alpha2.Hostname) gatewayv1alpha2.Hostname {
for _, gateway := range gateways {
Expand Down Expand Up @@ -281,7 +282,7 @@ func isRouteAccepted(gateways []supportedGatewayWithCondition) bool {
return false
}

// isHTTPReferenceGranted checks that the backendRef referenced by the HTTPRoute is granted by the a specific ReferenceGrant.
// isHTTPReferenceGranted checks that the backendRef referenced by the HTTPRoute is granted by a ReferenceGrant.
func isHTTPReferenceGranted(grantSpec gatewayv1alpha2.ReferenceGrantSpec, backendRef gatewayv1alpha2.HTTPBackendRef, fromNamespace string) bool {
var backendRefGroup gatewayv1alpha2.Group
var backendRefKind gatewayv1alpha2.Kind
Expand Down
47 changes: 24 additions & 23 deletions internal/controllers/gateway/route_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/utils/pointer"
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)

func Test_filterHostnames(t *testing.T) {
Expand All @@ -14,15 +15,15 @@ func Test_filterHostnames(t *testing.T) {
Listeners: []gatewayv1alpha2.Listener{
{
Name: "listener-1",
Hostname: (*gatewayv1alpha2.Hostname)(pointer.StringPtr("very.specific.com")),
Hostname: util.StringToGatewayAPIHostnamePtr("very.specific.com"),
},
{
Name: "listener-2",
Hostname: (*gatewayv1alpha2.Hostname)(pointer.StringPtr("*.wildcard.io")),
Hostname: util.StringToGatewayAPIHostnamePtr("*.wildcard.io"),
},
{
Name: "listener-3",
Hostname: (*gatewayv1alpha2.Hostname)(pointer.StringPtr("*.anotherwildcard.io")),
Hostname: util.StringToGatewayAPIHostnamePtr("*.anotherwildcard.io"),
},
},
},
Expand All @@ -45,16 +46,16 @@ func Test_filterHostnames(t *testing.T) {
httpRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("*.anotherwildcard.io"),
(gatewayv1alpha2.Hostname)("*.nonmatchingwildcard.io"),
(gatewayv1alpha2.Hostname)("very.specific.com"),
util.StringToGatewayAPIHostname("*.anotherwildcard.io"),
util.StringToGatewayAPIHostname("*.nonmatchingwildcard.io"),
util.StringToGatewayAPIHostname("very.specific.com"),
},
},
},
expectedHTTPRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("very.specific.com"),
util.StringToGatewayAPIHostname("very.specific.com"),
},
},
},
Expand All @@ -70,15 +71,15 @@ func Test_filterHostnames(t *testing.T) {
httpRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("non.matching.com"),
(gatewayv1alpha2.Hostname)("*.specific.com"),
util.StringToGatewayAPIHostname("non.matching.com"),
util.StringToGatewayAPIHostname("*.specific.com"),
},
},
},
expectedHTTPRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("very.specific.com"),
util.StringToGatewayAPIHostname("very.specific.com"),
},
},
},
Expand All @@ -94,20 +95,20 @@ func Test_filterHostnames(t *testing.T) {
httpRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("non.matching.com"),
(gatewayv1alpha2.Hostname)("wildcard.io"),
(gatewayv1alpha2.Hostname)("foo.wildcard.io"),
(gatewayv1alpha2.Hostname)("bar.wildcard.io"),
(gatewayv1alpha2.Hostname)("foo.bar.wildcard.io"),
util.StringToGatewayAPIHostname("non.matching.com"),
util.StringToGatewayAPIHostname("wildcard.io"),
util.StringToGatewayAPIHostname("foo.wildcard.io"),
util.StringToGatewayAPIHostname("bar.wildcard.io"),
util.StringToGatewayAPIHostname("foo.bar.wildcard.io"),
},
},
},
expectedHTTPRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("foo.wildcard.io"),
(gatewayv1alpha2.Hostname)("bar.wildcard.io"),
(gatewayv1alpha2.Hostname)("foo.bar.wildcard.io"),
util.StringToGatewayAPIHostname("foo.wildcard.io"),
util.StringToGatewayAPIHostname("bar.wildcard.io"),
util.StringToGatewayAPIHostname("foo.bar.wildcard.io"),
},
},
},
Expand All @@ -123,14 +124,14 @@ func Test_filterHostnames(t *testing.T) {
httpRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("*.anotherwildcard.io"),
util.StringToGatewayAPIHostname("*.anotherwildcard.io"),
},
},
},
expectedHTTPRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("*.anotherwildcard.io"),
util.StringToGatewayAPIHostname("*.anotherwildcard.io"),
},
},
},
Expand All @@ -145,8 +146,8 @@ func Test_filterHostnames(t *testing.T) {
httpRoute: &gatewayv1alpha2.HTTPRoute{
Spec: gatewayv1alpha2.HTTPRouteSpec{
Hostnames: []gatewayv1alpha2.Hostname{
(gatewayv1alpha2.Hostname)("specific.but.wrong.com"),
(gatewayv1alpha2.Hostname)("wildcard.io"),
util.StringToGatewayAPIHostname("specific.but.wrong.com"),
util.StringToGatewayAPIHostname("wildcard.io"),
},
},
},
Expand Down
3 changes: 2 additions & 1 deletion internal/controllers/gateway/tcproute_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane"
"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -371,7 +372,7 @@ func (r *TCPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte
gatewayParentStatus := &gatewayv1alpha2.RouteParentStatus{
ParentRef: gatewayv1alpha2.ParentReference{
Group: (*gatewayv1alpha2.Group)(&gatewayv1alpha2.GroupVersion.Group),
Kind: (*gatewayv1alpha2.Kind)(&tcprouteParentKind),
Kind: util.StringToGatewayAPIKindPtr(tcprouteParentKind),
Namespace: (*gatewayv1alpha2.Namespace)(&gateway.gateway.Namespace),
Name: gatewayv1alpha2.ObjectName(gateway.gateway.Name),
},
Expand Down
3 changes: 2 additions & 1 deletion internal/controllers/gateway/tlsroute_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane"
"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -371,7 +372,7 @@ func (r *TLSRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte
gatewayParentStatus := &gatewayv1alpha2.RouteParentStatus{
ParentRef: gatewayv1alpha2.ParentReference{
Group: (*gatewayv1alpha2.Group)(&gatewayv1alpha2.GroupVersion.Group),
Kind: (*gatewayv1alpha2.Kind)(&tlsrouteParentKind),
Kind: util.StringToGatewayAPIKindPtr(tlsrouteParentKind),
Namespace: (*gatewayv1alpha2.Namespace)(&gateway.gateway.Namespace),
Name: gatewayv1alpha2.ObjectName(gateway.gateway.Name),
},
Expand Down
3 changes: 2 additions & 1 deletion internal/controllers/gateway/udproute_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane"
"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
)

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -371,7 +372,7 @@ func (r *UDPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte
gatewayParentStatus := &gatewayv1alpha2.RouteParentStatus{
ParentRef: gatewayv1alpha2.ParentReference{
Group: (*gatewayv1alpha2.Group)(&gatewayv1alpha2.GroupVersion.Group),
Kind: (*gatewayv1alpha2.Kind)(&udprouteParentKind),
Kind: util.StringToGatewayAPIKindPtr(udprouteParentKind),
Namespace: (*gatewayv1alpha2.Namespace)(&gateway.gateway.Namespace),
Name: gatewayv1alpha2.ObjectName(gateway.gateway.Name),
},
Expand Down
Loading

0 comments on commit c22f7fb

Please sign in to comment.