Define CRDs for ServiceRole and ServiceRoleBinding in pilot #4246

yangminzhu · 2018-03-14T00:49:35Z

create corresponding CRD for ServiceRole and ServiceRoleBinding (See
https://github.com/istio/api/blob/master/rbac/v1alpha1/rbac.proto).
The ServiceRole and ServiceRoleBinding is currently defined in istio-auth.yaml
but we have plans to migrate them to pilot for the local RBAC.
add corresponding validation code for new model types (Validation for Istio RBAC policies #3927).
in longer term after the local RBAC is implemented, the pilot watches for changes on Istio RBAC policies, fetches the updated RBAC policies from Istio Config Store and distributes relevant Istio RBAC policies to Envoy proxies that are co-located with service instances and enforce the policies inside Envoy (The mixer won't handle the RBAC policies anymore).

See the design doc (WIP) for more details: https://docs.google.com/document/d/1XkN9ls-ZDISz2uXGN20RaIre63DkJJOJNUOzcKCxbWA/edit?usp=sharing

1. create corresponding CRD for ServiceRole and ServiceRoleBinding (See https://github.com/istio/api/blob/master/rbac/v1alpha1/rbac.proto). The ServiceRole and ServiceRoleBinding is currently defined in istio-auth.yaml but we have plans to migrate them to pilot for the local RBAC. 2. add corresponding validation code for new model types (istio#3927).

codecov · 2018-03-14T01:13:52Z

Codecov Report

Merging #4246 into master will decrease coverage by 1%.
The diff coverage is 73%.

@@           Coverage Diff           @@
##           master   #4246    +/-   ##
=======================================
- Coverage      74%     74%   -<1%     
=======================================
  Files         313     313            
  Lines       29039   28968    -71     
=======================================
- Hits        21269   21164   -105     
- Misses       6451    6490    +39     
+ Partials     1319    1314     -5

Impacted Files	Coverage Δ
pilot/pkg/model/config.go	`64% <ø> (ø)`	⬆️
pilot/pkg/bootstrap/server.go	`41% <ø> (+1%)`	⬆️
pilot/pkg/model/validation.go	`90% <100%> (+1%)`	⬆️
pilot/pkg/config/kube/crd/types.go	`63% <62%> (ø)`	⬇️
pilot/pkg/serviceregistry/kube/cache.go	`63% <0%> (-37%)`	⬇️
pilot/pkg/config/aggregate/config.go	`78% <0%> (-16%)`	⬇️
pilot/pkg/config/monitor/monitor.go	`69% <0%> (-5%)`	⬇️
mixer/adapter/servicecontrol/testhelper.go	`72% <0%> (-2%)`	⬇️
pilot/pkg/bootstrap/monitoring.go	`64% <0%> (-1%)`	⬇️
mixer/adapter/list/list.go	`100% <0%> (ø)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88cf557...3eeab4e. Read the comment docs.

liminw

Looks great! Thanks.

ayj · 2018-03-15T19:59:49Z

The code LGTM. General question: what does Pilot do with this new configuration, i.e. who is the consumer?

yangminzhu · 2018-03-15T20:18:06Z

Thanks for the review! Currently pilot doesn't handle the new configuration other than the validation (both in admission controller and istioctl). There is a RBAC mixer adapter to consume these actual resources and apply the corresponding RBAC rules.
We have plans to implement the local RBAC and we'll need to change the pilot to directly consume these configuration at that time.

ayj

/lgtm

istio-merge-robot · 2018-03-16T20:54:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayj

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

Needs approval from an approver in each of these OWNERS Files:

~~pilot/OWNERS~~ [ayj]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2018-03-16T20:55:51Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-03-16T21:13:55Z

Automatic merge from submit-queue.

Automatic merge from submit-queue. Rerun generate_config_crd_types.go to update ServiceRole and ServiceRoleBinding types. Change #4246 might use the old script when generate types for the new CRDs.

diemtvu

I think also need to register the new proto schema to istioctl if you want that tool to check it (yeah, I know ;))

https://github.com/istio/istio/blob/master/istioctl/cmd/istioctl/main.go#L650

diemtvu · 2018-03-21T14:47:10Z

pilot/pkg/model/config.go

+		Type:        "service-role",
+		Plural:      "service-roles",
+		Group:       "config",
+		Version:     istioAPIVersion,


May be you should use separate api group (e.g 'authorization'), with your actual api version + @xiaolanz

Hey, if this is a new CRD, it should be in group= "rbac" and version= v1alpha1 to follow API style guide https://github.com/istio/api/blob/master/STYLE-GUIDE.md#crd-versioning

It's not a new CRD, it's currently defined here (group='config' and version='v1alpha2').

yangminzhu · 2018-03-21T18:09:41Z

@diemtvu Thanks for catching this!
I just realized that the istioctl has 2 different sources of scheme definition. The validation part actually works as the readInputs() is using model.IstioConfigTypes, but other parts of istioctl is using newClient().

…4428) Automatic merge from submit-queue. Add ServiceRole and ServiceRoleBinding to istioctl CRD definition. The CRD definition was added to pilot in #4246.

yangminzhu requested review from liminw and a team March 14, 2018 00:49

googlebot added the cla: yes label Mar 14, 2018

liminw approved these changes Mar 14, 2018

View reviewed changes

liminw requested a review from ayj March 14, 2018 17:35

ayj approved these changes Mar 16, 2018

View reviewed changes

istio-testing assigned ayj Mar 16, 2018

istio-testing added the lgtm label Mar 16, 2018

istio-merge-robot added the approved label Mar 16, 2018

istio-merge-robot merged commit 203e98a into istio:master Mar 16, 2018

This was referenced Mar 21, 2018

Rerun generate_config_crd_types.go to update ServiceRole and ServiceRoleBinding types. #4417

Merged

Test or pre-submit check to make sure types.go is consistent with output from generate_config_crd_types.go #4418

Closed

diemtvu reviewed Mar 21, 2018

View reviewed changes

yangminzhu mentioned this pull request Mar 21, 2018

Add ServiceRole and ServiceRoleBinding to istioctl CRD definition. #4428

Merged

yangminzhu deleted the addValidation branch March 1, 2019 19:34

yangminzhu added the area/security/aaa label Mar 8, 2019

yangminzhu added the area/security label Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Define CRDs for ServiceRole and ServiceRoleBinding in pilot #4246

Define CRDs for ServiceRole and ServiceRoleBinding in pilot #4246

Uh oh!

yangminzhu commented Mar 14, 2018 •

edited

Loading

Uh oh!

codecov bot commented Mar 14, 2018 •

edited

Loading

Uh oh!

liminw left a comment

Uh oh!

ayj commented Mar 15, 2018

Uh oh!

yangminzhu commented Mar 15, 2018

Uh oh!

ayj left a comment

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

diemtvu left a comment

Uh oh!

diemtvu Mar 21, 2018

Uh oh!

xiaolanz Mar 21, 2018 •

edited

Loading

Uh oh!

yangminzhu Mar 21, 2018

Uh oh!

yangminzhu commented Mar 21, 2018

Uh oh!

Uh oh!

Define CRDs for ServiceRole and ServiceRoleBinding in pilot #4246

Define CRDs for ServiceRole and ServiceRoleBinding in pilot #4246

Uh oh!

Conversation

yangminzhu commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liminw left a comment

Choose a reason for hiding this comment

Uh oh!

ayj commented Mar 15, 2018

Uh oh!

yangminzhu commented Mar 15, 2018

Uh oh!

ayj left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

diemtvu left a comment

Choose a reason for hiding this comment

Uh oh!

diemtvu Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

xiaolanz Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangminzhu Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

yangminzhu commented Mar 21, 2018

Uh oh!

Uh oh!

yangminzhu commented Mar 14, 2018 •

edited

Loading

codecov bot commented Mar 14, 2018 •

edited

Loading

xiaolanz Mar 21, 2018 •

edited

Loading