Add more checking for istio-remote.#6393
Conversation
There was a problem hiding this comment.
seems like a little too much overloading. I believe @costinm had thought using the actual DNS name would result in a workable solution with virtual services + coredns in addition.
One thing that might work a little better is:
global.pilotEnabled: true
global.telemetryEnabled: true
global.tracingEnabled: false
and not overloading the variables.
There was a problem hiding this comment.
I think a point is that having default values don't make sense anyway. The endpoint manifest won't validate with those defaults--the API demands an IP for the field.
There was a problem hiding this comment.
@sdake adding a new parameter to control the endpoint is enabled or not seems to be complex, as I need to update two places if I want to enable pilotEndpoint, including both global.pilotEnabled and pilotEndpoint, but with this PR, we only need to update pilotEndpoint.
There was a problem hiding this comment.
Is this a problem not defining a pilotEndpoint value? I am not a helm expert but it looks fishy since this is only defined in istio-remote
There was a problem hiding this comment.
Ignore my comment got a bit of tutorial on how these values are used to build the charts
|
LGTM -- without a valid endpoint value for a service, the corresponding sections in the manifest don't make sense on the remote. |
tiswanso
left a comment
There was a problem hiding this comment.
LGTM -- the *Endpoint settings MUST be IPs for the template and defaulting the value doesn't make sense. The lack of a setting for a specific service equates to not needing the istio-remote to contain a corresponding representation of selectorless service and endpoint manifests.
c332257 to
450852d
Compare
Codecov Report
@@ Coverage Diff @@
## master #6393 +/- ##
======================================
+ Coverage 71% 71% +1%
======================================
Files 354 354
Lines 30666 30743 +77
======================================
+ Hits 21503 21602 +99
+ Misses 8314 8289 -25
- Partials 849 852 +3
Continue to review full report at Codecov.
|
|
@tiswanso the endpoint does not need to be an IP, as long as it is resolvable via Kubernetes. resolution can happen via coredns. I think in the past @costinm had mentioned that coredns can be used to serve the purposes of providing DNS resolution with the kubernetai plugin. @costinm can you weigh in - IIRC you had the strong desire to add these endpoint definitiions. |
|
Added do-not-merge tag until Costin can weigh in. |
|
@sdake I played with coredns and integration with kubernetes, it does work but a couple of points to consider:
|
|
@sdake -- The error I see is not a DNS resolution error. It's an immediate error on validating the endpoint kind--the error indicates that an IP is required. Anyway, regardless... the whole point of the selectorless services and endpoints is to get a DNS entry. If a setup has multicluster coredns is setup, this will already be achieved and none of these selectorless services is required. |
|
@sbezverk coredns is standard in kubernetes 1.11+. |
|
@rshriram is also using CoreDNS for zero VPN multi-cloud solution, mainly using the CoreDNS to resolve service |
|
@sdake -- Could we not hold this up for analysis of a special case? It's a blocker for e2e multicluster pilot tests. |
|
Why are we holding this up on DNS approach? The current charts are broken because the multicluster e2e tests will fail due to the way the helm charts are generated for the tests. Let's let this merge if everyone agrees that the code is sound. We can revamp the recommended install as a separate PR as the changes are more encompassing. |
|
We may not need this after #6366 got merged. |
|
@gyliu513 IMO, let's not wait on #6366. This is an easy change for now and not having this is a blocker for using the instructions in https://preliminary.istio.io/docs/setup/kubernetes/multicluster-install/. |
|
/retest |
Add more checking for istio-remote chart to see if the endpoints and services are enabled for istio remote.
450852d to
aaf7df6
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gyliu513, morvencao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@gyliu513: The following tests failed, say
DetailsInstructions 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. |
|
@gyliu513: PR needs rebase. DetailsInstructions 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. |
|
#6366 is merged |
Add more checking for istio-remote chart to see if the endpoints
and services are enabled for istio remote.
Fixed #6321
/cc @sdake @baodongli @tiswanso @john-a-joyce