Add authorization policy v1beta1 #918

yangminzhu · 2019-05-07T02:27:51Z

This PR adds the authorization v1beta1 API.

Design doc: https://docs.google.com/document/d/1diwa9oYVwmLtarXb-kfWPq-Ul3O2Ic3b8Y21Z3R8DzQ.

istio-testing · 2019-05-07T02:27:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yangminzhu
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: costinm

If they are not already assigned, you can assign the PR to them by writing /assign @costinm in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rbac/v1alpha2/rbac.proto

pitlv2109 · 2019-05-09T23:36:12Z

Link to istio/istio#12394 for tracking

rbac/v1alpha2/rbac.proto

costinm

@duderino
@louiscryan

Not commenting on the content - but timing. We are 2 weeks before the 1.2 freeze, and I'm not sure this
API will be implemented and has all the required testing ( perf, stability, etc ) in the remaining time.

At the same time, the existing alpha1 is still not fully automated (AFAIK) and probably has code mauve gaps.

Would it make sense to delay this for 1.3 - and focus on code mauve ?

Even if it may be possible to finish implementation in the remaining 6 working days until freeze - I'm worried it will be rushed and not ready for prod.

If the plan is to have alpha-quality implementation for this - it would be better to focus the remaining time
on testing the stuff prod users use and not confuse users.

yangminzhu · 2019-05-11T00:07:02Z

@costinm
This is targeted for 1.3 so no rush for 1.2.

rbac/v1alpha2/rbac.proto

costinm · 2019-05-11T02:23:59Z

Great, but please hold it until we cut the branch, to avoid confusion. And to allow people to focus on tests for 1.2 instead of getting distracted, we have large gaps in auth testing (sds, automating, scale)

…

On Fri, May 10, 2019, 17:07 Yangmin Zhu ***@***.***> wrote: @costinm <https://github.com/costinm> This is targeted for 1.3 so no rush for 1.2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#918 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUR2Q4KENM5BV2DQYYYALPUYE2PANCNFSM4HLEZVXQ> .

rbac/v1alpha2/rbac.proto

pitlv2109 · 2019-05-28T17:30:44Z

Just to double check, are we waiting for 1.2 release before we check in this PR?

pitlv2109 · 2019-08-02T21:14:42Z

I think we can update the API now, given that the design has been approved?

yangminzhu · 2019-08-03T02:52:43Z

@liminw @pitlv2109 PTAL, thanks.

liminw

Thanks for the changes.

In the PR description, add reference to the design doc https://docs.google.com/document/d/1diwa9oYVwmLtarXb-kfWPq-Ul3O2Ic3b8Y21Z3R8DzQ.

security/v1beta1/authorization.proto

liminw

/lgtm

yangminzhu · 2019-08-08T01:29:56Z

@costinm @rshriram Could you review the API change? The design is approved and already shared with the community. Thank you.

rshriram · 2019-08-08T15:13:13Z

security/v1beta1/authorization.proto

+
+// $hide_from_docs
+// WorkloadSelector specifies the scope where the authorization policy should be applied to.
+message WorkloadSelector {


can you use this opportunity to move the workloadSelector from sidecar and use the same one? I do not want to duplicate the workload selector again. Second, you have a different field in workload selector than what sidecar has, which is not a good UX.

rshriram

Please reconcile with the workload selector in sidecar/envoyfilters. We cannot have yet another implementation of workload selector with different semantics (matchLabels, etc.)..

costinm · 2019-08-08T15:16:44Z

+1

…

On Thu, Aug 8, 2019 at 8:15 AM Shriram Rajagopalan ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please reconcile with the workload selector in sidecar/envoyfilters. We cannot have yet another implementation of workload selector with different semantics (matchLabels, etc.).. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#918?email_source=notifications&email_token=AAAUR2TPRUQU7ZHHOQPIAT3QDQ2CRA5CNFSM4HLEZVX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBAEDUA#pullrequestreview-272646608>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUR2RG2TATGLR7MU2KMELQDQ2CRANCNFSM4HLEZVXQ> .

costinm · 2019-08-08T15:18:33Z

And preferably use one of the protos and syntax for selector that already exists - either the one in Sidecar or Envoyfilters.

…

On Thu, Aug 8, 2019 at 8:16 AM Costin Manolache ***@***.***> wrote: +1 On Thu, Aug 8, 2019 at 8:15 AM Shriram Rajagopalan < ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > Please reconcile with the workload selector in sidecar/envoyfilters. We > cannot have yet another implementation of workload selector with different > semantics (matchLabels, etc.).. > > — > You are receiving this because you were assigned. > Reply to this email directly, view it on GitHub > <#918?email_source=notifications&email_token=AAAUR2TPRUQU7ZHHOQPIAT3QDQ2CRA5CNFSM4HLEZVX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBAEDUA#pullrequestreview-272646608>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAAUR2RG2TATGLR7MU2KMELQDQ2CRANCNFSM4HLEZVXQ> > . >

liminw · 2019-08-08T17:24:02Z

Add @louiscryan @smawson

@costinm @rshriram Agree that we should have a single "selector" syntax. Based on Istio Config Model, the following is the only approved "selector" format. Shall we reconcile the existing APIs to move to the agreed syntax?

  selector:
     matchLabels:
          <label selector>

yangminzhu · 2019-08-08T19:14:22Z

@rshriram @costinm @liminw I changed to use the WorkloadSelector in Sidecar. We could move the WorkloadSelector to a common directory if you prefer, I can do it in this PR.

As for Gateway, I would prefer not to change it in this PR. The syntax will change and all code and yaml need to update, and this should be done in a separate PR.

liminw · 2019-08-08T21:04:04Z

In existing code, there are two different selector definitions.

In sidecar

workloadSelector:
   labels:
      app: A

In Gateway,

selector:
   app: A

Both are different from the agreed on selector definition in Istio config model doc. Either picking one of the definitions above, or following the recommendation in Istio config model, we need a standard selector definition, and use it for all APIs consistently.

yangminzhu · 2019-08-09T02:07:25Z

/test api-presubmit

yangminzhu · 2019-08-09T02:15:17Z

Within this PR's scope, I think we have 3 options:

Add new WorkloadSelector that is exactly the same as the one approved in the Istio config model doc
Reuse the WorkloadSelector in Sidecar.proto which is a little bit different from the approved one. The difference is in the field name: labels vs matchLabels
Reuse the WorkloadSelector in Sidecar.proto but also move it to a new common directory.

@rshriram @costinm @louiscryan @smawson @liminw I can do any of the above in this PR, could you let me know which one you prefer? Thank you.

rshriram · 2019-08-09T03:36:47Z

I care less about naming. We need consistency. So, if this new "selector" is the answer, then lets go for it. You can change the EnvoyFilter's workloadSelector to this new Selector thing immediately as it has not been released yet. For the gateway, you have a problem. Sidecars would need some deprecation model..

yangminzhu requested review from pitlv2109 and liminw May 7, 2019 02:27

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 7, 2019

istio-testing requested review from costinm and geeknoid May 7, 2019 02:27

yangminzhu added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 7, 2019

yangminzhu force-pushed the authz-v2 branch 2 times, most recently from 443eb7f to 6b4bab2 Compare May 9, 2019 22:43

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 9, 2019

pitlv2109 reviewed May 9, 2019

View reviewed changes

liminw reviewed May 9, 2019

View reviewed changes

costinm reviewed May 10, 2019

View reviewed changes

liminw reviewed May 11, 2019

View reviewed changes

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

yangminzhu force-pushed the authz-v2 branch 2 times, most recently from 25a45f3 to 9d3b0a5 Compare May 13, 2019 22:26

liminw reviewed May 13, 2019

View reviewed changes

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

yangminzhu force-pushed the authz-v2 branch 2 times, most recently from e62b8fc to e740981 Compare May 14, 2019 01:34

liminw reviewed May 14, 2019

View reviewed changes

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

pitlv2109 reviewed May 15, 2019

View reviewed changes

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

liminw reviewed May 17, 2019

View reviewed changes

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

rbac/v1alpha2/rbac.proto Outdated Show resolved Hide resolved

yangminzhu force-pushed the authz-v2 branch from 5d582f1 to 9283bc4 Compare May 29, 2019 19:06

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 29, 2019

yangminzhu force-pushed the authz-v2 branch 2 times, most recently from 43fe5bf to 459578c Compare August 3, 2019 02:44

liminw reviewed Aug 5, 2019

View reviewed changes

yangminzhu force-pushed the authz-v2 branch from 231f26a to 55ae3b7 Compare August 5, 2019 22:38

liminw reviewed Aug 6, 2019

View reviewed changes

security/v1beta1/authorization.proto Outdated Show resolved Hide resolved

security/v1beta1/authorization.proto Outdated Show resolved Hide resolved

add authorization policy v1beta1

418d100

yangminzhu force-pushed the authz-v2 branch from 55ae3b7 to 4f07aa4 Compare August 8, 2019 00:03

address comments

099ce8f

yangminzhu force-pushed the authz-v2 branch from 4f07aa4 to 099ce8f Compare August 8, 2019 00:14

liminw approved these changes Aug 8, 2019

View reviewed changes

yangminzhu assigned costinm and rshriram Aug 8, 2019

yangminzhu mentioned this pull request Aug 8, 2019

authz-v2: clean out-dated authorization implementation istio/istio#16119

Merged

rshriram reviewed Aug 8, 2019

View reviewed changes

rshriram suggested changes Aug 8, 2019

View reviewed changes

use workload selector in Sidecar

d79fae8

yangminzhu force-pushed the authz-v2 branch from 7c89a95 to d79fae8 Compare August 9, 2019 02:19

rshriram approved these changes Aug 9, 2019

View reviewed changes

istio-testing merged commit b632c65 into istio:master Aug 9, 2019

liminw mentioned this pull request Aug 12, 2019

Add WorkloadSelector definition in a common directory. #1032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authorization policy v1beta1 #918

Add authorization policy v1beta1 #918

yangminzhu commented May 7, 2019 •

edited

Loading

istio-testing commented May 7, 2019

pitlv2109 commented May 9, 2019

costinm left a comment

yangminzhu commented May 11, 2019

costinm commented May 11, 2019 via email

pitlv2109 commented May 28, 2019

pitlv2109 commented Aug 2, 2019

yangminzhu commented Aug 3, 2019

liminw left a comment

liminw left a comment

yangminzhu commented Aug 8, 2019

rshriram Aug 8, 2019

rshriram left a comment

costinm commented Aug 8, 2019 via email

costinm commented Aug 8, 2019 via email

liminw commented Aug 8, 2019

yangminzhu commented Aug 8, 2019 •

edited

Loading

liminw commented Aug 8, 2019

yangminzhu commented Aug 9, 2019

yangminzhu commented Aug 9, 2019 •

edited

Loading

rshriram commented Aug 9, 2019

Add authorization policy v1beta1 #918

Add authorization policy v1beta1 #918

Conversation

yangminzhu commented May 7, 2019 • edited Loading

istio-testing commented May 7, 2019

pitlv2109 commented May 9, 2019

costinm left a comment

Choose a reason for hiding this comment

yangminzhu commented May 11, 2019

costinm commented May 11, 2019 via email

pitlv2109 commented May 28, 2019

pitlv2109 commented Aug 2, 2019

yangminzhu commented Aug 3, 2019

liminw left a comment

Choose a reason for hiding this comment

liminw left a comment

Choose a reason for hiding this comment

yangminzhu commented Aug 8, 2019

rshriram Aug 8, 2019

Choose a reason for hiding this comment

rshriram left a comment

Choose a reason for hiding this comment

costinm commented Aug 8, 2019 via email

costinm commented Aug 8, 2019 via email

liminw commented Aug 8, 2019

yangminzhu commented Aug 8, 2019 • edited Loading

liminw commented Aug 8, 2019

yangminzhu commented Aug 9, 2019

yangminzhu commented Aug 9, 2019 • edited Loading

rshriram commented Aug 9, 2019

yangminzhu commented May 7, 2019 •

edited

Loading

yangminzhu commented Aug 8, 2019 •

edited

Loading

yangminzhu commented Aug 9, 2019 •

edited

Loading