clarify attachment by two policies to the same object when one uses a sectionName

[maleck13]

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Clarifies how two policies targeting the same resource should work when one uses a sectionName and the other does not

Which issue(s) this PR fixes:
created based on Slack chat https://kubernetes.slack.com/archives/CR0H13KGA/p1695713117110159

Does this PR introduce a user-facing change?:
None

Release Note

clarify policy attachment by two of the same policy types when using section names.

[k8s-ci-robot]

Welcome @maleck13!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @maleck13. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[youngnick]

One small change, then this LGTM.

[youngnick]

LGTM, will approve and hold for @robscott

/approve
/hold

[robscott]

Thanks for working on this @maleck13! Unfortunately I think this one ended up being more complex than I initially thought it would be, added a long comment with more detail.

[robscott]

This is surprising complex to get right, although this is a great improvement, I think there's actually still some ambiguity here. Consider the following example:

• AcmePolicy A targets just Gateway listener "http" and sets spec.foo to 1
• AcmePolicy B targets the entire Gateway and sets spec.foo to 2, and spec.bar to 3

What should the computed policy for the "http" listener be?

1. {foo: 1}
2. {foo: 1, bar: 3}
3. {foo: 2, bar: 3}

1 might make sense if we say that each Directly Attached Policy can target exactly one level. IE AcmePolicy can only be used to target Gateway Listeners, other targets are invalid. The current wording in GEP-713 seems to imply that, but it's not clear how this interacts with SectionName:

A Direct Policy Attachment is tightly bound to one instance of a particular Kind within a single namespace (or to an instance of a single Kind at cluster scope), and only modifies the behavior of the object that matches its binding.

The number or scope of objects to be modified is limited or singular. Direct Policy Attachments must target one specific object.

2 might make sense if we say that each Directly Attached Policy can target either a full resource or a section within that resource. The GEP does not currently allow for that, but as noted above, it also doesn't have any wording that would prevent that.

One concern I have is that both 2 and 3 are starting to overlap with "Inherited Policy Attachment", with 2 effectively behaving as both policies set "defaults" and 3 behaving as both policies set "overrides". Importantly we've only allowed SectionName to be used with Direct Policy Attachment.

gateway-api/apis/v1alpha2/policy_types.go

Lines 62 to 64 in 3b87766

	// Note: This should only be used for direct policy attachment when references
	// to SectionName are actually needed. In all other cases, PolicyTargetReference
	// should be used.

So all of the above actually leads me to think that 1 is the correct interpretation here (despite what I said in Slack). Interested in what others think here though. Looping in the people from the Slack thread for more discussion here:

/cc @arkodg @david-martin @frankbu @sanjaypujare @youngnick

[arkodg]

@robscott is foo and bar part of the AcmePolicy spec ? or are they targets . (foo has also been used as targets as well as policy names in this doc, so im confused :) )

[robscott]

@arkodg good question, I updated my comment to be a bit more clear, they were both intended to be part of the spec.

[arkodg]

thanks @robscott , have to go with 1. entire spec is overridden for that section by AcmePolicy A

[frankbu]

I also tend to think that 1 is the intended behavior, but I think there are still some inconsistencies in the spec. For example:

When multiple Policies of the same type target the same object, one with a sectionName specified, and one without

Note that it says of the same type. This implies they must both be Inherited Policy Attachments, but that goes against:

Note that the sectionName is currently intended to be used only for Direct Policy Attachment

And there is also this:

Note also that it's not intended that Direct and Inherited Policies should overlap, so this should only come up in exceptional circumstances.

So, it's really not very clear, how and when this applies, but I think the answer to the original question is still that a less specific policy applies to all other sections, just not the one that the more specific policy attaches to.

The question about whether the specific policy completely overrides the less specific one for the specific section is IMO yes (option 1).

[youngnick]

Thinking about this, I also support option 1, and think that we should include @robscott's example in the godoc.

[maleck13]

I may be missing this so looking to clarify.

Option 1 looks correct to me also, and so the text suggested below is correct?
still be applied but MUST not affect the named target i.e. it will be applied to all other sections which have not been targeted by another Policy

[arkodg]

@maleck13 I think we should somehow insert entire spec of the Policy instead of it to add more clarity

[maleck13]

took another stab at this, let me know if it is clearer now

[sanjaypujare]

I also support option 1. Also we should leave out Inherited Policy Attachment (with section names) to avoid the complexity. We can always add that later, right?

[k8s-ci-robot]

@robscott: GitHub didn't allow me to request PR reviews from the following users: david-martin, sanjaypujare.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This is surprising complex to get right, although this is a great improvement, I think there's actually still some ambiguity here. Consider the following example:

• AcmePolicy A attaches to Gateway listener "http" and sets foo to 1
• AcmePolicy B attaches to Gateway and sets foo to 2, and bar to 3

What should the computed policy for the "http" listener be?

1. {foo: 1}
2. {foo: 1, bar: 3}
3. {foo: 2, bar: 3}

1 might make sense if we say that each Directly Attached Policy can target exactly one level. IE AcmePolicy can only be used to target Gateway Listeners, other targets are invalid. The current wording in GEP-713 seems to imply that, but it's not clear how this interacts with SectionName:

A Direct Policy Attachment is tightly bound to one instance of a particular Kind within a single namespace (or to an instance of a single Kind at cluster scope), and only modifies the behavior of the object that matches its binding.

The number or scope of objects to be modified is limited or singular. Direct Policy Attachments must target one specific object.

2 might make sense if we say that each Directly Attached Policy can target either a full resource or a section within that resource. The GEP does not currently allow for that, but as noted above, it also doesn't have any wording that would prevent that.

One concern I have is that both 2 and 3 are starting to overlap with "Inherited Policy Attachment", with 2 effectively behaving as both policies set "defaults" and 3 behaving as both policies set "overrides". Importantly we've only allowed SectionName to be used with Direct Policy Attachment.

gateway-api/apis/v1alpha2/policy_types.go

Lines 62 to 64 in 3b87766

	// Note: This should only be used for direct policy attachment when references
	// to SectionName are actually needed. In all other cases, PolicyTargetReference
	// should be used.

So all of the above actually leads me to think that 1 is the correct interpretation here (despite what I said in Slack). Interested in what others think here though. Looping in the people from the Slack thread for more discussion here:

/cc @arkodg @david-martin @frankbu @sanjaypujare @youngnick

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sanjaypujare]

@robscott: GitHub didn't allow me to request PR reviews from the following users: david-martin, sanjaypujare.

Ack. Will take a look. (Hopefully this comment will add me as a reviewer.)

[arkodg]

thanks for adding this clarity !
/approve

[sanjaypujare]

@robscott: GitHub didn't allow me to request PR reviews from the following users: david-martin, sanjaypujare.

Ack. Will take a look. (Hopefully this comment will add me as a reviewer.)

Done. Looks good

[sanjaypujare]

LGTM

[frankbu]

LGTM

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, frankbu, maleck13, sanjaypujare, 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:

• geps/OWNERS [youngnick]

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

[robscott]

Thanks @maleck13!

/lgtm
/hold cancel

[youngnick]

@maleck13 If you can rebase this one into the new format, we can merge - I'm about to start work on splitting up GEP-713, so it would be helpful to have this in first.

[frankbu]

Why has this suggested change been ignored? #2442 (comment)

[maleck13]

@frankbu I seem to have missed that. Updated now

[robscott]

/retest

[robscott]

The current test failure looked like a flake. Adding another LGTM so we can get this one in.

/lgtm

[robscott]

@maleck13 do you mind adding a release note to this PR? Once you do, I think this should merge.

/hold cancel

[maleck13]

done