Add a separate mTLS port for pilot and mixer #370
Conversation
googlebot
added
the
cla: yes
mesh/v1alpha1/config.proto
Outdated
|
|
||
| // Address of the discovery service exposing xDS with mTLS (e.g. | ||
| // _istio-pilot:15005_). | ||
| string discovery_mtls_address = 17; |
There was a problem hiding this comment.
Suggestion: create a common message called ServerAddress, which should have two strings: secure, insecure
Then do ServerAddress pilot, ServerAddress mixer_check, ServerAddress mixer_report
(for backward compatibility, mark the current discovery_address as deprecated).
mesh/v1alpha1/config.proto
Outdated
| @@ -41,6 +41,15 @@ enum AuthenticationPolicy { | |||
| INHERIT = 1000; | |||
| } | |||
|
|
|||
| // ServerAddress defines the address of servers like mixer, pilot, etc. | |||
There was a problem hiding this comment.
defines => specifies
Please be more clear what address means. For example, is it an address of a specific server node? Is it a load balanced address like a DNS name? Is it an address of an HTTP server or gRPC server?
If we need to add deadline, retry hint, and other settings, is ServerAddress still the right term? Envoy uses GrpcService to specify equivalent concept.
There was a problem hiding this comment.
Comment is updated. It's only for mixer and pilot and this name is suggested by @rshriram , i think it makes sense.
| string mutual_tls = 1; | ||
|
|
||
| // The address for plain text server, e.g., (_istio-pilot:15005_) | ||
| string plain_text = 2; |
There was a problem hiding this comment.
Are the two address both required or only one is required?
There was a problem hiding this comment.
comment updated, at least one needs to be specified.
mesh/v1alpha1/config.proto
Outdated
| @@ -74,7 +83,7 @@ message ProxyConfig { | |||
| // MUST BE greater than _drain_duration_ parameter. | |||
| google.protobuf.Duration parent_shutdown_duration = 5; | |||
|
|
|||
| // Address of the discovery service exposing xDS (e.g. _istio-pilot:8080_). | |||
| // Deprecated, use server_address instread. | |||
There was a problem hiding this comment.
If you mark a feature is deprecated, please try to give a deadline for this to be removed. Otherwise it would sit here forever.
There was a problem hiding this comment.
That's a good point, i think the idea to put a 'Deprecated' here is to prevent people adding new code to use it. And for clean up, i am not sure the general process of Istio api. (As you can see there are other deprecated fields in this file)
mesh/v1alpha1/config.proto
Outdated
| // | ||
| // NOTE: Omitting mixerCheckServer while specifying mixerReportServer is | ||
| // equivalent to setting disablePolicyChecks to true. | ||
| // Deprecated, use mixer_check instread. |
mesh/v1alpha1/config.proto
Outdated
| // mixerCheckServer and mixerReportServer, it is possible to have one set | ||
| // of mixer servers handle policy check calls, while another set of mixer | ||
| // servers handle telemetry calls. | ||
| // Deprecated, use mixer_report instread. |
* Address comment * Address comment * Fix typo
App Envoy talks with pilot/mixer based on when auth is enabled during the app is brought up. However, if auth is disabled and then enabled at runtime, app envoy will not able to take to pilot. This allows pilot/mixer to expose two ports and fix the bug.