-
Notifications
You must be signed in to change notification settings - Fork 117
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
Update Istio Envoy docs to use AuthorizationPolicy instead of EnvoyFilter #492
Conversation
16b5bbf
to
3463be3
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.
LGTM (as an Istio maintainer)
examples/istio/quick_start.yaml
Outdated
rules: | ||
- to: | ||
- operation: | ||
paths: ["/*"] |
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.
Do we need this? if so, why?
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.
nope! unnecessary
examples/istio/quick_start.yaml
Outdated
############################################################ | ||
# ServiceEntry to register the OPA-Istio sidecars as external authorizers. | ||
############################################################ | ||
apiVersion: networking.istio.io/v1alpha3 |
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.
apiVersion: networking.istio.io/v1alpha3 | |
apiVersion: networking.istio.io/v1beta1 |
nit, both will 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.
will fix
# ServiceEntry to register the OPA-Istio sidecars as external authorizers. | ||
############################################################ | ||
apiVersion: networking.istio.io/v1alpha3 | ||
kind: ServiceEntry |
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.
nit: mind be nice to add exportTo: [.]
so it doesn't impact other namespaces
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.
👍
@@ -0,0 +1,33 @@ | |||
data: |
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 sure the testing setup, but this seems to patch a lot more than what is needed. Maybe thats intended, though
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.
this is intended. I found it impossible to just add the extensionProvider
to the mesh config since is a nested yaml object... it doesn't play nicely with kubectl patch
even with a json patch. Any thoughts? Otherwise this is just to restore the rest of the default data that is created when we install istio with the demo profile for our CI
@howardjohn addressed your comments - LMK if anything else should be addressed |
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.
LGTM. You may have already done this but have you tried these steps manually to ensure everything works end to end?
If you could squash your commits we can get this in.
3323757
to
a89f44e
Compare
Signed-off-by: tjons <[email protected]>
a89f44e
to
7e853c6
Compare
@ashutosh-narkar yes, have tested manually a bunch of different ways. Squashed and pushed |
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.
Thanks @tjons!
Resolves open-policy-agent/opa#5911 by rewriting the
quick_start.yaml
file to use the IstioAuthorizationPolicy
with aCUSTOM
authorizer instead of the unstableEnvoyFilter
API. I've also added some instructions to the YAML file as a comment on how to register the OPA sidecars as an external authorizer in the meshconfig object.I'm working on resolving the issues with the Istio e2e tests - the mesh config object needs to be modified directly in this case to register OPA as the external authorizer so I'm figuring out how to automate that in the end-to-end tests