handle hostname type waypoint for istio 1.24#995
Conversation
hzxuzhonghu
left a comment
There was a problem hiding this comment.
It makes me confusing with so many map around waypoint added, can you reduce unnecessary ones.
| AddOrUpdateService(svc *workloadapi.Service) | ||
| DeleteService(resourceName string) | ||
| GetService(resourceName string) *workloadapi.Service | ||
| HandleWaypoint(svc *workloadapi.Service) []*workloadapi.Service |
There was a problem hiding this comment.
not clear what does this method do, please comment
| // NOTE: The following data structure is used to change the waypoint | ||
| // address of type hostname in the service to type ip address. | ||
| waypointToServices map[string]map[string]*workloadapi.Service | ||
| waypointToAddress map[string]*workloadapi.NetworkAddress |
There was a problem hiding this comment.
comment all the new added map ,what is the key
| // otherwise it waits for the arrival of the waypoint. | ||
| func (s *serviceCache) HandleWaypoint(svc *workloadapi.Service) []*workloadapi.Service { | ||
| s.mutex.Lock() | ||
| defer s.mutex.Unlock() |
There was a problem hiding this comment.
moe it close to the critical section
| } | ||
|
|
||
| if svc.GetWaypoint() == nil || svc.GetWaypoint().GetAddress() != nil { | ||
| return res |
There was a problem hiding this comment.
Seems we donot need to return as the svc has been processed originally
There was a problem hiding this comment.
We don't need to process waypoint with IP address type and res is empty in this context.
| return res | ||
| } | ||
|
|
||
| func (s *serviceCache) updateWaypoint(svc *workloadapi.Service, addr *workloadapi.NetworkAddress) { |
There was a problem hiding this comment.
| func (s *serviceCache) updateWaypoint(svc *workloadapi.Service, addr *workloadapi.NetworkAddress) { | |
| func updateWaypoint(svc *workloadapi.Service, addr *workloadapi.NetworkAddress) { |
| AddOrUpdateService(svc *workloadapi.Service) | ||
| DeleteService(resourceName string) | ||
| GetService(resourceName string) *workloadapi.Service | ||
| HandleWaypoint(svc *workloadapi.Service) []*workloadapi.Service |
There was a problem hiding this comment.
This can be called within AddOrUpdateService
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
| import ( | ||
| "sync" | ||
|
|
||
| "istio.io/istio/pkg/log" |
There was a problem hiding this comment.
Why not use kmesh.net/kmesh/pkg/logger?
There was a problem hiding this comment.
Good catch, IDE automatically completes it.
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Codecov ReportAttention: Patch coverage is
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@hzxuzhonghu Updated, CI fail because of #1004 |
| log.Errorf("storeServiceData failed, err:%s", err) | ||
| return err | ||
| services := []*workloadapi.Service{service} | ||
| services = append(services, servicesToRefresh...) |
There was a problem hiding this comment.
pre allocate memory make ( ,o, len(servicesToRefresh)+1)
| svc3 := createFakeService("svc3", "10.240.10.4", "default/waypoint.default.svc.cluster.local", createLoadBalancing(workloadapi.LoadBalancing_UNSPECIFIED_MODE, make([]workloadapi.LoadBalancing_Scope, 0))) | ||
| assert.NoError(t, p.handleService(svc3)) | ||
|
|
||
| updateWaypointOfService(svc3, waypointIP) |
There was a problem hiding this comment.
don't you feel the test cases is not friendly to add, it has tight dependency with previous cases?
There was a problem hiding this comment.
For other cases that are not related to testing waypoint hostname resolution, we could create a a fake service with IP address type, just like before, then we don't have to call updateWaypointOfService everytime.
|
/retest |
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu 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 |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Starting from istio 1.24, waypoint supports the address type of hostname and Kmesh need to support it.
Which issue(s) this PR fixes:
Fixes part of #984
Special notes for your reviewer:
Does this PR introduce a user-facing change?: