Implementing policy attachment GEP #736

robscott · 2021-07-26T19:31:27Z

What type of PR is this?
/kind documentation
/kind feature

What this PR does / why we need it:
This adds documentation for the policy attachment pattern described in GEP #713 and adds the associated PolicyTargetReference type. When combined with #732, this should be all that's required to implement the GEP.

Does this PR introduce a user-facing change?:

A new policy attachment model has been added.

Deploy Preview: https://deploy-preview-736--kubernetes-sigs-gateway-api.netlify.app/v1alpha2/references/policy-attachment/

robscott · 2021-07-28T17:50:09Z

Adding a hold to follow up with #732 (comment).

robscott · 2021-07-28T17:50:38Z

/hold

youngnick

This is looking great, just a few small wording changes from me.

apis/v1alpha2/policy_types.go

site-src/v1alpha2/references/policy-attachment.md

hbagdi · 2021-08-03T18:58:53Z

xref #98 #99 #91 #89 #114 #97 #611

site-src/v1alpha2/references/policy-attachment.md

robscott · 2021-08-04T01:54:08Z

Thanks for the great feedback on this @hbagdi and @youngnick! I think I've responded to all feedback now, PTAL.

apis/v1alpha2/policy_types.go

site-src/v1alpha2/references/policy-attachment.md

apis/v1alpha2/policy_types.go

site-src/v1alpha2/references/policy-attachment.md

hbagdi

nits and questions, this LGTM.

site-src/v1alpha2/references/policy-attachment.md

hbagdi · 2021-08-10T22:34:49Z

apis/v1alpha2/policy_types.go

+	// Namespace is the namespace of the referent. When unspecified, the local
+	// namespace is inferred. Even when policy targets a resource in a different
+	// namespace, it MUST only apply to traffic originating from the same
+	// namespace as the policy.


So, let's say we have gateway-a in gateway-ns namespace and route-b in route-ns namespace.

If I've a policy that in gateway-ns that targets a route-b and I also have a policy in route-ns that targets route-b, the hierarchy is policy in gateway-ns > policy in route-ns.
Is that right?

Yes, that's my interpretation, but you've got me thinking that we may want to add more restrictions around cross-namespace policy attachment. I think the primary use case was for a mesh implementation to be able to apply policy to requests originating from the local namespace to a given destination. There's clearly also use cases for Gateway as you described above, but they may become messier than intended and/or be difficult for an implementation to support. I was about to suggest that we should make this an "Extended" concept, but the whole idea of policy attachment is already extended. So that's a long way of saying that I think your interpretation is correct, but we probably need to communicate that implementations can choose to not support cross-namespace policy.

but we probably need to communicate that implementations can choose to not support cross-namespace policy.

Since we are not sure, I recommend keeping cross-namespace policy for gateway out of scope. There is a need for sure but punting on would be for the best. There are a lot of other features here and we shouldn't feel pressured.

site-src/v1alpha2/references/policy-attachment.md

youngnick · 2021-08-10T23:36:01Z

/lgtm

robscott · 2021-08-11T02:32:23Z

/hold cancel

site-src/v1alpha2/references/policy-attachment.md

howardjohn · 2021-08-11T17:01:48Z

site-src/v1alpha2/references/policy-attachment.md

+* The oldest Policy based on creation timestamp. For example, a Policy with a
+  creation timestamp of "2021-07-15 01:02:03" is given precedence over a Policy
+  with a creation timestamp of "2021-07-15 01:02:04".
+* The Policy appearing first in alphabetical order by "<namespace>/<name>". For


nit: during a conflict namespace should always be the same, right? due to the hierarchy.

I think that's likely true, but I'd prefer to avoid any possibility of conflict here if any form of hierarchy changes in the future. This is the same guidance we've used for other conflict resolution throughout the API, so I think it makes sense to keep it the same.

k8s-ci-robot · 2021-08-11T17:09:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, robscott

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~~ [robscott]

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

robscott · 2021-08-11T17:27:29Z

Thanks for the great feedback on this one! I think I've addressed everything now, PTAL.

hbagdi · 2021-08-11T20:20:52Z

/lgtm

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 26, 2021

k8s-ci-robot requested review from jpeach and thockin July 26, 2021 19:31

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 26, 2021

jpeach mentioned this pull request Jul 27, 2021

Pluggable access control #114

Closed

robscott mentioned this pull request Jul 28, 2021

Removing BackendPolicy #732

Merged

robscott added this to the v1alpha2 milestone Jul 28, 2021

robscott linked an issue Jul 28, 2021 that may be closed by this pull request

Extensible Service policy and configuration #611

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2021

youngnick reviewed Jul 29, 2021

View reviewed changes

apis/v1alpha2/policy_types.go Outdated Show resolved Hide resolved

site-src/v1alpha2/references/policy-attachment.md Outdated Show resolved Hide resolved

site-src/v1alpha2/references/policy-attachment.md Outdated Show resolved Hide resolved

hbagdi self-requested a review August 2, 2021 22:05

hbagdi reviewed Aug 3, 2021

View reviewed changes

robscott force-pushed the policy-implementation branch from dd8737b to f25c417 Compare August 4, 2021 01:43

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 4, 2021

robscott force-pushed the policy-implementation branch 2 times, most recently from bf74cbe to b145f5c Compare August 4, 2021 01:52

hbagdi reviewed Aug 5, 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 9, 2021

robscott added 2 commits August 9, 2021 17:18

Adding documentation for policy attachment model

725dd0b

Improvements to policy attachment docs, symlinking docs images

cb8e97d

robscott force-pushed the policy-implementation branch from b145f5c to 4f56cfe Compare August 10, 2021 01:38

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

hbagdi reviewed Aug 10, 2021

View reviewed changes

k8s-ci-robot assigned youngnick Aug 10, 2021

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

robscott force-pushed the policy-implementation branch from 4f56cfe to ed15276 Compare August 11, 2021 02:31

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2021

Cleaning up policy docs in response to PR feedback

fa70c18

robscott force-pushed the policy-implementation branch from ed15276 to fa70c18 Compare August 11, 2021 02:36

Miciah reviewed Aug 11, 2021

View reviewed changes

site-src/v1alpha2/references/policy-attachment.md Outdated Show resolved Hide resolved

howardjohn approved these changes Aug 11, 2021

View reviewed changes

Updating Policy images to match new direction, other doc tweaks

f35e993

k8s-ci-robot assigned hbagdi Aug 11, 2021

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

k8s-ci-robot merged commit 73456c6 into kubernetes-sigs:master Aug 11, 2021

robscott deleted the policy-implementation branch January 8, 2022 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing policy attachment GEP #736

Implementing policy attachment GEP #736

robscott commented Jul 26, 2021 •

edited

Loading

robscott commented Jul 28, 2021

robscott commented Jul 28, 2021

youngnick left a comment

hbagdi commented Aug 3, 2021

robscott commented Aug 4, 2021

hbagdi left a comment

hbagdi Aug 10, 2021

robscott Aug 11, 2021

hbagdi Aug 11, 2021

youngnick commented Aug 10, 2021

robscott commented Aug 11, 2021

howardjohn Aug 11, 2021

robscott Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

robscott commented Aug 11, 2021

hbagdi commented Aug 11, 2021

Implementing policy attachment GEP #736

Implementing policy attachment GEP #736

Conversation

robscott commented Jul 26, 2021 • edited Loading

robscott commented Jul 28, 2021

robscott commented Jul 28, 2021

youngnick left a comment

Choose a reason for hiding this comment

hbagdi commented Aug 3, 2021

robscott commented Aug 4, 2021

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi Aug 10, 2021

Choose a reason for hiding this comment

robscott Aug 11, 2021

Choose a reason for hiding this comment

hbagdi Aug 11, 2021

Choose a reason for hiding this comment

youngnick commented Aug 10, 2021

robscott commented Aug 11, 2021

howardjohn Aug 11, 2021

Choose a reason for hiding this comment

robscott Aug 11, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 11, 2021

robscott commented Aug 11, 2021

hbagdi commented Aug 11, 2021

robscott commented Jul 26, 2021 •

edited

Loading