search

istio / enhancements

Enhancement Tracking and Backlog Repo for Istio Releases
14 stars 42 forks source link

External authorization promotion #125

Closed bochengchu closed 2 years ago

bochengchu commented 2 years ago

Promote external authorization to beta.

Performance/benchmark tests: https://github.com/istio/tools/tree/master/perf/benchmark/configs/istio/ext-authz

istio-testing commented 2 years ago

Hi @bochengchu. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
windsonsea commented 2 years ago

/ok-to-test

linsun commented 2 years ago

/ok-to-test

linsun commented 2 years ago

@bochengchu thanks for submitting this for review! I think it is reasonable overall.

I do have 1 comment while reading docs that I would like to be addressed before going to beta. The current doc says:

Restart Istiod to allow the change to take effect with the following command:

$ kubectl rollout restart deployment/istiod -n istio-system
deployment.apps/istiod restarted

Is this restart required? Seems odd to require restart of istiod after modifying the istio config map.

aryan16 commented 2 years ago

@bochengchu thanks for submitting this for review! I think it is reasonable overall.

I do have 1 comment while reading docs that I would like to be addressed before going to beta. The current doc says:

Restart Istiod to allow the change to take effect with the following command:

$ kubectl rollout restart deployment/istiod -n istio-system
deployment.apps/istiod restarted

Is this restart required? Seems odd to require restart of istiod after modifying the istio config map.

I agree we don’t require a restart, but we are updating the config map and if something is broken in the config, istiod restart would catch that.

linsun commented 2 years ago

@bochengchu thanks for submitting this for review! I think it is reasonable overall. I do have 1 comment while reading docs that I would like to be addressed before going to beta. The current doc says:

Restart Istiod to allow the change to take effect with the following command:

$ kubectl rollout restart deployment/istiod -n istio-system
deployment.apps/istiod restarted

Is this restart required? Seems odd to require restart of istiod after modifying the istio config map.

I agree we don’t require a restart, but we are updating the config map and if something is broken in the config, istiod restart would catch that.

I don't think we require restart Istiod anywhere in our doc these days. Requiring restart is a yellow flag to promote this to beta. I think we should test with no restart and ensure it can work.

bochengchu commented 2 years ago

@bochengchu thanks for submitting this for review! I think it is reasonable overall. I do have 1 comment while reading docs that I would like to be addressed before going to beta. The current doc says:

Restart Istiod to allow the change to take effect with the following command:

$ kubectl rollout restart deployment/istiod -n istio-system
deployment.apps/istiod restarted

Is this restart required? Seems odd to require restart of istiod after modifying the istio config map.

I agree we don’t require a restart, but we are updating the config map and if something is broken in the config, istiod restart would catch that.

I don't think we require restart Istiod anywhere in our doc these days. Requiring restart is a yellow flag to promote this to beta. I think we should test with no restart and ensure it can work.

I have tested it and it works as expected without restart.

bochengchu commented 2 years ago

PTAL @linsun @ericvn @therealmitchconnors

ericvn commented 2 years ago

LGTM.

ericvn commented 2 years ago

/cherry-pick release-1.16

istio-testing commented 2 years ago

@ericvn: once the present PR merges, I will cherry-pick it on top of release-1.16 in a new PR and assign it to you.

In response to [this](https://github.com/istio/enhancements/pull/125#issuecomment-1285433995): >/cherry-pick release-1.16 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
linsun commented 2 years ago

LGTM, thanks for addressing my comments earlier @bochengchu !

nrjpoddar commented 2 years ago

LGTM, awesome work!

istio-testing commented 2 years ago

@ericvn: new pull request created: #129

In response to [this](https://github.com/istio/enhancements/pull/125#issuecomment-1285433995): >/cherry-pick release-1.16 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.