Implements GEP 709 - ReferencePolicy #741

youngnick · 2021-08-02T04:03:28Z

What type of PR is this?

/kind feature
/kind documentation

What this PR does / why we need it:

This implements the design from GEP 709.

It adds a couple of extra things we missed in the GEP, that I noticed while implementing:

Condition Reason for Gateway status and an explanation of how the status must work.
Adds an ObjectReference type for cross-namespace references, to keep the existing LocalObjectReference type more similar to the core one.

Which issue(s) this PR fixes:

Fixes #709

Does this PR introduce a user-facing change?:

Added a ReferencePolicy object for controlling cross-namespace references.

k8s-ci-robot · 2021-08-02T04:03:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apis/v1alpha2/object_reference_types.go

jpeach · 2021-08-02T06:13:51Z

site-src/v1alpha2/references/cross-namespace-references.md

+```
+### Conformance Level
+
+ReferencePolicy support is a "CORE" coformance level for the following


Suggested change

ReferencePolicy support is a "CORE" coformance level for the following

ReferencePolicy support is a "Core" conformance level for the following

I'm not sure which is better here, but we should be consistent. Our docs around conformance levels use all caps, but usually the spec does not. Do we have a strong preference one way or the other?

I lifted this phrasing from one of the other GEPs - I don't mind changing, but if so, we should change the others as well.

apis/v1alpha2/httproute_types.go

apis/v1alpha2/referencepolicy_types.go

jpeach · 2021-08-02T06:22:38Z

apis/v1alpha2/referencepolicy_types.go

+// +kubebuilder:object:root=true
+// +kubebuilder:resource:categories=gateway-api,shortName=refpol
+// +kubebuilder:storageversion
+// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`


We can revisit this, but the age doesn't seem particularly useful?

I think it's a pretty standard column for k8s resources. I think it could be helpful to know when a resource was created.

+1, should be on all resources.

apis/v1alpha2/gateway_types.go

howardjohn · 2021-08-02T20:42:19Z

site-src/v1alpha2/references/cross-namespace-references.md

+1. Resource names are intentionally excluded from this policy for simplicity and because they rarely provide any meaningful protection.
+   A user that is able to write to resources of a certain kind within a namespace can always rename
+   resources or change the structure of the resources to match a given policy.
+1. A single Namespace is allowed per "From" struct.


is "all"/"any" not a common use case? I see the concerns of making it too permissive but generally APIs offer some way to be fairly permissive, even if its not easy.

I think that's probably something we'll want to add in the future, but I'd rather start with something more restrictive that we can choose to open up if it's absolutely needed.

+1 to both the comments above

robscott

Thanks for the work on this @youngnick! Mostly LGTM.

apis/v1alpha2/referencepolicy_types.go

robscott · 2021-08-02T20:55:48Z

site-src/v1alpha2/references/cross-namespace-references.md

+These references are only considered valid if a ReferencePolicy in the target namespace explicitly allows it.
+
+The following example shows how a HTTPRoute in namespace `foo` can reference a Service in namespace `bar`.
+In this example a ReferencePolicy in the `bar` namespace explicitly allows references to Services from HTTPRoutes in the `foo` namespace.


Nit: Much of our markdown is wrapped at 80 chars. I don't think this is a firm requirement, but the shorter lines can make reviews slightly easier.

robscott · 2021-08-02T20:57:10Z

site-src/v1alpha2/references/cross-namespace-references.md

+1. Resource names are intentionally excluded from this policy for simplicity and because they rarely provide any meaningful protection.
+   A user that is able to write to resources of a certain kind within a namespace can always rename
+   resources or change the structure of the resources to match a given policy.
+1. A single Namespace is allowed per "From" struct.


I think that's probably something we'll want to add in the future, but I'd rather start with something more restrictive that we can choose to open up if it's absolutely needed.

site-src/v1alpha2/references/cross-namespace-references.md

robscott · 2021-08-02T21:00:52Z

site-src/geps/gep-709.md

@@ -1,7 +1,7 @@
 # GEP-709: Cross Namespace References from Routes

 * Issue: [#709](https://github.com/kubernetes-sigs/gateway-api/issues/709)
-* Status: Implementable
+* Status: Implemented


👏 Good catch!

hbagdi

Great stuff! Thanks Nick.

apis/v1alpha2/httproute_types.go

apis/v1alpha2/object_reference_types.go

apis/v1alpha2/referencepolicy_types.go

hbagdi · 2021-08-03T18:05:46Z

apis/v1alpha2/referencepolicy_types.go

+// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
+
+// ReferencePolicy identifies kinds of resources in other namespaces that are
+// trusted to reference the specified kinds of resources in the local namespace.


local is good but same might reduce the scope of confusion.

Suggested change

// trusted to reference the specified kinds of resources in the local namespace.

// trusted to reference the specified kinds of resources in the same namespace as the policy.

apis/v1alpha2/referencepolicy_types.go

site-src/v1alpha2/references/cross-namespace-references.md

hbagdi · 2021-08-03T18:30:43Z

site-src/v1alpha2/references/cross-namespace-references.md

+1. Resource names are intentionally excluded from this policy for simplicity and because they rarely provide any meaningful protection.
+   A user that is able to write to resources of a certain kind within a namespace can always rename
+   resources or change the structure of the resources to match a given policy.
+1. A single Namespace is allowed per "From" struct.


+1 to both the comments above

site-src/v1alpha2/references/cross-namespace-references.md

youngnick · 2021-08-06T07:46:34Z

Okay, thanks for the reviews everyone, updated. I think I got all the actions I could. Please take another look!

robscott · 2021-08-06T16:08:46Z

apis/v1alpha2/referencepolicy_types.go

+	//
+	// Support: Core
+	//
+	// +kubebuilder:validation:MinLength=1


Nit: I missed this in the GEP, but we can't enforce a min length of 1 for Group. We have to support "" for core.

robscott · 2021-08-06T16:10:00Z

apis/v1alpha2/referencepolicy_types.go

+	// * HTTPRoute
+	// * TCPRoute
+	// * TLSRoute
+	// * UDPRoute


Maybe we should remove these until we actually implement Route inclusion?

robscott · 2021-08-06T16:13:41Z

apis/v1alpha2/shared_types.go

 	// Support: Custom
 	//
 	// +optional
-	BackendRef *LocalObjectReference `json:"backendRef,omitempty"`
+	BackendRef *ObjectReference `json:"backendRef,omitempty"`


This is on a collision course with GEP #718, but since there's not an implementation PR for that one yet, I think this one will likely make it in first.

robscott · 2021-08-06T16:19:00Z

A few small things but otherwise LGTM, thanks @youngnick!

hbagdi

LGTM, some nits.

hbagdi · 2021-08-06T21:08:28Z

apis/v1alpha2/object_reference_types.go

+// ObjectReference identifies an API object including its namespace.
+type ObjectReference struct {
+	// Group is the group of the referent.
+	// When empty, the "core" API group is inferred.


So interestingly, when the field is empty, it doesn't show up in 'kubectl get'. Just leaving a note here. No action required.

apis/v1alpha2/referencepolicy_types.go

hbagdi · 2021-08-06T21:12:18Z

apis/v1alpha2/referencepolicy_types.go

+	// to be an additional place that references can be valid from, or to put
+	// this another way, entries must be combined using OR.


nit: I personally got more confused. Feel free to reject.

Suggested change

// to be an additional place that references can be valid from, or to put

// this another way, entries must be combined using OR.

// to be an additional place that references can be valid from.

I've gone back and forth on this, agree it's a bit confusing, but think the specific "combined using OR" may end up being helpful. Either way, we can clean this up later if it ends up being a point of confusion.

hbagdi · 2021-08-06T21:15:22Z

apis/v1alpha2/referencepolicy_types.go

+	// To describes the resources that may be referenced by the resources
+	// described in "From". Each entry in this list must be considered to be an
+	// additional place that references can be valid to, or to put this another
+	// way, entries must be combined using OR.


Same comment.

hbagdi · 2021-08-06T21:16:30Z

apis/v1alpha2/referencepolicy_types.go

+	// * HTTPRoute
+	// * TCPRoute
+	// * TLSRoute
+	// * UDPRoute


site-src/v1alpha2/references/cross-namespace-references.md

robscott · 2021-08-06T23:45:33Z

Fixes #582

Signed-off-by: Nick Young <ynick@vmware.com>

robscott

Thanks for all the work on this @youngnick! Have a bunch of tiny nits, most are non-blocking though. Otherwise LGTM.

apis/v1alpha2/httproute_types.go

robscott · 2021-08-09T03:46:32Z

apis/v1alpha2/object_reference_types.go

+	//
+	// Note that when a namespace is specified, a ReferencePolicy object
+	// is required in the referent namespace to allow that namespace's
+	// owner to accept the reference. See the ReferencePolicy object for details.


Suggested change

// owner to accept the reference. See the ReferencePolicy object for details.

// owner to accept the reference. See the ReferencePolicy documentation for details.

robscott · 2021-08-09T03:50:03Z

apis/v1alpha2/object_reference_types.go

+	// +optional
+	// +kubebuilder:default=""
+	// +kubebuilder:validation:MaxLength=253
+	Group *string `json:"group"`


Suggested change

Group *string `json:"group"`

Group *string `json:"group,omitempty"`

robscott · 2021-08-09T03:50:15Z

apis/v1alpha2/object_reference_types.go

+	// +kubebuilder:default=Service
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	Kind *string `json:"kind"`


Suggested change

Kind *string `json:"kind"`

Kind *string `json:"kind,omitempty"`

robscott · 2021-08-09T03:53:30Z

apis/v1alpha2/referencepolicy_types.go

+	// to be an additional place that references can be valid from, or to put
+	// this another way, entries must be combined using OR.


I've gone back and forth on this, agree it's a bit confusing, but think the specific "combined using OR" may end up being helpful. Either way, we can clean this up later if it ends up being a point of confusion.

robscott · 2021-08-09T03:56:35Z

site-src/v1alpha2/references/cross-namespace-references.md

+
+

Can probably remove these new lines.

robscott · 2021-08-09T03:58:19Z

site-src/v1alpha2/references/cross-namespace-references.md

+    kind: HTTPRoute
+    namespace: foo
+  to:
+  - group: core


Now that we're allowing "" to represent core, we should probably update our examples to match.

Suggested change

- group: core

- group: ""

robscott · 2021-08-09T04:00:17Z

site-src/v1alpha2/references/cross-namespace-references.md

+Other "ImplementationSpecific" objects and references MUST also use this flow
+for cross-namespace references, except as noted in the Exceptions section
+above.


More a note for me, but when I implement the Route-Gateway binding GEP, I'll need to update this to discuss the uniqueness of that relationship.

robscott · 2021-08-09T04:03:42Z

site-src/v1alpha2/references/cross-namespace-references.md

+When discussing the process of creating cross-namespace object references, this
+document and the documentation on the API itself refers to the object being
+referred to as "the referent object", using the meaning of "referent" to be
+"the person, thing, or idea that a word, phrase, or object refers to".[1](https://dictionary.cambridge.org/dictionary/english/referent)


This footnote looks a bit off in the deploy preview. Maybe either use <sup> tag or a full word like (source).
https://deploy-preview-741--kubernetes-sigs-gateway-api.netlify.app/v1alpha2/references/cross-namespace-references/

robscott · 2021-08-09T04:06:54Z

site-src/v1alpha2/references/cross-namespace-references.md

@@ -0,0 +1,127 @@
+# Cross namespace references and ReferencePolicy
+
+### Terminology note


This feels a bit out of place. Not sure how this would render with mkdocs, but maybe something like a blockquote or callout format with this content added in the Intro section right before/after "Referent" is mentioned for the first time.

Yeah, I didn't like it there either, but couldn't think of the right place to put it. I'll see how we can do a blockquote or something more inline instead.

I think I found a "Note" syntax that should work, I'll check the deploy preview.

I added in an "admonitions" extension, that we can use with:

!!! admonition blockquote text is indented by four spaces

This looks perfect! Thanks for adding it in

Signed-off-by: Nick Young <ynick@vmware.com>

robscott · 2021-08-09T06:15:30Z

Thanks for all the work on this!

/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2021

k8s-ci-robot requested review from hbagdi and jpeach August 2, 2021 04:03

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2021

jpeach reviewed Aug 2, 2021

View reviewed changes

robscott reviewed Aug 2, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Show resolved Hide resolved

howardjohn reviewed Aug 2, 2021

View reviewed changes

robscott reviewed Aug 2, 2021

View reviewed changes

hbagdi reviewed Aug 3, 2021

View reviewed changes

robscott reviewed Aug 6, 2021

View reviewed changes

hbagdi mentioned this pull request Aug 6, 2021

implement GEP-718: rework RouteForwardTo #750

Merged

hbagdi reviewed Aug 6, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2021

robscott linked an issue Aug 6, 2021 that may be closed by this pull request

Allow cross namespace references #582

Closed

Nick Young added 4 commits August 8, 2021 23:43

Implement GEP-709, ReferencePolicy

3362e3d

Signed-off-by: Nick Young <ynick@vmware.com>

Complete documentation and add Gateway status info

0aee0fc

Signed-off-by: Nick Young <ynick@vmware.com>

Update after PR review

50a148b

Signed-off-by: Nick Young <ynick@vmware.com>

Refactor after GEP-718 changes

0ad53af

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick force-pushed the gep-709-referencepolicy branch from 37884a1 to 0ad53af Compare August 9, 2021 01:27

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

youngnick force-pushed the gep-709-referencepolicy branch 2 times, most recently from 31e2f68 to 4c9e188 Compare August 9, 2021 01:36

Fix last round of PR comments

e4bfd3a

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick force-pushed the gep-709-referencepolicy branch from 4c9e188 to e4bfd3a Compare August 9, 2021 01:39

robscott reviewed Aug 9, 2021

View reviewed changes

Nick Young added 2 commits August 9, 2021 05:06

Fix another round of PR comments

4038057

Signed-off-by: Nick Young <ynick@vmware.com>

Add admonitions to mkdocs and note to cross-namespace-references.md

677a3c0

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick force-pushed the gep-709-referencepolicy branch from 61078d8 to 677a3c0 Compare August 9, 2021 06:02

k8s-ci-robot assigned robscott Aug 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2021

k8s-ci-robot merged commit 5096d25 into kubernetes-sigs:master Aug 9, 2021

robscott mentioned this pull request Oct 17, 2021

Canary deployments with Gateway API between namespaces #695

Closed

	ReferencePolicy support is a "CORE" coformance level for the following
	ReferencePolicy support is a "Core" conformance level for the following

	// trusted to reference the specified kinds of resources in the local namespace.
	// trusted to reference the specified kinds of resources in the same namespace as the policy.

		// to be an additional place that references can be valid from, or to put
		// this another way, entries must be combined using OR.

	// owner to accept the reference. See the ReferencePolicy object for details.
	// owner to accept the reference. See the ReferencePolicy documentation for details.

	Group *string `json:"group"`
	Group *string `json:"group,omitempty"`

	Kind *string `json:"kind"`
	Kind *string `json:"kind,omitempty"`

		@@ -0,0 +1,127 @@
		# Cross namespace references and ReferencePolicy

		### Terminology note

Implements GEP 709 - ReferencePolicy #741

Implements GEP 709 - ReferencePolicy #741

Conversation

youngnick commented Aug 2, 2021

k8s-ci-robot commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 6, 2021

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 6, 2021

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 9, 2021