Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Final changes for RC2 based on sig-net review feedback #898

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

robscott
Copy link
Member

@robscott robscott commented Oct 8, 2021

What type of PR is this?
/kind cleanup
/kind api-change

What this PR does / why we need it:
There are three changes here, all based on feedback from @thockin in #861:

  1. The "Prefix" path match type has been renamed "PathPrefix" to leave room for a future "StringPrefix" match.
  2. The "ClassName" field in PolicyTargetReference has been removed as it is not clearly necessary at this point.
  3. A new optional "Name" field has been added to ReferencePolicyTo, enabling more fine grained control over which resource(s) may be referenced.

More thoughts on all of the above:

  1. I think it could be useful to add StringPrefix matching in the future. Currently Envoy based implementations of this API need to use RegEx matching to support our PathPrefix matching. Although this is fine as a starting point, I can imagine at least some users of the API would prefer to have access to the StringPrefix matching they're familiar with for the implementations that support it. This doesn't commit us to providing StringPrefix as a match type, but does make that simpler if we want to.
  2. I was the one who originally added "ClassName", but after looking through the docs, GEP, and asking around, it does not seem like this field was clearly needed. It may be helpful in certain use cases, but I don't think it's necessary at this point.
  3. The idea of adding to.name in ReferencePolicy has come up many times already, and even reappeared when I presented this concept to sig-auth. Although it's true that we could add this at a later point, it would be messier. Users would need to know whether the implementation they were using was new enough to support that particular field of ReferencePolicy, and if it wasn't you may be unknowingly allowing more references than intended. Although I've generally pushed to keep ReferencePolicy as simple as possible to start, this feels like an inevitable addition and it is significantly simpler to add this up front.

Does this PR introduce a user-facing change?:

* The "Prefix" path match type has been renamed "PathPrefix".
* The "ClassName" field in PolicyTargetReference has been removed.
* A new optional "Name" field has been added to ReferencePolicyTo.

/hold for consensus
/cc @bowei @hbagdi @jpeach @youngnick

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2021
@robscott robscott force-pushed the v1a2-final-changes branch 3 times, most recently from 38f46eb to 810411c Compare October 8, 2021 18:50
@youngnick
Copy link
Contributor

I agree with all three of these changes, I think that the call on ReferencePolicy name is particularly good.

/lgtm

/hold
in case anyone else wants to go.

I'll update the changelog again once this merges.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2021
// namespace.
//
// +optional
Name *ObjectName `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Name be added to ReferenceFrom for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the structural similarities of from and to, I think they're fundamentally different. Since this policy is in the referent namespace, it's helpful to be able to control which of your resources may be referenced. For example only granting access to a specific cert in a namespace that contains many.

On the other hand restricting access from resources you don't control by name is not particularly helpful. The resource owner in the other namespace can just use the allowed name, so it becomes quite easy to bypass any controls intended by the referent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your certificate example, the reference policy allows all gateways from a namespace to reference a specific named certificate. A reasonable extension of that would be to let a specific gateway reference a specific certificate. The names can be aligned by controllers or by convention. However, control over the name of the referee does make this more of an operational safety mechanism than a security one, so maybe it is better not to muddy those concerns.

Not a blocker for me, anyway :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, control over the name of the referee does make this more of an operational safety mechanism than a security one, so maybe it is better not to muddy those concerns.

That's a great way to differentiate these. Agree that it's best not to muddy those concerns yet, but if we start getting feedback/feature requests asking for from.name, we should probably revisit this.

Copy link
Contributor

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

apis/v1alpha2/httproute_types.go Show resolved Hide resolved
site-src/geps/gep-713.md Outdated Show resolved Hide resolved

// Name is the name of the referent. When unspecified or empty, this policy
// refers to all resources of the specified Group and Kind in the local
// namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, woot!

There are three changes here:

1. The "Prefix" path match type has been renamed "PathPrefix" to leave
room for a future "StringPrefix" match.

2. The "ClassName" field in PolicyTargetReference has been removed as it
is not clearly necessary at this point.

3. A new optional "Name" field has been added to ReferencePolicyTo,
enabling more fine grained control over which resource(s) may be
referenced.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2021
@robscott
Copy link
Member Author

I think we have consensus here, removing the hold, but still need a final LGTM.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Oct 11, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit dfd24e4 into kubernetes-sigs:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants