do not hardcode cert watch path in pilot agent by drichelson · Pull Request #12142 · istio/istio

drichelson · 2019-02-28T00:14:35Z

Addresses #11984 for pilot-agent.

Note: Changes made in vendor directory will be reverted and a proper update of the api dependency will be committed once istio/api#824 is merged

istio-testing · 2019-02-28T00:14:41Z

[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: andraxylia

If they are not already assigned, you can assign the PR to them by writing /assign @andraxylia 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
vendor/istio.io/api/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-02-28T00:14:43Z

@drichelson: PR needs rebase.

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.

istio-testing · 2019-02-28T00:14:51Z

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.

golangcibot · 2019-02-28T00:16:05Z

pilot/pkg/model/validation_test.go

+			isValid: false,
+		},
+		{
+			name:    "cert files to watch partially invalid",


File is not goimports-ed (from goimports)

drichelson · 2019-02-28T00:16:12Z

pilot/pkg/proxy/envoy/watcher.go

-
 type watcher struct {
-	certs   []CertSource
+	certs   []string


The watcher code now takes a list of absolute file paths to watch. This allows for maximum flexibility when cert file names and locations are dictated by outside forces.

rshriram · 2019-02-28T01:17:45Z

/ok-to-test

istio-testing · 2019-02-28T01:56:40Z

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

Test name	Commit	Details	Rerun command
prow/e2e-simpleTests.sh	`563a496`	link	`/test e2e-simpleTests`
prow/e2e-bookInfoTests-v1alpha3.sh	`563a496`	link	`/test e2e-bookInfoTests-envoyv2-v1alpha3`
prow/e2e_pilotv2_auth_sds.sh	`563a496`	link	`/test istio_auth_sds_e2e`
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh	`563a496`	link	`/test istio-pilot-e2e-envoyv2-v1alpha3`
prow/e2e-mixer-no_auth.sh	`563a496`	link	`/test e2e-mixer-no_auth`
prow/e2e-dashboard.sh	`563a496`	link	`/test e2e-dashboard`
prow/release-test.sh	`563a496`	link	`/test release-test`
prow/istio-pilot-multicluster-e2e.sh	`563a496`	link	`/test istio-pilot-multicluster-e2e`
prow/istio-unit-tests.sh	`e994dda`	link	`/test istio-unit-tests`
prow/istio-integ-local-tests.sh	`e994dda`	link	`/test istio-integ-local-tests`
prow/istio-presubmit.sh	`e994dda`	link	`/test istio-presubmit`
prow/istio-integ-k8s-tests.sh	`e994dda`	link	`/test istio-integ-k8s-tests`

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.

hzxuzhonghu · 2019-02-28T02:18:42Z

pilot/cmd/pilot-agent/main.go

 			envoyProxy := envoy.NewProxy(proxyConfig, role.ServiceNode(), proxyLogLevel, pilotSAN, role.IPAddresses)
 			agent := proxy.NewAgent(envoyProxy, proxy.DefaultRetry, pilot.TerminationDrainDuration())
-			watcher := envoy.NewWatcher(certs, agent.ConfigCh())
+			watcher := envoy.NewWatcher(proxyConfig.TlsCertsToWatch, agent.ConfigCh())


I can not see how these files are used other than watch them ans restart envoy.

true. dont you have to update the mTLS plugin in pilot to generate envoy TLS contexts to use the right paths?

@hzxuzhonghu: correct. The actual certs that envoy uses come from other configs:

xDS (pilot-discovery) mTLS certs are defined in static config.

All cert info for proxied requests is passed down from xDS config.

Moving forward my understanding is that Envoy's SDS will handle more of #2.

my point is XDS still uses the old path /etc/certs.. the code change here only tells pilot agent to watch the new path. but Pilot (discovery) is still using the old one

@rshriram We'll open a separate issue/PR for any pilot-discovery changes needed to customize these cert paths if that's ok.

No, I think we need fix it here.

drichelson · 2019-02-28T17:05:12Z

@rshriram This PR depends on an update of the vendored istio.io/api dependency. I have it updated locally. Should that change be a part of this PR, or should api get updated in a separate workflow?

drichelson · 2019-02-28T17:44:59Z

Due to the large number of changes in the istio.io/api dependency I created a separate PR for updating it: #12160

rshriram · 2019-02-28T21:19:48Z

Hi, per my other comment, could you send the API pr to master branch and send this PR to master branch as well ? This is a two part fix and I am not sure if both will end up in Pilot before the 1.1 release. I would much rather have a complete feature in 1.1 vs a partial one. We can always cherry pick these fixes into a 1.1.x patch release, as its a trivial fix

rshriram · 2019-02-28T21:21:25Z

pilot/cmd/pilot-agent/main.go

+					path.Join(model.AuthCertsPath, model.KeyFilename),
+					path.Join(model.AuthCertsPath, model.RootCertFilename),
+				}
+				if role.Type == model.Ingress {


I think yu can get rid of this ingress cert thing.. and restrict this to just the mTLS certs. we are using SDS for ingress and gateways. and the certs specified in both do not have to have a standard path. So thsi code is likely a legacy leftover..

rshriram

okay with the changes as it is, but please move to master

drichelson · 2019-03-01T19:59:38Z

Closing and creating a new more comprehensive PR that targets master branch

WIP: Proxy: Add command line arg to specify tls certs to watch.

61a33c8

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

googlebot added the cla: yes label Feb 28, 2019

istio-testing requested review from kyessenov and wattli February 28, 2019 00:14

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

istio-testing added the needs-ok-to-test label Feb 28, 2019

golangcibot reviewed Feb 28, 2019

View reviewed changes

drichelson commented Feb 28, 2019

View reviewed changes

WIP: update temporary vendored files to match companion PR

563a496

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 Feb 28, 2019

Dan Richelson added 2 commits February 27, 2019 17:51

WIP: update temporary vendored files to match companion PR

8a3ae27

WIP: fix formatting

e994dda

hzxuzhonghu reviewed Feb 28, 2019

View reviewed changes

rshriram changed the title ~~WIP: do not merge. Proxy cert watch config~~ do not hardcode cert watch path in pilot agent Feb 28, 2019

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

drichelson mentioned this pull request Feb 28, 2019

Make cert location watched by pilot agent configurable #11984

Closed

rshriram reviewed Feb 28, 2019

View reviewed changes

rshriram approved these changes Feb 28, 2019

View reviewed changes

drichelson closed this Mar 1, 2019

drichelson mentioned this pull request Mar 1, 2019

Configuration: no longer hardcode mesh certs #12189

Merged

Conversation

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

Uh oh!

Uh oh!

istio-testing commented Feb 28, 2019

Uh oh!

istio-testing commented Feb 28, 2019

Uh oh!

istio-testing commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Feb 28, 2019

Uh oh!

istio-testing commented Feb 28, 2019

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!

Choose a reason for hiding this comment

Uh oh!

drichelson commented Feb 28, 2019

Uh oh!

drichelson commented Feb 28, 2019

Uh oh!

rshriram commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

drichelson commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

drichelson commented Feb 28, 2019 •

edited

Loading