[authpolicy-v2] docs #275
Merged
[authpolicy-v2] docs #275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## authpolicy-v2 #275 +/- ##
=================================================
- Coverage 64.35% 63.40% -0.96%
=================================================
Files 35 35
Lines 3776 3776
=================================================
- Hits 2430 2394 -36
- Misses 1156 1182 +26
- Partials 190 200 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
5 times, most recently
from
84fa658 to
4320601
Compare
9 tasks
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
from
4320601 to
18f5688
Compare
maleck13
reviewed
Oct 17, 2023
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
from
18f5688 to
3f246b9
Compare
maleck13
reviewed
Oct 18, 2023
|
|
||
| ## Using the AuthPolicy | ||
|
|
||
| ### Targeting a HTTPRoute networking resource |
There was a problem hiding this comment.
Interesting I think I would have inverted these to follow the hierarchy. IE cover gateway first and HTTPRoute second?
There was a problem hiding this comment.
I see targeting the HTTPRoute as the simplest case and therefore the one to explain first, but maybe that's because of how later we explain, when targeting a gateway, what happens in case there are also other policies targeting HTTPRoutes under that gateway. If we keep that example as-is, then the reader needs to know what targeting a HTTPRoute means beforehand.
Should we go then?
OPTION 1 (current)
- Targeting a HTTPRoute – focus on the direct effect of the policy over that route, no mention about Gateway policies
- Targeting a Gateway – explain how the policy affects all routes under the gateway EXCEPT the ones that have a more specific policy of its own
OPTION 2
- Targeting a Gateway – explain how it affects all HTTPRoutes under the gateway, no mention about HTTPRoute policies
- Targeting a HTTPRoute – explain how the policy directly affects that route AND replaces any other policy targeting gateways the route is under
There was a problem hiding this comment.
So I don't see it as a big deal and not a blocker to merge. But my preference is prob Option 2 as it (perhaps I am biased) fits how I think about gateway API resources
guicassolato
force-pushed
the
authpolicy-v2
branch
from
1ba111e to
856d6d8
Compare
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
from
3f246b9 to
7d52a85
Compare
Boomatang
requested changes
Oct 18, 2023
doc/auth.md
Outdated
Comment on lines
58
to
61
| # (optional) selectors of HTTPRouteRules within the targeted HTTPRoute that activate the AuthPolicy | ||
| # each element contains a HTTPRouteMatch object that will be used to select HTTPRouteRules that include at least one identical HTTPRouteMatch | ||
| # the HTTPRouteMatch part does not have to be fully identical, but the what's stated in the selector must be identically stated in the HTTPRouteRule | ||
| # do not use it on AuthPolicies that target a Gateway |
There was a problem hiding this comment.
Well the long strings render correctly on the website or will this cause a horizontal scroll bar. It would be better if that was avoided. I had to go back and read this for a second time when seen there was a horizontal scroll bar. This was when reading the file using the rich diff and not the source diff view that github gives.
| | `unauthorized` | [Custom denial status spec](https://docs.kuadrant.io/authorino/docs/features/#custom-denial-status-responseunauthenticated-and-responseunauthorized) | No | Customizations on the denial status and other HTTP attributes when the request is unauthorized. (Default: `403 Forbidden`) | | ||
| | `success` | [SuccessResponseSpec](#successresponsespec) | No | Response items to be included in the auth response when the request is authenticated and authorized. | | ||
|
|
||
| ##### SuccessResponseSpec |
There was a problem hiding this comment.
Is this and the next heading SuccessResponseItem at the correct heading level. When reading it seems like they should be at h3 not h4.
There was a problem hiding this comment.
SuccessResponseSpec (h4) is used within ResponseSpec (h3). So it's correct.
It's actually SuccessResponseItem that should be h5, because it's used only within SuccessResponseSpec (h4).
There was a problem hiding this comment.
I will leave this up to you, if you want to change it. It did seem odd but that could also be how it was rendered in the rich diff.
doc/auth.md
Outdated
Comment on lines
5
to
6
| 1. Allows it to target Gateway API networking resources such as [HTTPRoutes](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute) and [Gateways](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.Gateway), using these resources to obtain additional context, i.e., which traffic workload (HTTP attributes, hostnames, user attributes, etc) to enforce auth. | ||
| 2. Allows to specify which specific subsets of the targeted network resource to apply the auth rules to. |
There was a problem hiding this comment.
The start of these points reads funny to me. I wonder if the suggested is better.
Suggested change
| 1. Allows it to target Gateway API networking resources such as [HTTPRoutes](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute) and [Gateways](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.Gateway), using these resources to obtain additional context, i.e., which traffic workload (HTTP attributes, hostnames, user attributes, etc) to enforce auth. | |
| 2. Allows to specify which specific subsets of the targeted network resource to apply the auth rules to. | |
| 1. Targets Gateway API networking resources such as [HTTPRoutes](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute) and [Gateways](https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.Gateway), using these resources to obtain additional context, i.e., which traffic workload (HTTP attributes, hostnames, user attributes, etc) to enforce auth. | |
| 2. Specify which specific subsets of the targeted network resource to apply the auth rules to. |
There was a problem hiding this comment.
I can cope with the suggestion for bullet point 1 (to be ported to https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/rate-limiting.md as well), but 2 needs to be clear that specific subsets is optional.
There was a problem hiding this comment.
If point 2 needs to be clear that specific subsets are optional I think both version are not clear. Possible might need to use the word optional in there.
maleck13
reviewed
Oct 18, 2023
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
from
b4525c1 to
abf8ac1
Compare
guicassolato
force-pushed
the
docs/authpolicy-v2
branch
from
abf8ac1 to
bea0101
Compare
Boomatang
approved these changes
Oct 18, 2023
guicassolato
added a commit
that referenced
this pull request
Oct 20, 2023
* docs: authpolicy v1beta2 * docs: addressing suggestions of enhancements to the authpolicy docs
guicassolato
added a commit
that referenced
this pull request
Oct 20, 2023
* AuthPolicy v1beta2 Defines new `v1beta2` version of the `AuthPolicy` CRD, based on Authorino's `AuthConfig/v1beta2`. Closes #247 Depends on Kuadrant/authorino#417, Kuadrant/authorino-operator#137 Bump Authorino to latest (unreleased) version Bump Authorino to latest (unreleased) version Bump Authorino to latest (unreleased) version Update AuthPolicy manifests based on latest AuthConfig v1beta2 changes Bump Authorino to latest (unreleased) version Bump Authorino to latest (unreleased) version * Bump Authorino Operator to v0.9.0 Closes #263 * Superseding of strict host subsets between AuthConfigs Enables [superseding of strict host subsets](https://github.com/Kuadrant/authorino/blob/main/docs/architecture.md#avoiding-host-name-collision) in Authorino – i.e., set `SupersedingHostSubsets` to `true` in the Authorino CR. Closes #264. * Route selectors for the AuthPolicy * Merge the hostnames of HTTPRoute (direct or inherited from the Gateway) into all Istio AuthorizationPolicy rules that do not include hostnames already built from the route selectors, so we don't send a request to authorino for hosts that are not in the scope of the policy * AuthConfig with OR conditions between HTTPRouteMatches of a HTTPRouteRule and between HTTPRouteRules themselves * Fix reconciliation of gateway AuthPolicies Skips creation of Istio AuthorizationPolicies and Authorino AuthConfigs for gateways without any accepted HTTPRoutes. * Ensure Gateway API group is used when checking the targetRef kind * Trigger reconciliation of possibly affected gateway policies after reconciling HTTPRoute ones * Mapper of HTTPRoute events to policy events that goes through the parentRefs of the route and finds all policies that target one of its parent resources, thus yielding events for those policies. * AuthConfig condition from GWAPI QueryParamMatchRegularExpression * Skip Istio AuthorizationPolicy rules for GWAPI PathMatchRegularExpression and HeaderMatchRegularExpression * Generate Istio AuthorizationPolicy rules out of top-level conditions or full HTTPRoute only (i.e. ignore config-level conditions) * tests: fix integration tests for authpolicy with route selectors * fix: AuthorizationPolicy rules when PathMatchRegularExpression + unit tests from Istio AuthorizationPolicy rules from HTTPRouteRules and hostnames * tests: unit tests from AuthConfig conditions from HTTPRouteRules and hostnames * tests: unit tests for the AuthPolicy type * tests: unit test for RateLimitPolicyList.GetItems() * tests: unit test for RouteSelectors.HostnamesForConditions() * tests: integration test for Gateway policy with having other policies attached to HTTPRoutes * fixup: AuthPolicy SuccessResponseSpec type * tests: integration tests for AuthPolicy <-> AuthConfig mapping * tests: integration tests gateway policies whith all HTTPRoutes talken * Well-known attributes used in the generated AuthConfigs Closes: - #265 * tests: integration tests for policies only with unmatching route selectors * lint: fix duplicated imported package * Move AuthPolicy v1beta top-level 'patterns' and 'when' fields one level up This will pair the level of these policy-wide options to the top-level 'routeSelectors', rather than having two things that have semantics over the same scope defined at different levels in the API. This change also separates the auth scheme, making it now exclusively about auth rules. * [authpolicy-v2] docs (#275) * docs: authpolicy v1beta2 * docs: addressing suggestions of enhancements to the authpolicy docs * Add mandatory Gateway API label to the AuthPolicy CRD (#279) Closes #278 * docs: fix typos * [authpolicy-v2] AuthPolicy validation rules (#281) * prevent usage of routeSelectors in a gateway AuthPolicy * AuthPolicy CEL validation rules - Invalid targetRef.group - Invalid targetRef.kind - Route selectors not supported when targeting a Gateway Note: cannot set a validation rule for !has(spec.targetRef.namespace) || spec.targetRef.namespace == metadata.namespace, because Kubernetes does not allow accessing `metadata.namespace`. See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.