ignore STRICT_DNS/STATIC Cluster doesnot have endpoints by ydh926 · Pull Request #20981 · istio/istio

ydh926 · 2020-02-10T03:00:05Z

No description provided.

Update

update

Update

istio-testing · 2020-02-10T03:00:29Z

Hi @ydh926. 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.

googlebot · 2020-02-10T03:00:40Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

ydh926 · 2020-02-10T03:06:11Z

STRICT_DNS/STATIC Cluster does not have endpoint will block envoy.
envoyproxy/envoy#9963

Impact on pilot：
#20933

ydh926 · 2020-02-10T04:00:53Z

@googlebot I fixed it

googlebot · 2020-02-10T04:01:05Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

hzxuzhonghu · 2020-02-10T04:19:53Z

/ok-to-test

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

hzxuzhonghu · 2020-02-10T04:27:03Z

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

Can you explain how can this happen?

For example:

configure a DNS type ServiceEntry

apiVersion: networking.istio.io/v1alpha3 kind: ServiceEntry metadata: name: static-20191230 namespace: gateway-system spec: endpoints: - address: 10.108.251.133 ports: http: 80 - address: 10.96.144.80 ports: http: 80 exportTo: - '*' hosts: - com.netease.static-20191230 location: MESH_EXTERNAL ports: - name: http number: 80 protocol: HTTP resolution: DNS

configure a subset, which cannot select any endpoint

apiVersion: networking.istio.io/v1alpha3 kind: DestinationRule metadata: name: static-1230 namespace: gateway-system spec: host: com.netease.static-20191230 subsets: - altStatName: healthCheckTest name: static-20191230-test1 labels: app: none

hzxuzhonghu · 2020-02-10T07:50:26Z

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

no need check outbound

hzxuzhonghu · 2020-02-10T08:06:04Z

@ydh926 Add a test for it in cluster_test.go

ydh926 · 2020-02-10T13:33:19Z

@ydh926 Add a test for it in cluster_test.go
is this ok? @hzxuzhonghu

rshriram · 2020-02-10T21:12:45Z

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

Remove the error message please as this is going to get very noisy. Instead add it as a metric (look around pilot code for how we do metrics for these kinds of things).

ydh926 · 2020-02-11T04:17:39Z

/retest

hzxuzhonghu · 2020-02-11T08:58:23Z

pilot/pkg/model/push_context.go

pilot_dns_cluster_without_endpoints

hzxuzhonghu

One question: we do have a BlackHoleCluster cluster of static type without endpoints, why there is no error?

ydh926 · 2020-02-11T09:55:22Z

One question: we do have a BlackHoleCluster cluster of static type without endpoints, why there is no error?

The field loadassignment of BlackHoleCluster is nil，envoy will give it a default value. But it in STRICT_DNS cluster is a struct with an empty Endpoints array, envoy will try to resolve it. That is a bug in envoy, but I am not sure when they will fix it.

hzxuzhonghu · 2020-02-11T13:17:01Z

pilot/pkg/model/push_context.go

pilot_dns_cluster_without_endpoints, actually we donot sum up ths static cluster.

why this is back

sorry，it is a mistake. fixed already

ramaraochavali · 2020-02-11T16:27:44Z

I think we should exclude STATIC from this check because imagine a case we create a service entry with STATIC resolution type and if its endpoints ever become zero, with this change we do not send that cluster to Envoy and Envoy would delete the cluster - That is not correct because we should just clear the endpoints not the cluster it self. I fixed similar thing to allow empty endpoints to static cluster in service entries #19036

ydh926 · 2020-02-12T02:11:51Z

I think we should exclude STATIC from this check because imagine a case we create a service entry with STATIC resolution type and if its endpoints ever become zero, with this change we do not send that cluster to Envoy and Envoy would delete the cluster - That is not correct because we should just clear the endpoints not the cluster it self. I fixed similar thing to allow empty endpoints to static cluster in service entries #19036

In your description, a ServiceEntry of static type will be converted to a cluster of type eds. If so, these changes will not affect the ServiceEntry of static type. We only deal with static or DNS clusters.

For Service Entries with STATIC resolution, we should allow empty endpoints. Since we internally convert it to EDS type of cluster in Envoy - it will work and it is valid for the external service represented by the Service Entry to have zero endpoints.

Static type clusters seem to only be used internally, so I will remove the description to avoid ambiguity

hzxuzhonghu · 2020-02-12T03:04:48Z

make sense, fix the lint, and i will approve.

ramaraochavali · 2020-02-12T04:59:57Z

Ah.. sorry. Makes sense. Thank you

ydh926 · 2020-02-12T07:31:32Z

fixed @hzxuzhonghu

hzxuzhonghu · 2020-02-12T08:20:47Z

@ydh926 Do you forget to push?

…0981)

lambdai · 2020-04-09T22:45:09Z

cherrypick/release-1.4

lambdai · 2020-04-09T22:48:02Z

cherrypick/release-1.5

istio/istio#22837 (comment) envoyproxy#10728 istio/istio#20981 Signed-off-by: Venil Noronha <veniln@vmware.com>

chaudyg · 2020-05-01T10:31:26Z

@lambdai I don't think the cherrypick in 1.4 worked. Any idea?

ydh926 and others added 4 commits August 15, 2019 09:42

Merge pull request #1 from istio/master

e771e05

Update

Merge pull request #2 from istio/master

9bc4b17

Update

Merge pull request #3 from istio/master

44e40c3

update

Merge pull request #4 from istio/master

cdcda39

Update

ydh926 requested a review from a team as a code owner February 10, 2020 03:00

istio-policy-bot added the area/networking label Feb 10, 2020

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test labels Feb 10, 2020

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Feb 10, 2020

googlebot added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Feb 10, 2020

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 Feb 10, 2020

hzxuzhonghu reviewed Feb 10, 2020

View reviewed changes

ydh926 force-pushed the master branch from b38f340 to a667ee7 Compare February 10, 2020 08:00

ydh926 force-pushed the master branch from a667ee7 to a41a24e Compare February 10, 2020 13:22

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 10, 2020

ydh926 force-pushed the master branch 2 times, most recently from 1362d2a to b27b4b1 Compare February 10, 2020 13:28

ydh926 force-pushed the master branch from b27b4b1 to bb1aa7e Compare February 10, 2020 14:21

rshriram reviewed Feb 10, 2020

View reviewed changes

ydh926 force-pushed the master branch 2 times, most recently from 3699941 to a38dae2 Compare February 11, 2020 02:56

hzxuzhonghu reviewed Feb 11, 2020

View reviewed changes

ydh926 force-pushed the master branch 2 times, most recently from d3f70b2 to 6d21ead Compare February 11, 2020 10:27

hzxuzhonghu reviewed Feb 11, 2020

View reviewed changes

ydh926 force-pushed the master branch from 6d21ead to 37c9837 Compare February 12, 2020 02:29

ydh926 force-pushed the master branch from 37c9837 to 32142f0 Compare February 12, 2020 03:19

Fix: ignore STRICT_DNS cluster which does not have endpoints

c8ffb99

ydh926 force-pushed the master branch from 32142f0 to c8ffb99 Compare February 12, 2020 08:28

hzxuzhonghu approved these changes Feb 12, 2020

View reviewed changes

istio-testing merged commit af85f9c into istio:master Feb 12, 2020

sdake pushed a commit to sdake/istio that referenced this pull request Feb 21, 2020

Fix: ignore STRICT_DNS cluster which does not have endpoints (istio#2…

df0f4f5

…0981)

lambdai mentioned this pull request Apr 13, 2020

Removing a locality from localityLbSetting.Distribute breaks the mesh #22837

Closed

venilnoronha added a commit to vmware-allspark/envoy that referenced this pull request Apr 21, 2020

Fix CDS pause

6f5eb8a

istio/istio#22837 (comment) envoyproxy#10728 istio/istio#20981 Signed-off-by: Venil Noronha <veniln@vmware.com>

ydh926 mentioned this pull request Jan 11, 2021

Update members.yaml istio/community#512

Merged

5 tasks

Conversation

ydh926 commented Feb 10, 2020 • edited by istio-policy-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Feb 10, 2020

Uh oh!

googlebot commented Feb 10, 2020

Uh oh!

ydh926 commented Feb 10, 2020

Uh oh!

ydh926 commented Feb 10, 2020

Uh oh!

googlebot commented Feb 10, 2020

Uh oh!

hzxuzhonghu commented Feb 10, 2020

Uh oh!

Uh oh!

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!

hzxuzhonghu commented Feb 10, 2020

Uh oh!

ydh926 commented Feb 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydh926 commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

ydh926 commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ramaraochavali commented Feb 11, 2020

Uh oh!

ydh926 commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzxuzhonghu commented Feb 12, 2020

Uh oh!

ramaraochavali commented Feb 12, 2020

Uh oh!

ydh926 commented Feb 12, 2020

Uh oh!

hzxuzhonghu commented Feb 12, 2020

Uh oh!

lambdai commented Apr 9, 2020

Uh oh!

lambdai commented Apr 9, 2020

Uh oh!

chaudyg commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ydh926 commented Feb 10, 2020 •

edited by istio-policy-bot

Loading

ydh926 commented Feb 11, 2020 •

edited

Loading

ydh926 commented Feb 12, 2020 •

edited

Loading