add sds client/server example for manual testing by wenchenglu · Pull Request #3803 · istio/istio

wenchenglu · 2018-02-27T04:03:42Z

No description provided.

codecov · 2018-02-27T06:20:42Z

Codecov Report

Merging #3803 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3803    +/-   ##
=======================================
- Coverage      76%     76%   -<1%     
=======================================
  Files         291     291            
  Lines       26869   26825    -44     
=======================================
- Hits        20379   20315    -64     
- Misses       5178    5203    +25     
+ Partials     1312    1307     -5

Impacted Files	Coverage Δ
mixer/adapter/rbac/controller.go	`39% <0%> (-15%)`	⬇️
mixer/adapter/stackdriver/helper/common.go	`63% <0%> (-15%)`	⬇️
mixer/adapter/stackdriver/stackdriver.go	`55% <0%> (-15%)`	⬇️
mixer/adapter/solarwinds/metrics_handler.go	`75% <0%> (-8%)`	⬇️
mixer/adapter/servicecontrol/handler.go	`39% <0%> (-7%)`	⬇️
mixer/adapter/opa/opa.go	`79% <0%> (-3%)`	⬇️
...olarwinds/internal/papertrail/papertrail_logger.go	`59% <0%> (-3%)`	⬇️
mixer/adapter/denier/denier.go	`93% <0%> (ø)`	⬇️
mixer/adapter/stdio/stdio.go	`100% <0%> (ø)`	⬆️
broker/pkg/model/config/store.go	`100% <0%> (ø)`	⬆️
... and 6 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 c24c689...12872ee. Read the comment docs.

wattli · 2018-02-27T17:10:53Z

Gopkg.lock

  analyzer-name = "dep"
  analyzer-version = 1
-  inputs-digest = "64e85d7b234617a9afbc0a95c64d1716f88ea99a3bbf475ab377c8d92f1100e4"
+  inputs-digest = "29d2c4c3d4cd0875ee211ab5e55c87a9554e4fb76b3ecec300dd055fd0014c97"


You might want to revert this file, which causes circleci dependency error in presubmit: https://circleci.com/gh/istio/istio/38114?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

wenchenglu · 2018-02-27T17:54:41Z

done

myidpt · 2018-02-27T18:16:59Z

/lgtm
We could add the server code to this package: https://github.com/istio/istio/tree/master/security/pkg/workload
This package holds libraries for interactions between node agent and workload for delivering key/cert, trust bundles, etc.

istio-merge-robot · 2018-02-27T18:17:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: myidpt

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

Details

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

~~security/OWNERS~~ [myidpt]

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

wenchenglu · 2018-02-27T18:36:00Z

@myidpt
good point, will have a follow up cl to refactor this to library in the directory you suggested

kyessenov · 2018-02-27T18:46:36Z

Are there plans to aggregate SDS with regular xDS? Specifically, are there any consistency issues with secrets not being available for routes?

…

On Tue, Feb 27, 2018, 10:37 AM Wencheng Lu ***@***.***> wrote: @myidpt <https://github.com/myidpt> good point, will have a follow up cl to refactor this to library in the directory you suggested — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3803 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJGIxjRqbQ62gPk3BygceyJ9NU9YiBuAks5tZEsZgaJpZM4SUUrM> .

myidpt · 2018-02-27T18:59:25Z

@wenchenglu That sounds good to me.

istio-merge-robot · 2018-02-27T19:01:25Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @myidpt @wenchenglu

myidpt · 2018-02-27T19:42:03Z

@kyessenov I think currently we don't plan to aggregate SDS with xDS. For the consistency issue, I guess we are still implementing the SDS on Envoy. I think it's reasonable to start with having Envoy disable the upstream traffic relying on the key/certs, when the key/certs are not present.

wenchenglu · 2018-02-27T19:51:17Z

@kyessenov
This is solely for the bootstrap path to get key/cert locally from a local node agent. I don't expect aggregatingg SDS with xDS in this path. Once key/cert got bootstrapped for the workload, we might be able to aggregate SDS for remote key/cert exchange with other xDS if needed.

although I am not sure what you meant by "any consistency issues with secrets not being available for routes"?

costinm

Looks good in general - it would be great to put this into the node agent main.

costinm · 2018-02-27T19:51:19Z

security/cmd/node_agent/sds/server.go

+	if *tls {
+		creds, err := credentials.NewServerTLSFromFile(*certFile, *keyFile)
+		if err != nil {
+			log.Fatalf("Failed to generate credentials %v", err)


Failed to load.

Should it wait for some time ( since certs show up with some delay ) ? Or fail ?

Also what's the use case - isn't it over UDS or localhost ?

yes, it is over UDS. This is a server code for manual test. I'll refactor this PR to a library with UDS support only

costinm · 2018-02-27T19:53:33Z

security/cmd/node_agent/sds/server.go

+}
+
+// StreamSecrets is not supported.
+func (s *SDSServer) StreamSecrets(stream sds.SecretDiscoveryService_StreamSecretsServer) error {


Did you test this with envoy ? My understanding was that fetch is called by REST ( which is json ), while gRPC is calling stream.

Fetch is fine for this use-case. You do need to a gRPC gateway for HTTP mapping.

costinm · 2018-02-27T20:00:48Z

For the 'client' side - it would be great to try it out by changing the envoy bootstrap file, so we can get a
test with the secret watcher disabled and real envoy getting secrets from the node agent. This may solve
quite a few problems, excited to see it moving ahead !

istio-testing · 2018-02-27T20:11:31Z

@wenchenglu: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-simpleTests.sh	`12872ee`	link	`/test e2e-simple`

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. I understand the commands that are listed here.

kyessenov · 2018-02-27T20:43:33Z

@myidpt @wenchenglu The consistency issue I have in mind is that if any LDS (or RDS?) resource references a secret that's not delivered through SDS, then those LDS/RDS resources get rejected by envoy. If they don't get rejected, envoy would blackhole the traffic since secrets are not delivered at the same time.

kyessenov · 2018-02-27T20:44:10Z

security/cmd/node_agent/sds/server.go

+
+// FetchSecrets fetches the X.509 key/cert for a given workload whose identity
+// can be derived from the UDS path where this call is received.
+func (s *SDSServer) FetchSecrets(ctx context.Context, request *api.DiscoveryRequest) (*api.DiscoveryResponse, error) {


As an optimization, you can ignore requests that have version info "0".

This is a server code mainly for manual test. how to set versioninfo is still to be figured out.

wenchenglu · 2018-02-27T22:10:04Z

@kyessenov
Thanks for bringing up the consistency issue. This PR is mostly for bringing up SDS server on the node agent side. We will have a separate effort to address the client (Envoy) side regarding the LDS/RDS issue. @mangchiandjjoe is working on that. We can have this discussion separately.

add sds client/server example for manual testing

d113cff

wenchenglu requested a review from a team February 27, 2018 04:03

googlebot added the cla: yes label Feb 27, 2018

wenchenglu requested a review from wattli February 27, 2018 04:04

fix dependencies

afb1f65

wenchenglu requested a review from a team February 27, 2018 06:20

wattli approved these changes Feb 27, 2018

View reviewed changes

remove testdata dependency

6b161bb

fix format with gofmt -s

aa0a295

istio-testing assigned myidpt Feb 27, 2018

istio-testing added the lgtm label Feb 27, 2018

istio-merge-robot added the approved label Feb 27, 2018

fix lint errors

fdbeb33

istio-merge-robot removed the lgtm label Feb 27, 2018

fix lint error

12872ee

costinm approved these changes Feb 27, 2018

View reviewed changes

kyessenov reviewed Feb 27, 2018

View reviewed changes

wenchenglu merged commit cd82ed4 into istio:master Feb 27, 2018

Conversation

wenchenglu commented Feb 27, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenchenglu commented Feb 27, 2018

Uh oh!

myidpt commented Feb 27, 2018

Uh oh!

istio-merge-robot commented Feb 27, 2018

Uh oh!

wenchenglu commented Feb 27, 2018

Uh oh!

kyessenov commented Feb 27, 2018 via email

Uh oh!

myidpt commented Feb 27, 2018

Uh oh!

istio-merge-robot commented Feb 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

wenchenglu commented Feb 27, 2018

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm commented Feb 27, 2018

Uh oh!

istio-testing commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenchenglu commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Feb 27, 2018 •

edited

Loading

myidpt commented Feb 27, 2018 •

edited

Loading

istio-testing commented Feb 27, 2018 •

edited

Loading