Configuration: no longer hardcode mesh certs by drichelson · Pull Request #12189 · istio/istio

drichelson · 2019-03-01T21:01:51Z

Addresses #11984

Follow-on to #12142 (closed)

…Discovery: no longer hardcode Envoy listener cert paths.

istio-testing · 2019-03-01T21:02:07Z

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-03-01T21:04:30Z

pilot/cmd/pilot-agent/main.go

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


@rshriram mentioned that we probably don't need this. Can this be confirmed? if not I'd prefer to remove it in a separate PR in case it needs to be reverted.

keep it for now

golangcibot · 2019-03-01T21:07:34Z

pilot/pkg/networking/plugin/authn/authentication.go


 // setupFilterChains sets up filter chains based on authentication policy.
-func setupFilterChains(authnPolicy *authn.Policy, sdsUdsPath string, sdsUseTrustworthyJwt, sdsUseNormalJwt bool, meta map[string]string) []plugin.FilterChain {
+func setupFilterChains(authnPolicy *authn.Policy, serverCertPaths *meshconfig.ServerCertPaths, sdsUdsPath string, sdsUseTrustworthyJwt, sdsUseNormalJwt bool, meta map[string]string) []plugin.FilterChain {


line is 204 characters (from lll)

golangcibot · 2019-03-01T21:07:34Z

pilot/pkg/networking/plugin/authn/authentication.go

 	port := in.ServiceInstance.Endpoint.ServicePort
 	authnPolicy := model.GetConsolidateAuthenticationPolicy(in.Env.IstioConfigStore, in.ServiceInstance.Service, port)
-	return setupFilterChains(authnPolicy, in.Env.Mesh.SdsUdsPath, in.Env.Mesh.EnableSdsTokenMount, in.Env.Mesh.SdsUseK8SSaJwt, in.Node.Metadata)
+	return setupFilterChains(authnPolicy, in.Env.Mesh.MeshTrafficServerCertPaths, in.Env.Mesh.SdsUdsPath, in.Env.Mesh.EnableSdsTokenMount, in.Env.Mesh.SdsUseK8SSaJwt, in.Node.Metadata)


line is 181 characters (from lll)

…retchr/testify/assert

golangcibot · 2019-03-01T22:04:12Z

pilot/pkg/networking/plugin/authn/authentication_test.go

 	}
 	for _, c := range cases {
-		if got := setupFilterChains(c.in, c.sdsUdsPath, c.useTrustworthyJwt, c.useNormalJwt, map[string]string{}); !reflect.DeepEqual(got, c.expected) {
+		if got := setupFilterChains(c.in, c.serverCertPaths, c.sdsUdsPath, c.useTrustworthyJwt, c.useNormalJwt, map[string]string{}); !reflect.DeepEqual(got, c.expected) {


line is 165 characters (from lll)

rshriram · 2019-03-04T02:06:54Z

/ok-to-test

rshriram · 2019-03-04T02:12:09Z

You are missing changes to cluster.go (istio mtls setting client side)

drichelson · 2019-03-04T17:55:52Z

@rshriram

You are missing changes to cluster.go (istio mtls setting client side)
I've updated the PR comments noting that this only covers TLSSettings_MUTUAL mode.

Based on my reading of cluster.go: when in TLSSettings_MUTUAL mode we always pull mTLS client cert paths from DestinationRules. Based on this the client certs are already configurable, so we're covered.

Only in when in TLSSettings_ISTIO_MUTUAL mode do we use the hardcoded client certs when building clusters.

Does this make sense? It looks like wiring in the client cert paths for ISTIO_MUTUAL mode will be a bit more gnarly.

…nd use them when setting up listeners

golangcibot · 2019-03-04T23:47:09Z

pilot/pkg/model/context.go

 	NodeMetadataDNSDomains = "DNS_DOMAINS"
+
+	// NodeMetadataTlsServerCertChain is the absolute path to server cert-chain file
+	NodeMetadataTlsServerCertChain = "TLS_SERVER_CERT_CHAIN"


const NodeMetadataTlsServerCertChain should be NodeMetadataTLSServerCertChain (from golint)

golangcibot · 2019-03-04T23:47:10Z

pilot/pkg/model/context.go

+	NodeMetadataTlsServerCertChain = "TLS_SERVER_CERT_CHAIN"
+
+	// NodeMetadataTlsServerKey is the absolute path to server private key file
+	NodeMetadataTlsServerKey = "TLS_SERVER_KEY"


const NodeMetadataTlsServerKey should be NodeMetadataTLSServerKey (from golint)

golangcibot · 2019-03-04T23:47:10Z

pilot/pkg/model/context.go

+	NodeMetadataTlsServerKey = "TLS_SERVER_KEY"
+
+	// NodeMetadataTlsServerRootCert is the absolute path to server root cert file
+	NodeMetadataTlsServerRootCert = "TLS_SERVER_ROOT_CERT"


const NodeMetadataTlsServerRootCert should be NodeMetadataTLSServerRootCert (from golint)

golangcibot · 2019-03-04T23:47:10Z

pilot/pkg/model/context.go

+	NodeMetadataTlsServerRootCert = "TLS_SERVER_ROOT_CERT"
+
+	// NodeMetadataTlsClientCertChain is the absolute path to client cert-chain file
+	NodeMetadataTlsClientCertChain = "TLS_CLIENT_CERT_CHAIN"


const NodeMetadataTlsClientCertChain should be NodeMetadataTLSClientCertChain (from golint)

golangcibot · 2019-03-04T23:47:10Z

pilot/pkg/model/context.go

+	NodeMetadataTlsClientCertChain = "TLS_CLIENT_CERT_CHAIN"
+
+	// NodeMetadataTlsClientKey is the absolute path to client private key file
+	NodeMetadataTlsClientKey = "TLS_CLIENT_KEY"


const NodeMetadataTlsClientKey should be NodeMetadataTLSClientKey (from golint)

golangcibot · 2019-03-04T23:47:10Z

pilot/pkg/model/context.go

+	NodeMetadataTlsClientKey = "TLS_CLIENT_KEY"
+
+	// NodeMetadataTlsClientRootCert is the absolute path to client root cert file
+	NodeMetadataTlsClientRootCert = "TLS_CLIENT_ROOT_CERT"


const NodeMetadataTlsClientRootCert should be NodeMetadataTLSClientRootCert (from golint)

…lusters

drichelson · 2019-03-05T00:39:04Z

pilot/cmd/pilot-agent/main.go

 			role.TrustDomain = spiffe.GetTrustDomain()
 			log.Infof("Proxy role: %#v", role)

+			// Add cert paths as node metadata only if they differ from defaults


Should we always set the metadata? we handle the absence of these fields properly in Pilot-discovery.

costinm · 2019-03-05T03:46:38Z

Do you need to customize the names of the files, or just the directory ? I can see the use case for using a different dir - that's why we have the base dir. ( and watching the base dir is indeed needed - again, the base was mostly intended for local testing - with the plan to move to SDS ).

It feels far too complicated for something that will soon be dead code - and our tradition of paying technical debt and removing dead code is not so great. In general the entire watching business has
major problems - it would be much better to just spend less effort writing a small SDS server that watches files, which will address the reload-without-restart problem as well.

drichelson · 2019-03-05T16:25:20Z

@costinm

Do you need to customize the names of the files, or just the directory ?

We need the path to each cert/key to be custom. This includes separate keys/certs for client and server connections.

I can see the use case for using a different dir - that's why we have the base dir. ( and watching the > base dir is indeed needed - again, the base was mostly intended for local testing - with the plan to > move to SDS ).

It feels far too complicated for something that will soon be dead code - and our tradition of paying technical debt and removing dead code is not so great. In general the entire watching business has
major problems -

it would be much better to just spend less effort writing a small SDS server that watches files, which will address the reload-without-restart problem as well.

A small SDS server makes sense to simplify Pilot-Agent and envoy hot restarts. The changes I made here to watcher code and agent config will be useful and needed for our use case if/when that happens.

drichelson · 2019-03-08T16:58:46Z

@rshriram any thoughts on these changes?

pilot/pkg/networking/core/v1alpha3/cluster.go

+	tls *networking.TLSSettings,
+	serviceAccounts []string,
+	sni string,
+	metadata map[string]string,


Configuration: Pilot-Agent: no longer hardcode certs to watch. Pilot-…

a7dc2e2

…Discovery: no longer hardcode Envoy listener cert paths.

googlebot added the cla: yes label Mar 1, 2019

istio-testing requested review from mandarjog and wattli March 1, 2019 21:01

istio-testing added the needs-ok-to-test label Mar 1, 2019

drichelson commented Mar 1, 2019

View reviewed changes

golangcibot reviewed Mar 1, 2019

View reviewed changes

Dan Richelson added 2 commits March 1, 2019 13:29

Address demands of golangcibot overlord

59770b6

Change usages of github.com/stretchr/testify/require to github.com/st…

13b7608

…retchr/testify/assert

golangcibot reviewed Mar 1, 2019

View reviewed changes

Address code style violation

3dc1530

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 Mar 4, 2019

rshriram approved these changes Mar 4, 2019

View reviewed changes

Dan Richelson added 2 commits March 4, 2019 15:24

Merge branch 'master' into configurableMeshCerts

d893dc8

Revert temporary api changes. Set cert paths in envoy node metadata a…

55b93ab

…nd use them when setting up listeners

golangcibot reviewed Mar 4, 2019

View reviewed changes

Dan Richelson added 2 commits March 4, 2019 16:27

Use envoy node metadata cert paths (if available) when constructing c…

548ad80

…lusters

Rename constants to make golint happy

e208d82

drichelson mentioned this pull request Mar 5, 2019

Add config options for mesh server certs and watched certs in Pilot-agent istio/api#828

Closed

drichelson commented Mar 5, 2019

View reviewed changes

Dan Richelson added 2 commits March 4, 2019 16:42

Fix imports

5aac66d

Ignore ordering in test

593f203

rshriram reviewed Mar 8, 2019

View reviewed changes

pilot/pkg/networking/core/v1alpha3/cluster.go Outdated

tls *networking.TLSSettings,

serviceAccounts []string,

sni string,

metadata map[string]string,

Copy link

Conversation

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

Uh oh!

Uh oh!

istio-testing commented Mar 1, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Mar 4, 2019

Uh oh!

rshriram commented Mar 4, 2019

Uh oh!

drichelson commented Mar 4, 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!

Choose a reason for hiding this comment

Uh oh!

costinm commented Mar 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

drichelson commented Mar 8, 2019

Uh oh!

drichelson commented Mar 1, 2019 •

edited

Loading

drichelson commented Mar 5, 2019 •

edited

Loading