-
Notifications
You must be signed in to change notification settings - Fork 31
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
AuthConfig v1beta2 #417
AuthConfig v1beta2 #417
Conversation
c049de7
to
1f52145
Compare
1f52145
to
1eaead9
Compare
It cannot require 'value' or the conversion will fail when the value is an empty string (due to JSON marshalling/unmarshalling involved).
Ran through the verification steps and all working as expected 👍 |
- Let the Operator deploy the default conversion webhook service - Substitute the webhook image with the one built locally - Keep the patches to the AuthConfig CRD performed by the Operator to activate the webhook before re-applying the CRD with local changes
fa9e7eb
to
4db62fa
Compare
4db62fa
to
aca78c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good to me 👍
Ran throught the verification steps and all working as expected 🥇
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
This field is required and therefore should be set as the Kube API server would – i.e. not as nil, but with an empty array when it has no elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read the changes. Nothing is standing out to me in the change as a problem. With the size the PR and my lack of experiences with authorino in general I can't confidently say these changes are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good to me anyhow
…e to avoid shell command issues
…rapped within quotes to avoid auto-escaping when pasting in a few terminals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through all the user-guides as a second pair of eyes and found some very minor suggestions (unrelated to this PR) all fixed in latest commits - thanks @guicassolato - the changes look good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the changes, with attention to the new architecture of the main.go spawning the services (auth, webhook) with cobra cmds coming from the Deployment object and the conversions of the designated hub v1beta1... Haven't had time to go through the guides, but I've seen in the comments that they work as expected, will try to do some of the more complex ones in the near future. Great work! 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surely missing something. The expected is that the new version of the CRD is the stored one so eventually the older one can be dropped
kubectl apply -f https://raw.githubusercontent.com/Kuadrant/authorino-operator/$(OPERATOR_BRANCH)/config/deploy/manifests.yaml | ||
kubectl -n authorino-operator wait --timeout=300s --for=condition=Available deployments --all | ||
install-operator: ## Installs Authorino Operator and dependencies into the Kubernetes cluster configured in ~/.kube/config | ||
curl -sL https://raw.githubusercontent.com/Kuadrant/authorino-operator/$(OPERATOR_BRANCH)/utils/install.sh | bash -s -- --git-ref $(OPERATOR_BRANCH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future, for this kind of tasks, I would use kustomize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the same pattern adopted by https://docs.kuadrant.io/getting-started/#set-up-clusters-and-multicluster-gateway-controller.
kustomize
is also bit more cumbersome when you need to support many options (even with overlays). Besides, it isn't much helpful for things like sequencing commands, e.g. https://github.com/Kuadrant/authorino-operator/blob/152a1627a0b2f5d36c3983192644ec8c5d625bad/utils/install.sh#L140
@@ -774,6 +774,7 @@ func (s *AuthConfigStatus) Ready() bool { | |||
// AuthConfig is the schema for Authorino's AuthConfig API | |||
// +kubebuilder:object:root=true | |||
// +kubebuilder:subresource:status | |||
// +kubebuilder:storageversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be v1beta2 the one to be stored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. Usually, first you introduce a new version of the API and allow time for users to adapt; then the newer version becomes the stored one in a subsequent rolling update. This way, when they install the new version of the CRD, previously existing CRs based on the old version won't be a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing something. If the stored version is the old one, even the clients of the new version will store CR with the old version. then the newer version becomes the stored one in a subsequent rolling update
. Then, there will be the same situation, all the CR stored versions would be the old one and the CRD's stored one will be the new one. I do not see any advantage of postponing the stored version.
What I was assuming, and correct me if I am wrong, whenever a client updates / creates an object, if the object's version is not the stored one, the webhook conversion API will be called and the object converted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the Versions in CustomResourceDefinitions, the workflow for upgrading CRD version would be (high level):
- Release 1: Add a new version to the version list. The old version is the stored one. Both versions will be served. No deprecation flag.
- Release 2 (or later): Old version deprecation. The old version is served, but not stored and added deprecation warning. The new version is the stored (and served) one. Just after installing the new CRD with the changes, the stored (in old version) CR's need to be migrated. This is procedure needs to be run by some cluster admin.
- Release 3 (or later): Version removal. When all the clients have migrated to the new version (one option is checking kube-apiserver logs), the old version can either be disabled for serving (
served: false
) or just removed from the CRDspec.versions
list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent summary, @eguzki. Thank you for this!
@@ -774,6 +774,7 @@ func (s *AuthConfigStatus) Ready() bool { | |||
// AuthConfig is the schema for Authorino's AuthConfig API | |||
// +kubebuilder:object:root=true | |||
// +kubebuilder:subresource:status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considered adding kubebuilder:deprecatedversion
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be done in this step. check my comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not review all changes deeply, but I think is good enough for going forward. Fixes can be done later.
Awesome work 🎖️
…he manifests while Kuadrant/authorino#417 is not merged
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
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
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
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
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
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
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
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
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
* 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
Introduces a new version of the
AuthConfig
API (v1beta2
), implementing roughly the following bullet points of #400:2.1. Dynamic values (or values fetched from the Authorization JSON with
valueFrom.authJSON
)3.1.
identity
→authentication
4.1.
identity.oidc
→.jwt
4.2.
identity.oauth2
→.oauth2Introspection
4.3.
identity.mtls
→.x509
4.4.
identity.kubernetes
→.kubernetesTokenReview
4.5.
identity.credentials
4.6.
identity.extendedProperties
→.override
and.default
4.7.
authorization.json
→.rules
.patternMatching
4.8.
authorization.kubernetes
→.kubernetesSubjectAccessReview
4.9.
authorization.authzed
→.spicedb
response
5.1.
wrapper
s as structured property5.2.
denyWith
merged intoresponse
Plus a few other minor enhancements preparing for the future, such as:
Because this introduces a second version of the CRD without removing the existing one, this PR also prepares Authorino for the deployment of a conversion webhook. Authorino itself can double as either the data plane external authorization server (by running it, as usual, with
authorino server
) or as a control plane-level webhook service.The tooling for running Authorino locally (developer workflow) will take care of deploying an instance of the webhook service and configuring the Kubernetes API server to use by injecting the service refs in the CRD at runtime. I imagine soon this will be handled by the Authorino Operator as well.
No changes regarding the reconciliation of the AuthConfig CRs were made at this point. The reconciler will continue to request CRs based on
v1beta1
which are fully compatible with what's exposed byv1beta2
. Some new features ofv1beta2
might not yet be available, until the reconciler is updated to request CRs onv1beta2
.Verification steps
① Setup
Note: A default conversion webhook service will be deployed and managed by the Authorino Operator. The Authorino
local-setup
workflow substitutes the image of the webhook deployment with the one built locally, so the webhook runs based on the same code as the deployed Authorino instances will default to. Thelocal-setup
workflow also ensures that patches to the AuthConfig CRD performed by the Operator to activate the webhook are kept when re-applying the CRD with local changes.② Verify that the new version of API has been installed
③ Create a trivial AuthConfig based on v1beta2
Create the AuthConfig:
Request the AuthConfig in its default served version (v1beta2):
Request the AuthConfig in both versions:
④ Create a slightly more complex AuthConfig based on v1beta2
Create the AuthConfig:
Request the CR in both versions and compare.
⑤ Create a all-in-one AuthConfig based on v1beta2
Request the CR in both versions and compare.
TODOs
v1beta2.AuthConfig.ConvertTo
andv1beta2.AuthConfig.ConvertFrom
This PR should be merged together with Kuadrant/authorino-operator#137.