-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Fix eds cluster loadassignment with invalid endpoint #50690
Fix eds cluster loadassignment with invalid endpoint #50690
Conversation
@@ -335,6 +335,10 @@ func (b *EndpointBuilder) BuildClusterLoadAssignment(endpointIndex *model.Endpoi | |||
if svcPort.Name != ep.ServicePortName { | |||
return false | |||
} | |||
// filter out endpoint that has invalid ip address, mostly domain name. Because this is generated from ServiceEntry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the issue, it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious which commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start from 1.20.0, maybe 1.19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH i am not sure. It was since 1.18+? @zirain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it's fine in 1.18, and broken since 1.20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to confirm the regression so we know this is the correct fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. It would be good identify what commit caused this so that we can know what happened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one LGTM but I would love a high level reproduction test... maybe in sidecar_simulation_test.go?
// There are other two cases that should not be filtered out: | ||
// 1. ep.Address can be empty since https://github.com/istio/istio/pull/45150, in this case we will replace it with gateway ip. | ||
// 2. ep.Address can be uds when EndpointPort = 0 | ||
if ep.Address != "" && ep.EndpointPort != 0 && net.ParseIP(ep.Address) == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we have IsValidIPAddress function already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There is cases covering in pilot/pkg/xds/endpoints/ep_filters_test.go And also both for UDS and empty address, which is actually found via CI failiure |
In response to a cherrypick label: #50690 failed to apply on top of branch "release-1.20":
|
In response to a cherrypick label: new issue created for failed cherrypick: #50717 |
In response to a cherrypick label: #50690 failed to apply on top of branch "release-1.21":
|
In response to a cherrypick label: new issue created for failed cherrypick: #50718 |
In response to a cherrypick label: new pull request created: #50719 |
* fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP
* fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP
* fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP
* fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP
…50721) * move port name and labels check outside of filterIstioEndpoint (#49629) * move port name and labels check outside of filterIstioEndpoint * fix slices filter * fix ut * Fix eds cluster loadassignment with invalid endpoint (#50690) * fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP --------- Co-authored-by: Zhonghu Xu <xuzhonghu@huawei.com>
…50720) * move port name and labels check outside of filterIstioEndpoint (#49629) * move port name and labels check outside of filterIstioEndpoint * fix slices filter * fix ut * Fix eds cluster loadassignment with invalid endpoint (#50690) * fix eds cluster has invalid endpoints * Add release note * Fix specical case ep address empty * add uds addr check * use IsValidIP --------- Co-authored-by: Zhonghu Xu <xuzhonghu@huawei.com>
Please provide a description of this PR:
Fix #50688