Require ReferencePolicy for certificateRef in other namespace #154

nathancoleman · 2022-04-20T21:21:15Z

Changes proposed in this PR:

When a listener on a Gateway contains a certificateRef from a different namespace, require applicable ReferencePolicy. The expectations for the listener status in the scenario are described here.

Example

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: example-gateway
  namespace: default
spec:
  gatewayClassName: consul-api-gateway
  listeners:
  - protocol: HTTPS
    port: 8443
    name: https
    allowedRoutes:
      namespaces:
        from: Same
    tls:
      certificateRefs:
        - name: consul-server-cert
          namespace: consul
---
apiVersion: gateway.networking.k8s.io/v1alpha2
kind: ReferencePolicy
metadata:
  name: secret-reference-policy
  namespace: consul
spec:
  from:
    - group: gateway.networking.k8s.io
      kind: Gateway
      namespace: default
  to:
    - group: ""
      kind: Secret

How I've tested this PR:

Deployed the API Gateway using this build and added certificateRefs across namespaces and within the same namespaces, both with and without an applicable ReferencePolicy in place.

How I expect reviewers to test this PR:

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use imperative present tense (e.g. Add support for...)

nathancoleman

Personal Review

nathancoleman · 2022-04-21T19:11:44Z

internal/k8s/reconciler/listener_test.go

-	// protocols
-	listener := NewK8sListener(&gw.Gateway{}, gw.Listener{}, K8sListenerConfig{
-		Logger: hclog.NewNullLogger(),
+	t.Run("Unsupported protocol", func(t *testing.T) {


The changes in this file are an attempt at breaking up the test into subtests so that it's easier to understand what's happening where. The only new test logic is in the subtests whose name mentions "cross-namespace" secret refs.

.changelog/154.txt

nathancoleman · 2022-04-21T19:21:33Z

internal/k8s/reconciler/utils.go

+//
+// For example, a Gateway in namespace "foo" may only reference a Secret in namespace "bar"
+// if a ReferencePolicy in namespace "bar" allows references from namespace "foo".
+func referenceAllowed(ctx context.Context, fromGK metav1.GroupKind, fromNamespace string, toGK metav1.GroupKind, toNamespace, toName string, c gatewayclient.Client) (bool, error) {


The diff here is kinda hard to read, but this function just contains the shared logic for checking existence of an applicable ReferencePolicy that we already had for xRoute->Backend.

nathancoleman · 2022-04-21T19:22:25Z

internal/k8s/reconciler/utils.go

-	serviceID := fmt.Sprintf("%s/%s", name, parentNamespace)
+// getNamespacedName returns types.NamespacedName defaulted to a parent
+// namespace in the case where the provided namespace is nil.
+func getNamespacedName(name gw.ObjectName, namespace *gw.Namespace, parentNamespace string) types.NamespacedName {


I needed a log-friendly name for secrets and generalized this function rather than duplicating it

nathancoleman · 2022-04-26T15:58:52Z

Holding for inclusion in our v0.3.0 release

mikemorris

I think there's a minor logic error in the refPolicy.Spec.To loop in referenceAllowed, aside from that LGTM.

.changelog/154.txt

internal/k8s/reconciler/listener_test.go

internal/k8s/reconciler/utils.go

mikemorris · 2022-05-13T15:53:50Z

internal/k8s/reconciler/utils.go

-				route.GroupVersionKind().Kind == string(from.Kind) &&
-				route.GetNamespace() == string(from.Namespace) {
-				validFrom = true
+			if fromGK.Group == string(from.Group) && fromGK.Kind == string(from.Kind) && fromNamespace == string(from.Namespace) {


I know the gateway certificate and route functions wrapping this set the appropriate default Kind already, but it might be good to implement the common logic for converting from Group "" to core/v1 here (and inrefPolicy.Spec.To loop) to correctly match if it's specified explicitly anywhere?

Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>

nathancoleman marked this pull request as ready for review April 21, 2022 19:12

nathancoleman requested review from mikemorris, sarahalsmiller and andrewstucki April 21, 2022 19:12

nathancoleman commented Apr 21, 2022

View reviewed changes

nathancoleman marked this pull request as draft April 26, 2022 15:57

nathancoleman added the do not merge label Apr 26, 2022

nathancoleman marked this pull request as ready for review April 28, 2022 15:44

nathancoleman added pr/run-conformance and removed do not merge labels Apr 28, 2022

nathancoleman added 8 commits May 2, 2022 18:13

Require ReferencePolicy for certificateRef in other namespace

f5f7fce

Modify getServiceID to use generic NamespacedName

a761fd2

Generalize checking of ReferencePolicies

1ac383b

Organize + update existing unit tests

1325036

Add test coverage for validation ReferencePolicy for listener

d012282

Add gatewayAllowedForSecretRef unit test coverage

6f92282

Add changelog entry

987edc2

Add docstring for referenceAllowed

75a8362

nathancoleman force-pushed the secret-reference-policy branch from 5822699 to 75a8362 Compare May 2, 2022 22:13

mikemorris suggested changes May 13, 2022

View reviewed changes

Apply suggestions from code review

1c5856b

Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>

nathancoleman removed the pr/run-conformance label May 26, 2022

Check subsequent ReferencePolicies if to.name doesn't match

8d86dee

mikemorris approved these changes May 27, 2022

View reviewed changes

nathancoleman merged commit adc42cd into main May 27, 2022

nathancoleman deleted the secret-reference-policy branch May 27, 2022 20:45

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require ReferencePolicy for certificateRef in other namespace #154

Require ReferencePolicy for certificateRef in other namespace #154

nathancoleman commented Apr 20, 2022 •

edited

Loading

nathancoleman left a comment

nathancoleman Apr 21, 2022

nathancoleman Apr 21, 2022

nathancoleman Apr 21, 2022

nathancoleman commented Apr 26, 2022

mikemorris left a comment

mikemorris May 13, 2022 •

edited

Loading

Require ReferencePolicy for certificateRef in other namespace #154

Require ReferencePolicy for certificateRef in other namespace #154

Conversation

nathancoleman commented Apr 20, 2022 • edited Loading

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

nathancoleman left a comment

Choose a reason for hiding this comment

nathancoleman Apr 21, 2022

Choose a reason for hiding this comment

nathancoleman Apr 21, 2022

Choose a reason for hiding this comment

nathancoleman Apr 21, 2022

Choose a reason for hiding this comment

nathancoleman commented Apr 26, 2022

mikemorris left a comment

Choose a reason for hiding this comment

mikemorris May 13, 2022 • edited Loading

Choose a reason for hiding this comment

nathancoleman commented Apr 20, 2022 •

edited

Loading

mikemorris May 13, 2022 •

edited

Loading