[WIP] Allow identity domain to be configured in istio by c0d1ngm0nk3y · Pull Request #8976 · istio/istio

c0d1ngm0nk3y · 2018-09-26T13:30:32Z

see issue #8661

Use identity domain parameter to configure spiffe to use a different domain than "cluster.local".

googlebot · 2018-09-26T13:30:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

istio-testing · 2018-09-26T13:30:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
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

Details

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

istio-testing · 2018-09-26T13:30:48Z

Hi @c0d1ngm0nk3y. 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.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

c0d1ngm0nk3y · 2018-09-26T13:35:47Z

Still work in progress: If mTLS is not used, the pilot proxy is crashing.

/cc @lita
/cc @myidpt
/cc @elevran

codecov · 2018-09-26T14:29:37Z

Codecov Report

Merging #8976 into master will decrease coverage by 1%.
The diff coverage is 71%.

@@           Coverage Diff           @@
##           master   #8976    +/-   ##
=======================================
- Coverage      69%     69%   -<1%     
=======================================
  Files         412     425    +13     
  Lines       38423   38617   +194     
=======================================
+ Hits        26433   26550   +117     
- Misses      10740   10860   +120     
+ Partials     1250    1207    -43

Impacted Files	Coverage Δ
pilot/pkg/networking/plugin/authz/rbac.go	`82% <ø> (ø)`	⬆️
pilot/pkg/model/context.go	`75% <ø> (+1%)`	⬆️
pkg/bootstrap/bootstrap_config.go	`39% <0%> (+2%)`	⬆️
security/pkg/nodeagent/sds/sdsservice.go	`63% <0%> (ø)`	⬆️
pilot/pkg/proxy/envoy/v2/debug.go	`33% <0%> (ø)`	⬆️
pilot/pkg/serviceregistry/memory/discovery.go	`57% <0%> (ø)`	⬆️
pilot/pkg/serviceregistry/consul/controller.go	`71% <0%> (ø)`	⬆️
security/pkg/platform/onprem.go	`67% <0%> (ø)`	⬆️
security/pkg/pki/util/san.go	`89% <100%> (ø)`	⬇️
security/pkg/registry/kube/serviceaccount.go	`77% <100%> (ø)`	⬆️
... and 92 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 6717060...1ae85f0. Read the comment docs.

wattli · 2018-09-26T18:07:54Z

install/kubernetes/helm/istio/values-istio-one-namespace-identity-domain.yaml

update comment?

wattli · 2018-09-26T18:12:03Z

install/kubernetes/helm/istio/values-istio-one-namespace-identity-domain.yaml

This file seems to be a testing file, if so, put it under testing dir?

wattli · 2018-09-26T18:12:27Z

pilot/cmd/pilot-agent/main.go

can we put spiffe under security/pkg/... since it's mostly about security feature.

wattli · 2018-09-26T18:13:57Z

pilot/cmd/pilot-agent/main.go

The name of this func is very confusing. Please rename to something more meaningful.

And it would be great to have some input/output values for testing purpose.

wattli · 2018-09-26T18:15:36Z

pilot/cmd/pilot-agent/main.go

Please add comments for what it does.

And when leave a TODO, please add the corresponding person for this TODO.

myidpt · 2018-09-27T16:23:08Z

Thanks for the great work. Could you split this PR into smaller ones? It's hard for us to review.

axel7born · 2018-09-28T14:30:26Z

@myidpt, your right, the PR got to big. We are preparing smaller ones.
@wattli, thanks for looking at our changes. We will take your comments into account for the smaller PRs.

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>

This reverts commit 793a43059b699b286e89e5249614ce90bb2db48c.

kramerul · 2018-10-09T14:15:24Z

@myidpt We spitted up this pull request into 3 pull requests:
#9059, #9090 and #9226
It would be nice, if we could do the move of spiffe to security/pkg after these pull requests are merged.

This pull request is no longer needed. It's replaced by the requests mentioned above.

c0d1ngm0nk3y · 2018-10-12T06:30:34Z

see comment, this PR has been splitted.

istio-testing requested review from andraxylia, ayj, costinm, hklai, linsun, nmittler, ozevren, wattli and yangminzhu September 26, 2018 13:30

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Sep 26, 2018

istio-testing added the needs-ok-to-test label Sep 26, 2018

c0d1ngm0nk3y mentioned this pull request Sep 26, 2018

Allow configurable trust domain in Istio identities #8661

Closed

4 tasks

wattli reviewed Sep 26, 2018

View reviewed changes

c0d1ngm0nk3y changed the title ~~Allow identity domain to be configured in istio~~ [WIP] Allow identity domain to be configured in istio Sep 27, 2018

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 27, 2018

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 30, 2018

axel7born mentioned this pull request Oct 1, 2018

Add flag trustDomain to pilot. #9059

Merged

axel7born and others added 6 commits October 1, 2018 09:22

Move value identityDomain to global scope.

5e974ca

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Refactor to make pilotSAN testable

70a4bab

Add unit test for existing pilotSAN determination

3c8ff38

Add IdentityDomain to model.Proxy

61bd92a

Move lines after compare to upstream

bfd5c9c

Add parameter --identity-domain to start pilot

57d5683

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

axel7born and others added 9 commits October 1, 2018 09:22

Move everything related to spiffe URIs to package spiffe

89392ef

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Refactor identity domain handling and adapt unit tests

7367cc1

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Add copyrights and change format to match lint tools

ff65c55

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Add identity domain feature to integration tests

60bb9ac

Co-authored-by: Ulrich Kramer <u.kramer@sap.com>

Keep pilot SAN nil if not mTLS

6624da8

Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>

Fix misbehaviour for citadel on kubernetes

1d11d3b

Fix usage of identity domain if not mTLS

015df57

Refactor setting of identity domain

5cf5842

Revert "Keep pilot SAN nil if not mTLS"

1ae85f0

This reverts commit 793a43059b699b286e89e5249614ce90bb2db48c.

jsPlachetka force-pushed the pilot-trust-domain branch from 8e022a3 to 1ae85f0 Compare October 1, 2018 07:22

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 1, 2018

jsPlachetka mentioned this pull request Oct 2, 2018

Move everything related to spiffe URIs to package spiffe #9090

Merged

c0d1ngm0nk3y closed this Oct 12, 2018

kramerul mentioned this pull request Mar 22, 2019

REQUEST: New membership for kramerul istio/community#160

Closed

4 tasks

Conversation

c0d1ngm0nk3y commented Sep 26, 2018

Uh oh!

googlebot commented Sep 26, 2018

Uh oh!

istio-testing commented Sep 26, 2018

Uh oh!

istio-testing commented Sep 26, 2018

Uh oh!

c0d1ngm0nk3y commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

wattli Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

myidpt commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axel7born commented Sep 28, 2018

Uh oh!

kramerul commented Oct 9, 2018

Uh oh!

c0d1ngm0nk3y commented Oct 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

c0d1ngm0nk3y commented Sep 26, 2018 •

edited

Loading

codecov bot commented Sep 26, 2018 •

edited

Loading

myidpt commented Sep 27, 2018 •

edited

Loading