Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

gateway-controller: watch ReferencePolicy lifecycle #207

Merged
merged 11 commits into from
Jun 2, 2022
Merged

gateway-controller: watch ReferencePolicy lifecycle #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
087a5da
Stub gateway controller watch on ReferencePolicy
nathancoleman May 31, 2022
d11b376
Reconcile all Gateways in referenced namespace on ReferencePolicy watch
nathancoleman May 31, 2022
039fcad
Regenerate mock gatewayclient
nathancoleman Jun 1, 2022
3834481
Remove unnecessary middle functions for fetching impacted xRoutes
nathancoleman Jun 1, 2022
afaf5c9
Add e2e test for ReferencePolicy lifecycle applied to Gateways
nathancoleman Jun 1, 2022
6c5c6db
Remove redundant assertion
nathancoleman Jun 2, 2022
ec4c36e
Add changelog entry
nathancoleman Jun 2, 2022
aee35c7
Use non-default namespace for Secret instead of Gateway
nathancoleman Jun 2, 2022
8c1296d
Remove inapplicable comment
nathancoleman Jun 2, 2022
a00ae89
Add comment explaining Gateway config
nathancoleman Jun 2, 2022
3f5882c
Remove debug log
nathancoleman Jun 2, 2022
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
3 changes: 3 additions & 0 deletions .changelog/207.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:enhancement
k8s/controllers: watch for ReferencePolicy changes to reconcile and revalidate affected Gateways
```
143 changes: 123 additions & 20 deletions internal/commands/server/k8s_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ import (
"sigs.k8s.io/e2e-framework/pkg/features"
gateway "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/hashicorp/consul/api"
appsv1 "k8s.io/api/apps/v1"

"github.com/hashicorp/consul-api-gateway/internal/k8s"
"github.com/hashicorp/consul-api-gateway/internal/testing/e2e"
apigwv1alpha1 "github.com/hashicorp/consul-api-gateway/pkg/apis/v1alpha1"
"github.com/hashicorp/consul/api"
appsv1 "k8s.io/api/apps/v1"
)

var (
Expand Down Expand Up @@ -83,7 +84,7 @@ func TestGatewayWithClassConfigChange(t *testing.T) {

// Create a Gateway and wait for it to be ready
firstGatewayName := envconf.RandomName("gw", 16)
firstGateway := createGateway(ctx, t, resources, firstGatewayName, gc, []gateway.Listener{httpsListener})
firstGateway := createGateway(ctx, t, resources, firstGatewayName, namespace, gc, []gateway.Listener{httpsListener})
require.Eventually(t, gatewayStatusCheck(ctx, resources, firstGatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")
checkGatewayConfigAnnotation(ctx, t, resources, firstGatewayName, namespace, firstConfig)

Expand All @@ -97,7 +98,7 @@ func TestGatewayWithClassConfigChange(t *testing.T) {

// Create a second Gateway and wait for it to be ready
secondGatewayName := envconf.RandomName("gw", 16)
secondGateway := createGateway(ctx, t, resources, secondGatewayName, gc, []gateway.Listener{httpsListener})
secondGateway := createGateway(ctx, t, resources, secondGatewayName, namespace, gc, []gateway.Listener{httpsListener})
require.Eventually(t, gatewayStatusCheck(ctx, resources, secondGatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")

// Verify that 1st Gateway retains initial GatewayClassConfig and 2nd Gateway retains updated GatewayClassConfig
Expand Down Expand Up @@ -130,7 +131,7 @@ func TestGatewayWithReplicas(t *testing.T) {

// Create a Gateway and wait for it to be ready
gatewayName := envconf.RandomName("gw", 16)
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{createHTTPSListener(ctx, t, 443)})
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{createHTTPSListener(ctx, t, 443)})
require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")
checkGatewayConfigAnnotation(ctx, t, resources, gatewayName, namespace, gcc)

Expand Down Expand Up @@ -166,7 +167,7 @@ func TestGatewayWithReplicasCanScale(t *testing.T) {

// Create a Gateway and wait for it to be ready
gatewayName := envconf.RandomName("gw", 16)
gateway := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{createHTTPSListener(ctx, t, 443)})
gateway := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{createHTTPSListener(ctx, t, 443)})
require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")
checkGatewayConfigAnnotation(ctx, t, resources, gatewayName, namespace, gcc)

Expand Down Expand Up @@ -201,8 +202,8 @@ func TestGatewayWithReplicasRespectMinMax(t *testing.T) {
var initialReplicas int32 = 3
var minReplicas int32 = 2
var maxReplicas int32 = 8
var exceedsMin int32 = minReplicas - 1
var exceedsMax int32 = maxReplicas + 1
var exceedsMin = minReplicas - 1
var exceedsMax = maxReplicas + 1
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
useHostPorts := false

// Create a GatewayClassConfig
Expand All @@ -220,11 +221,9 @@ func TestGatewayWithReplicasRespectMinMax(t *testing.T) {

// Create a Gateway and wait for it to be ready
gatewayName := envconf.RandomName("gw", 16)
gateway := createGateway(ctx, t, resources, gatewayName, gatewayClass, []gateway.Listener{httpsListener})
gateway := createGateway(ctx, t, resources, gatewayName, namespace, gatewayClass, []gateway.Listener{httpsListener})

require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")

require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), 30*time.Second, checkInterval, "no gateway found in the allotted time")
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
checkGatewayConfigAnnotation(ctx, t, resources, gatewayName, namespace, gatewayClassConfig)

// Fetch the deployment created by the gateway and check the number of replicas
Expand Down Expand Up @@ -271,7 +270,7 @@ func TestGatewayBasic(t *testing.T) {
require.Eventually(t, gatewayClassStatusCheck(ctx, resources, gc.Name, namespace, conditionAccepted), checkTimeout, checkInterval, "gatewayclass not accepted in the allotted time")

httpsListener := createHTTPSListener(ctx, t, 443)
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{httpsListener})
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{httpsListener})

require.Eventually(t, func() bool {
err := resources.Get(ctx, gatewayName, namespace, &apps.Deployment{})
Expand Down Expand Up @@ -334,7 +333,7 @@ func TestServiceListeners(t *testing.T) {
require.Eventually(t, gatewayClassStatusCheck(ctx, resources, gc.Name, namespace, conditionAccepted), checkTimeout, checkInterval, "gatewayclass not accepted in the allotted time")

httpsListener := createHTTPSListener(ctx, t, 443)
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{httpsListener})
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{httpsListener})

require.Eventually(t, func() bool {
service := &core.Service{}
Expand Down Expand Up @@ -397,7 +396,7 @@ func TestHTTPRouteFlattening(t *testing.T) {

checkPort := e2e.HTTPFlattenedPort(ctx)
httpsListener := createHTTPSListener(ctx, t, gateway.PortNumber(checkPort))
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{httpsListener})
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{httpsListener})
require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")

port := gateway.PortNumber(serviceOne.Spec.Ports[0].Port)
Expand Down Expand Up @@ -523,7 +522,7 @@ func TestHTTPMeshService(t *testing.T) {
require.Eventually(t, gatewayClassStatusCheck(ctx, resources, gc.Name, namespace, conditionAccepted), checkTimeout, checkInterval, "gatewayclass not accepted in the allotted time")

httpsListener := createHTTPSListener(ctx, t, gateway.PortNumber(e2e.HTTPPort(ctx)))
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{httpsListener})
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{httpsListener})
require.Eventually(t, gatewayStatusCheck(ctx, resources, gatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time")

// route 1
Expand Down Expand Up @@ -980,7 +979,7 @@ func TestReferencePolicyLifecycle(t *testing.T) {
fromSelector := gateway.NamespacesFromSelector

gwNamespace := gateway.Namespace(namespace)
gw := createGateway(ctx, t, resources, gatewayName, gc, []gateway.Listener{
gw := createGateway(ctx, t, resources, gatewayName, namespace, gc, []gateway.Listener{
{
Name: "https",
Port: gateway.PortNumber(httpCheckPort),
Expand Down Expand Up @@ -1252,6 +1251,110 @@ func TestReferencePolicyLifecycle(t *testing.T) {
err = resources.Delete(ctx, gw)
require.NoError(t, err)

return ctx
}).
Assess("gateway controller watches reference policy changes", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
namespace := e2e.Namespace(ctx)
gatewayNamespace := namespace
gatewayName := envconf.RandomName("gw", 16)
certNamespace := envconf.RandomName("ns", 16)
certName := "consul-server-cert"
gatewayRefPolicyName := envconf.RandomName("refpolicy", 16)

resources := cfg.Client().Resources(namespace)

// Make a copy of the certificate Secret in a different namespace for the Gateway to reference.
// This is easier than creating the Gateway in a different namespace due to pre-installed ServiceAccount dependency.
certCopy := &core.Secret{}
require.NoError(t, resources.Get(ctx, certName, namespace, certCopy))
certCopy.SetNamespace(certNamespace)
certCopy.SetResourceVersion("")
require.NoError(t, resources.Create(ctx, &core.Namespace{ObjectMeta: meta.ObjectMeta{Name: certNamespace}}))
require.NoError(t, resources.Create(ctx, certCopy))

_, gc := createGatewayClass(ctx, t, resources)
require.Eventually(t, gatewayClassStatusCheck(ctx, resources, gc.Name, namespace, conditionAccepted), checkTimeout, checkInterval, "gatewayclass not accepted in the allotted time")

fromSelector := gateway.NamespacesFromAll

// Create a Gateway with a listener that has a CertificateRef to a different namespace
certNamespaceTyped := gateway.Namespace(certNamespace)
gw := createGateway(ctx, t, resources, gatewayName, gatewayNamespace, gc, []gateway.Listener{
{
Name: "https",
Port: gateway.PortNumber(e2e.HTTPReferencePolicyPort(ctx)),
Protocol: gateway.HTTPSProtocolType,
TLS: &gateway.GatewayTLSConfig{
CertificateRefs: []*gateway.SecretObjectReference{{
Name: gateway.ObjectName(certName),
Namespace: &certNamespaceTyped,
}},
},
AllowedRoutes: &gateway.AllowedRoutes{
Namespaces: &gateway.RouteNamespaces{
From: &fromSelector,
},
},
Comment on lines +1293 to +1297
Copy link
Contributor

Choose a reason for hiding this comment

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

This config (and fromSelector := gateway.NamespacesFromAll above) seems extraneous, doesn't look like any routes are attaching to the gateway in this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct that there are no routes attaching. I kinda assumed things would break if we didn't have this minimal config here but didn't dig in to find out for sure.

},
})

// Expect that Gateway has expected error condition
// due to missing ReferencePolicy for CertificateRef in other namespace
gatewayConditionCheck := createConditionsCheck([]meta.Condition{{Type: "Ready", Status: "False", Reason: "ListenersNotValid"}})
gatewayCheck := gatewayStatusCheck(ctx, resources, gatewayName, gatewayNamespace, gatewayConditionCheck)
require.Eventually(t, gatewayCheck, checkTimeout, checkInterval, "Gateway status not set in allotted time")

// Expect that Gateway listener has expected error condition
// due to missing ReferencePolicy for CertificateRef in other namespace
listenerConditionCheck := createListenerStatusConditionsCheck([]meta.Condition{{Type: "ResolvedRefs", Status: "False", Reason: "InvalidCertificateRef"}})
listenerCheck := listenerStatusCheck(ctx, resources, gatewayName, gatewayNamespace, listenerConditionCheck)
require.Eventually(t, listenerCheck, checkTimeout, checkInterval, "Gateway listener status not set in allotted time")

// Create ReferencePolicy allowing Gateway CertificateRef
certReferencePolicy := &gateway.ReferencePolicy{
ObjectMeta: meta.ObjectMeta{
Name: gatewayRefPolicyName,
Namespace: string(certNamespace),
},
Spec: gateway.ReferencePolicySpec{
From: []gateway.ReferencePolicyFrom{{
Group: "gateway.networking.k8s.io",
Kind: "Gateway",
Namespace: gateway.Namespace(gatewayNamespace),
}},
To: []gateway.ReferencePolicyTo{{
Group: "",
Kind: "Secret",
Name: nil,
}},
},
}
require.NoError(t, resources.Create(ctx, certReferencePolicy))

// Expect that Gateway has expected success condition
gatewayCheck = gatewayStatusCheck(ctx, resources, gatewayName, gatewayNamespace, conditionReady)
require.Eventually(t, gatewayCheck, checkTimeout, checkInterval, "Gateway status not set in allotted time")

// Expect that Gateway listener has expected success condition
listenerConditionCheck = createListenerStatusConditionsCheck([]meta.Condition{{Type: "ResolvedRefs", Status: "True", Reason: "ResolvedRefs"}})
listenerCheck = listenerStatusCheck(ctx, resources, gatewayName, gatewayNamespace, listenerConditionCheck)
require.Eventually(t, listenerCheck, checkTimeout, checkInterval, "Gateway listener status not set in allotted time")

// Delete Gateway ReferencePolicy
require.NoError(t, resources.Delete(ctx, certReferencePolicy))

// Check for error status conditions again
gatewayConditionCheck = createConditionsCheck([]meta.Condition{{Type: "Ready", Status: "False", Reason: "ListenersNotValid"}})
gatewayCheck = gatewayStatusCheck(ctx, resources, gatewayName, gatewayNamespace, gatewayConditionCheck)
require.Eventually(t, gatewayCheck, checkTimeout, checkInterval, "Gateway status not set in allotted time")

listenerConditionCheck = createListenerStatusConditionsCheck([]meta.Condition{{Type: "ResolvedRefs", Status: "False", Reason: "InvalidCertificateRef"}})
listenerCheck = listenerStatusCheck(ctx, resources, gatewayName, gatewayNamespace, listenerConditionCheck)
require.Eventually(t, listenerCheck, checkTimeout, checkInterval, "Gateway listener status not set in allotted time")

// Clean up
require.NoError(t, resources.Delete(ctx, gw))

return ctx
})

Expand Down Expand Up @@ -1279,6 +1382,7 @@ func TestRouteParentRefChange(t *testing.T) {
t,
resources,
firstGatewayName,
namespace,
gc,
[]gateway.Listener{createHTTPSListener(ctx, t, gateway.PortNumber(firstGatewayCheckPort))},
)
Expand Down Expand Up @@ -1344,6 +1448,7 @@ func TestRouteParentRefChange(t *testing.T) {
t,
resources,
secondGatewayName,
namespace,
gc,
[]gateway.Listener{createHTTPSListener(ctx, t, gateway.PortNumber(secondGatewayCheckPort))},
)
Expand Down Expand Up @@ -1565,15 +1670,13 @@ func createHTTPSListener(ctx context.Context, t *testing.T, port gateway.PortNum
}
}

func createGateway(ctx context.Context, t *testing.T, resources *resources.Resources, gatewayName string, gc *gateway.GatewayClass, listeners []gateway.Listener) *gateway.Gateway {
func createGateway(ctx context.Context, t *testing.T, resources *resources.Resources, gatewayName, gatewayNamespace string, gc *gateway.GatewayClass, listeners []gateway.Listener) *gateway.Gateway {
Copy link
Member

Choose a reason for hiding this comment

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

We might consider making a createGatewayWithParams wrapper that matches the createGatewayClassWithParams pattern that Mike set up, but thats a nitpick.

t.Helper()

namespace := e2e.Namespace(ctx)

gw := &gateway.Gateway{
ObjectMeta: meta.ObjectMeta{
Name: gatewayName,
Namespace: namespace,
Namespace: gatewayNamespace,
},
Spec: gateway.GatewaySpec{
GatewayClassName: gateway.ObjectName(gc.Name),
Expand Down
1 change: 1 addition & 0 deletions internal/k8s/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ func (k *Kubernetes) Start(ctx context.Context) error {
}

err = (&controllers.GatewayReconciler{
Context: ctx,
Client: gwClient,
Log: k.logger.Named("Gateway"),
Manager: reconcileManager,
Expand Down
48 changes: 46 additions & 2 deletions internal/k8s/controllers/gateway_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

// GatewayReconciler reconciles a Gateway object
type GatewayReconciler struct {
Context context.Context
Client gatewayclient.Client
Log hclog.Logger
ControllerName string
Expand Down Expand Up @@ -93,19 +94,62 @@ func (r *GatewayReconciler) SetupWithManager(mgr ctrl.Manager) error {
handler.EnqueueRequestsFromMapFunc(podToGatewayRequest),
builder.WithPredicates(predicate),
).
Watches(
&source.Kind{Type: &gateway.ReferencePolicy{}},
handler.EnqueueRequestsFromMapFunc(r.referencePolicyToGatewayRequests),
).
Complete(gatewayclient.NewRequeueingMiddleware(r.Log, r))
}

func podToGatewayRequest(object client.Object) []reconcile.Request {
gateway, managed := utils.IsManagedGateway(object.GetLabels())
gw, managed := utils.IsManagedGateway(object.GetLabels())

if managed {
return []reconcile.Request{
{NamespacedName: types.NamespacedName{
Name: gateway,
Name: gw,
Namespace: object.GetNamespace(),
}},
}
}
return nil
}

func (r *GatewayReconciler) referencePolicyToGatewayRequests(object client.Object) []reconcile.Request {
refPolicy := object.(*gateway.ReferencePolicy)

gateways := r.getGatewaysAffectedByReferencePolicy(refPolicy)

requests := make([]reconcile.Request, 0, len(gateways))

for _, gw := range gateways {
requests = append(requests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: gw.Name,
Namespace: gw.Namespace,
},
})
}

return requests
}

// getGatewaysAffectedByReferencePolicy retrieves all Gateways potentially impacted by the ReferencePolicy
// modification. Currently, this is unfiltered and so returns all Gateways in the namespace referenced by
// the ReferencePolicy.
func (r *GatewayReconciler) getGatewaysAffectedByReferencePolicy(refPolicy *gateway.ReferencePolicy) []gateway.Gateway {
var matches []gateway.Gateway

for _, from := range refPolicy.Spec.From {
// TODO: search by from.Group and from.Kind instead of assuming this ReferencePolicy references a Gateway
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we open an issue tracking this TODO now that we have three different controllers implementing similar logic? Could be a good first issue for a potential contributor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might just contribute the change - I think it amounts to just early outing before querying all Gateways, but I didn't want to throw that into this PR and complicate testing and whatnot. Thoughts?

if from.Group != "" || from.Kind != "Secret" {
    continue
}

gateways, err := r.Client.GetGatewaysInNamespace(r.Context, string(from.Namespace))
if err != nil {
r.Log.Error("error fetching gateways", err)
return matches
}

matches = append(matches, gateways...)
}

return matches
}
23 changes: 7 additions & 16 deletions internal/k8s/controllers/http_route_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import (
"sigs.k8s.io/controller-runtime/pkg/source"
gateway "sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/hashicorp/go-hclog"

"github.com/hashicorp/consul-api-gateway/internal/k8s/gatewayclient"
"github.com/hashicorp/consul-api-gateway/internal/k8s/reconciler"
"github.com/hashicorp/go-hclog"
)

// HTTPRouteReconciler reconciles a HTTPRoute object
Expand Down Expand Up @@ -94,24 +95,14 @@ func (r *HTTPRouteReconciler) referencePolicyToRouteRequests(object client.Objec
return requests
}

// getRoutesAffectedByReferencePolicy retrieves all HTTPRoutes potentially impacted
// by the ReferencePolicy being modified. Currently, this is unfiltered and so returns
// all HTTPRoutes in the namespace referenced by the ReferencePolicy.
func (r *HTTPRouteReconciler) getRoutesAffectedByReferencePolicy(refPolicy *gateway.ReferencePolicy) []gateway.HTTPRoute {
matches := []gateway.HTTPRoute{}

routes := r.getReferencePolicyObjectsFrom(refPolicy)

// TODO: match only routes with BackendRefs selectable by a
// ReferencePolicyTo instead of appending all routes. This seems expensive,
// so not sure if it would actually improve performance or not.
matches = append(matches, routes...)

return matches
}

func (r *HTTPRouteReconciler) getReferencePolicyObjectsFrom(refPolicy *gateway.ReferencePolicy) []gateway.HTTPRoute {
matches := []gateway.HTTPRoute{}
var matches []gateway.HTTPRoute

for _, from := range refPolicy.Spec.From {
// TODO: search by from.Group and from.Kind instead of assuming HTTPRoute
// TODO: search by from.Group and from.Kind instead of assuming this ReferencePolicy references a HTTPRoute
routes, err := r.Client.GetHTTPRoutesInNamespace(r.Context, string(from.Namespace))
if err != nil {
r.Log.Error("error fetching routes", err)
Expand Down
Loading