Skip to content
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

add istio patch to fix xds push #52

Merged
merged 1 commit into from
Nov 11, 2022
Merged

Conversation

johnlanni
Copy link
Collaborator

Ⅰ. Describe what this PR did

The previous hack implementation caused LDS to be pushed empty

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-commenter
Copy link

Codecov Report

Merging #52 (e217cd8) into main (268c733) will decrease coverage by 4.82%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
- Coverage   29.63%   24.80%   -4.83%     
==========================================
  Files          27       24       -3     
  Lines        4330     4076     -254     
==========================================
- Hits         1283     1011     -272     
- Misses       2986     3008      +22     
+ Partials       61       57       -4     
Impacted Files Coverage Δ
pkg/ingress/kube/annotations/canary.go 18.42% <0.00%> (-20.43%) ⬇️
pkg/ingress/kube/annotations/ip_access_control.go 87.50% <0.00%> (-12.50%) ⬇️
pkg/ingress/kube/annotations/loadbalance.go 89.18% <0.00%> (-10.82%) ⬇️
pkg/ingress/kube/annotations/parser.go 58.59% <0.00%> (-7.65%) ⬇️
pkg/ingress/kube/annotations/timeout.go
pkg/ingress/kube/annotations/local_rate_limit.go
pkg/ingress/kube/annotations/header_control.go
pkg/ingress/kube/ingressv1/controller.go 3.71% <0.00%> (+<0.01%) ⬆️
pkg/ingress/kube/ingress/controller.go 3.72% <0.00%> (+<0.01%) ⬆️
pkg/ingress/kube/annotations/downstreamtls.go 88.23% <0.00%> (+3.30%) ⬆️
... and 1 more

Copy link
Collaborator

@SpecialYang SpecialYang left a comment

Choose a reason for hiding this comment

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

Don't understand why use the mcp generator directly. Since the resources were sent to mcp store, why not use the xds generator?

@johnlanni
Copy link
Collaborator Author

Don't understand why use the mcp generator directly. Since the resources were sent to mcp store, why not use the xds generator?

Although these two Resource types are all convert to []Any in the DiscoveryResponse, xds generator generates the Resource which is defined by envoy's api in discovery/v3, and mcp generator generates the Resource which is defined by istio's api in mcp/v1alpha1.

@SpecialYang
Copy link
Collaborator

What's protocol between envoy and istio? xds or mcp-over-xds?

@johnlanni
Copy link
Collaborator Author

johnlanni commented Nov 11, 2022

What's protocol between envoy and istio? xds or mcp-over-xds?

I understand your doubts, in fact, istio will not call pushMcpXds here, only higress will, here is higress reuse the logic related to xds

Copy link
Collaborator

@SpecialYang SpecialYang left a comment

Choose a reason for hiding this comment

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

We should reconsider this way in the future. It's better to build our xds server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants