-
Notifications
You must be signed in to change notification settings - Fork 16
Require ReferencePolicy for certificateRef in other namespace #154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal Review
// protocols | ||
listener := NewK8sListener(&gw.Gateway{}, gw.Listener{}, K8sListenerConfig{ | ||
Logger: hclog.NewNullLogger(), | ||
t.Run("Unsupported protocol", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// | ||
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed a log-friendly name for secrets and generalized this function rather than duplicating it
Holding for inclusion in our v0.3.0 release |
5822699
to
75a8362
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a minor logic error in the refPolicy.Spec.To
loop in referenceAllowed
, aside from that LGTM.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Changes proposed in this PR:
When a listener on a
Gateway
contains acertificateRef
from a different namespace, require applicableReferencePolicy
. The expectations for the listener status in the scenario are described here.Example
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 applicableReferencePolicy
in place.How I expect reviewers to test this PR:
Checklist: