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

Assign the PR to them by writing /assign @geeknoid 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

@rshriram FYI

With closer looking at the code, I realized we have to modify cluster.go instead of authn plugin. Because that's the place we build both default and subset clusters config. whereas plugin is only involved with default cluster settings.

Plugin is invoked for all clusters

The reason for requesting changes is because this can be done cleanly in the plugin by making sure it’s called for all clusters and encapsulating the service account stuff in the auth plugin.

@rshriram you're right on this is called for all clusters including subset. https://github.com/istio/istio/blob/master/pilot/pkg/networking/core/v1alpha3/cluster.go#L114

So the determining config field comes from DR (TLSSettings), the question is who should consume this settings. I agree with @diemtvu that the consumer of this field should be in clusters.go, so the authn only focused on the server authn requirement, and let networking side deal with client settings. This is the intended seperation of client/server we want to have. Will be nice to make implementation aligned with user facing config.

If the concern is more of "why networking now needs to know where istio provided key cert live, etc?", we can extract those key certs paths logic and put it somewhere else.

@costinm wdyt?

Regarding why in master, we plan to make it work E2E first and only after ensuring it works, then merge with release branch.

Also does this mean we will no longer have a global auth setting and setup client side auth by “default”?

This is correct... We need to provide something global else after these work...

My point is retain the isDestinationEnabledForMtls. In that function, check global flag or local dest rule that comes as part of the plugin call. Add an extra parameter to plugin calls in cluster.go to supply the destination rule as well.

This way, there is very minimal code change

Codecov Report

Merging #5418 into master will increase coverage by 1%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master   #5418     +/-   ##
========================================
+ Coverage      74%     74%     +1%     
========================================
  Files         323     317      -6     
  Lines       27394   26289   -1105     
========================================
- Hits        20123   19420    -703     
+ Misses       6480    6140    -340     
+ Partials      791     729     -62

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 35e2b90...ca83544. Read the comment docs.

1. Global settings for mTLS is now preserved. I change how we buildDefaultTrafficPolicy.
2. I'm not sure if we need to handle MtlsExcludedServices specially. In the context of v2 api, maybe customers should write DestinationRule for those services, like apiserver? @costinm

Also this has nothing to do with tls sniffing. This pr is useful by itself in existing 0.8, where auth overrides tls settings in dest rule. So I still believe that 0.8 is the right place for the ISTIO_mutual stuff. The tls snuffing changes in inbound listener is a different PR

In auth plugin
OnOutboundCluster(cluster, destination rule, service, port)
If isIstioMtlsEnabled()
Set istio mTLS

isIstioMtlsEnabled(mesh, destrule)
if mesh.mtls and destrulr.tls == nil
or destrule.tlsmode == istio_mutual
return true
Return false

Technically, the approach Shriram suggested could work. However, per discussion with Louis, Costin (meeting note), we want to cleanly separate authn policy to affect server side only, and destination rule to affect client side only. As a result, I think it's make more sense to remove OutboundCluster authn plugin code, to the applyTrafficPolicy module. + @louiscryan @costinm for more comments.

We need to infer a 'default' destination rules from the global mesh config flag for now, for backward compatible. However, in the long run, I think we can remove that flag and have customers explicitly submit global authN policy and destination rule for that.

@rshriram We also think this is a useful change for 0.8, regardless of tls multiplexing. I'll work on testing this against 0.8 branch and if we can get test passing before release cut, will change this into 0.8.

There is enough breaking change in 0.8 already. That said this also means that people have to use v1alpha3 in order to do auth. But v1alpha3 is optional in 0.8 and becomes default only in 0.9. So you need a backward compatible way of enabling fleet wide auth in 0.8 (via global config option)

with v1/v1apha1, authN policy will set both client and server side, so it still work without destination rules. Do you think that is enough?

I changed to fill key/cert path in advance, instead of plumbing service account everywhere. I still want to keep ISTIO_MUTUAL separate from MUTUAL because in future when SDS is available we can't use key/certs paths to configure envoy.

Also removed the change on buildDefaultCluster, which is used for user app container as cluster as well. And there, we shouldn't add any TLSSettings.

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 the commit author(s) and merge this pull request when appropriate.

CLAs look good, thanks!

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

@incfly: PR needs rebase.

Is (ab)using TLSMode for this is really a good idea? Shouldn't this a part of the TrafficPolicy? Perhaps bool TrafficPolicy.InMesh?