-
Notifications
You must be signed in to change notification settings - Fork 33
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
Istio external authorizer available not only for IstioOperator #192
Conversation
da1f40b
to
d355268
Compare
57d211c
to
398b8c9
Compare
fbf8b2e
to
cc37f84
Compare
* Reconciles also the Istio ConfigMap with the external authorizers * If the IstioOperator CR returns an error, it continues reconciling CM
* With the new functionality
* Still relying on the istio CRD being installed. If not, means no istio * If it's 404, means it's installed other way than it's operator * If any other error happens, istio is installed, but an error occurred
9637df3
to
ffd9d15
Compare
* Added ENV for CM name
logger, _ := logr.FromContext(ctx) | ||
|
||
err := r.unregisterExternalAuthorizerIstio(ctx) | ||
isIstioInstalled, err := r.unregisterExternalAuthorizerIstio(ctx, kObj) |
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.
Maybe in a next iteration unregisterExternalAuthorizerIstio
and unregisterExternalAuthorizerOSSM
can become one, and similarly registerExternalAuthorizerIstio
and registerExternalAuthorizerOSSM
. Fundamentally, and given the new abstractions now introduced with this PR, these pairs of functions are not very different between them – if any different at all.
getIstioConfigObjects
could become simply getConfigObjects
(or perhaps getGatewayProviderConfigObjects
to be more literal with the name), and return all gateway provider config objects that need to be patched.
Istio or OSSM, it doesn't really matter. In the end, it all reduces to iterating over all configs, passing them into the calls to common.UnregisterKuadrantAuthorizer
and common.RegisterKuadrantAuthorizer
. Thanks to this PR, the details of how to patch each kind of config is decoupled from the caller, but abstracted by each config wrapper itself.
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.
Makes total sense now, I'll keep in mind this for the next refactor, that most possibly will be the plugin way of using GWs
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.
Nice job @didierofrivia!
I've left a couple additional comments, which, if you agree, can be addressed in this PR still or in another iteration.
Verification steps succeeded. Additionally, after step ❷:
❯ kubectl get istiooperator/installed-state-istiocontrolplane -n istio-system -o yaml | grep authorino
❯ kubectl get configmap/istio -n istio-system -o yaml | grep authorino
service: authorino-authorino-authorization.kuadrant-system.svc.cluster.local
This PR aims to fix the registration of our Authorino as an external authorizer using Istio when it's not installed via its Operator.
Closes #168
Notes
Verification Steps
❶ Create a cluster with Gateway API and Istio:
❷ Deploy Kuadrant dependencies and a local build, then apply its CR:
Create the namespace for kuadrant control plane
Then install dependencies and a local kuadrant image
Then apply Kuadrant CR
❸ Apply toystore example service and create an HTTPRoute
❹ Create API Key as Secrets for user Bob
❺ Apply Authorino AuthPolicy
❻ Send requests:
TODO: