Cert file path override fixes. by drichelson · Pull Request #13367 · istio/istio

drichelson · 2019-04-16T18:39:27Z

This PR addresses #11984
It is a followup to #12189
Please see api PR: istio/api#902 as it will need to be merged before this can be finalized.

Addresses a few bugs from the original PR and adds the ability to override client certs (set in DestinationRule) with envoy node metadata.

I've included the api changes in the vendor dir. I will remove them once the api PR is merged. This is causing the lint failure.

…arg.

…tadata

istio-testing · 2019-04-16T18:39:43Z

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

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.

drichelson · 2019-04-16T18:40:51Z

pilot/cmd/pilot-agent/main.go

 			// Add cert paths as node metadata only if they differ from defaults
 			if tlsServerCertChain != model.DefaultCertChain {
-				role.Metadata[model.NodeMetadataTLSServerCertChain] = tlsServerCertChain
+				proxyConfig.EnvoyNodeMetadata[model.NodeMetadataTLSServerCertChain] = tlsServerCertChain


The role.Metadata was never initialized or read, so setting it was a fail. Instead we are using the new EnvoyNodeMetadata field on the ProxyConfig struct. This gets passed in to the bootstrap code so it seems like the logical place for it to live.

instead of piggybacking on metadata, cant you do this check after https://github.com/istio/istio/pull/13367/files#diff-5cb6a377bcea0cba4f2dd118a326af06R181 has been called?

@rshriram I'm not sure what you mean by "piggybacking on metadata". The cert file overrides are used by both the file watcher here in pilot-agent and they are sent as node metadata so they can be used by pilot to configure listeners + clusters.

drichelson · 2019-04-16T18:41:05Z

pilot/cmd/pilot-agent/main.go

 	proxyCmd.PersistentFlags().StringVar(&tlsClientCertChain, "tlsClientCertChain",
 		model.DefaultCertChain, "Absolute path to client cert-chain file used for istio mTLS")
-	proxyCmd.PersistentFlags().StringVar(&tlsClientKey, "tlsSClientKey",
+	proxyCmd.PersistentFlags().StringVar(&tlsClientKey, "tlsClientKey",


fixing typo.

ericvn · 2019-04-17T20:34:47Z

/ok-to-test

rshriram · 2019-04-18T15:04:46Z

can you clarify this:

adds the ability to override client certs (set in DestinationRule) with envoy node metadata.

Destination rules merely specify the paths. they dont provide certs. So a bit more context would help.

drichelson · 2019-04-18T16:36:37Z

@rshriram These changes are all about cert file paths, not cert contents.

I'll outline the current scenario for our (Salesforce) mesh:
Current behavior:

MUTUAL_TLS mode
DestinationRule specifies client cert paths for mesh traffic. These client cert paths become the default for our mesh. For K8s mesh nodes we mount certs to these paths and everything works well.
In the same mesh: We have baremetal/VM nodes where we don't control cert file paths. This PR allows for us to override the client cert file paths in the DestinationRule with per-node settings (via command line args->node metadata)

I hope that helps.

rshriram · 2019-04-22T07:23:56Z

Sorry for the delayed response and sorry to keep this pr waiting. I understand the setup and the requirement to customize cert path. What I am uncomfortable about is changing the mesh config api to add the default node metadata under the default proxy config section.

This will lead to others stuffing additional metadata in the helm chart that might conflict with ones inserted by us per sidecar in the sidecar injector. What takes precedence ? Default or one in sidecar injector that appears as env vars? I believe it’s latter but that behavior needs documenting and good explanation.

If we can do this without modifying the mesh config proto, it would be great. Otherwise lets get this in with proper docs.

drichelson · 2019-04-23T15:28:17Z

@rshriram Does the comment in the api PR make sense? Beyond the comment, where in the docs should I be updating info about node metadata?

drichelson · 2019-04-23T20:38:17Z

@rshriram:

"If we can do this without modifying the mesh config proto, it would be great"

I'm thinking of this as a simpler approach: Abandon command line args for setting cert paths and instead set the ISTIO_META env vars for custom cert paths. The cert watcher can check for these env vars and watch non-default paths if they are set.

Thoughts?

rshriram · 2019-04-24T10:08:11Z

That works!

istio-testing · 2019-04-24T16:52:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @nmittler 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:

pilot/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 · 2019-04-24T17:37:00Z

@drichelson: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-integ-k8s-tests.sh	`b058687`	link	`/test istio-integ-k8s-tests`
prow/istio-pilot-multicluster-e2e.sh	`b058687`	link	`/test istio-pilot-multicluster-e2e`

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.

Dan Richelson added 12 commits April 11, 2019 09:48

Pilot-agent: instantiate proxy metadata map. Fix typo in arg.

5d27925

Pilot-agent: Properly wire in cert file paths from args. Fix typo in …

436e4c0

…arg.

Merge branch 'release-1.1' into pilotAgentCertConfigFixes

e626554

Pilot-discovery: allow cert file locations to be overriden by node me…

540b397

…tadata

Pilot-discovery: allow cert file locations to be overriden by node me…

7ce347e

…tadata

Fix import. improve test

5e37b1a

Rename function

b8bb3c8

rename function

aa0082c

Remove metadata validation

544d28f

Merge branch 'release-1.1' into pilotAgentCertConfigFixes

080e0dd

Cluster: add test for overriding client certs with node metadata

298a2a1

Fix linter/formatter

742c774

istio-testing requested review from frankbu and sebastienvas April 16, 2019 18:39

googlebot added the cla: yes label Apr 16, 2019

istio-testing added the needs-ok-to-test label Apr 16, 2019

drichelson commented Apr 16, 2019

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Apr 17, 2019

drichelson mentioned this pull request Apr 18, 2019

Fixes panic in pilot agent when provided with custom cert paths. #13409

Merged

Merge branch 'release-1.1' into pilotAgentCertConfigFixes

a93e1ee

Address PR comments

0b1e433

Add test

b058687

rshriram approved these changes Apr 24, 2019

View reviewed changes

rshriram merged commit be08091 into istio:release-1.1 Apr 24, 2019

rshriram mentioned this pull request Aug 17, 2020

Do not use env var specified certs for dest rule #26558

Closed

Conversation

drichelson commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Apr 16, 2019

Uh oh!

drichelson Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

drichelson Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

drichelson Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

ericvn commented Apr 17, 2019

Uh oh!

rshriram commented Apr 18, 2019

Uh oh!

drichelson commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented Apr 22, 2019

Uh oh!

drichelson commented Apr 23, 2019

Uh oh!

drichelson commented Apr 23, 2019

Uh oh!

rshriram commented Apr 24, 2019

Uh oh!

istio-testing commented Apr 24, 2019

Uh oh!

istio-testing commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drichelson commented Apr 16, 2019 •

edited

Loading

drichelson commented Apr 18, 2019 •

edited

Loading