Logic for generating LDS v2 response#4049
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
|
Is this for v1alpha1 or v1alpha3 or both? |
|
It depends on both alpha1 and alpha3 APIs so I guess that means it must be for alpha3 only. |
|
/retest |
7bea6b8 to
7b8f6e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #4049 +/- ##
=======================================
- Coverage 75% 74% -1%
=======================================
Files 305 313 +8
Lines 27845 28126 +281
=======================================
- Hits 20852 20532 -320
- Misses 5664 6337 +673
+ Partials 1329 1257 -72
Continue to review full report at Codecov.
|
|
The goal is alpha1 and alpha3 - but we can relax it if needed. |
costinm
left a comment
There was a problem hiding this comment.
Looks like most of this is a copy of v1, with conversions to the new protos.
Thanks for breaking this into smaller PRs.
pilot/pkg/proxy/envoy/v2/config.go
Outdated
There was a problem hiding this comment.
Does it have to be exported ? Same for other methods/etc.
pilot/pkg/proxy/envoy/v2/util.go
Outdated
There was a problem hiding this comment.
Where is it used ? Add a TODO to use a map, iterating is not good.
There was a problem hiding this comment.
It's used in a few places when building protos. I assume you mean to pass the map around (building it inside we still have to iterate). So that would complicate the code a little since we'd have to sync the map with the slice whenever that changes to avoid rebuilding every time. I've put a comment through.
tests/e2e/framework/mason.go
Outdated
There was a problem hiding this comment.
For this and other similar - did you use an automated tool ? Better to keep cosmetic changes separate.
There was a problem hiding this comment.
I used goimports. It was because make lint was complaining about it. It is better not to change and commit with the lint warnings?
There was a problem hiding this comment.
In a separate iteration, I suggest a bit of cleanup - no need to return clusters for example.
There was a problem hiding this comment.
Yes, absolutely. This is just a minimum viable product, there are many places it can be trimmed and improved. I wanted to keep the changes minimal to focus on correctness initially.
|
@ostromart: 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. |
|
One suggestion for a follow-up PR: please move all ingress related logic to an lds_ingress.go file. Decoupling ingress will both simplify testing and unblock SNI testing, and simplify the in-mesh logic |
This is a heavily refactored version of ostromart#8, due to lots of changes in the protos since the start of that PR.
Many of the functions are based on the v1 versions. For easy review, patch the PR and locally diff the v2 version against the v1 version e.g. meld v1/fault.go v2/fault.go. This will reveal the changes that were needed to build the v2 versions.