Skip to content

separate check and report clusters #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 8, 2018
Merged

separate check and report clusters #362

merged 5 commits into from
Feb 8, 2018

Conversation

rshriram
Copy link
Member

@rshriram rshriram commented Feb 7, 2018

Pursuant to PR #358 ..
TODO: Once this is implemented in Pilot such that policy_check_cluster and telemetry_cluster are always specified, we need to

  1. Change proxy such that a missing check_cluster implies disable policy checks, a missing telemetry_cluster implies disable telemetry, but enable policy.
  2. After changing proxy, remove the disable_policy_checks option from global config, update docs, and remove dead code from pilot.

@rshriram rshriram requested a review from ZackButcher as a code owner February 7, 2018 15:15
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 7, 2018
@@ -117,15 +117,25 @@ message ProxyConfig {
// MeshConfig defines mesh-wide variables shared by all Envoy instances in the
// Istio service mesh.
message MeshConfig {
// Address of the egress Envoy service (e.g. _istio-egress:80_).
string egress_proxy_address = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is egress_proxy_address being removed as part of the mixer_address separation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah shux.. It was an unused config item.. We got rid of egress proxy a long time ago. Thought I would remove it as well.

// reported to the mixer for HTTP requests and TCP connections. Default
// Address of the server that will be used by the proxies for policy
// check calls (e.g. _istio-mixer:15004_). By using different names for
// policyCheckServer and telemetryServer, it is possible to have one set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use mixer_check_server and mixer_report_server for consistency with the mixer API service?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point. What I am trying to do here is to keep the config at concept level (policies, telemetry, auth, etc.). There are other parts of the config that are essentially pointing to implementation specific things, which to a layman is going to be hard to understand. If I am starting off with Istio, I look at global config and turn off things I don't want/enable things I want. Make sense?

(I am waiting to get rid of those RDSrefresh delays, etc. but haven't found a good way to do it yet).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat. We seem to be conflating mesh administrator configuration and operator policy config into the same protobuf which makes this difficult. Configuring whether a feature (e.g. auth, telemetry) is enabled globally (or per namespace / service) is a different concept than how the services that enable those features are accessed by the proxies.

Shriram Rajagopalan added 2 commits February 7, 2018 14:47
Copy link
Contributor

@ZackButcher ZackButcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@rshriram
Copy link
Member Author

rshriram commented Feb 7, 2018

Lets not merge this until all tests pass in istio/istio for my PR. Dont want to upset stuff..

@rshriram rshriram merged commit b5d35aa into master Feb 8, 2018
@rshriram rshriram deleted the mixer_cluster_mesh branch February 13, 2018 19:41
ayj pushed a commit to ayj/api that referenced this pull request Feb 14, 2018
* separate check and report clusters

* fix

* nits

* nit

* backward compat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants