Support gateway service name in meshNetworks #13392
Conversation
| // when loading the config | ||
| if gwIP := net.ParseIP(gw.GetAddress()); gwIP != nil { | ||
| gwAddrs = []string{gw.GetAddress()} | ||
| } else { |
There was a problem hiding this comment.
elseif: can replace 'else {if cond {}}' with 'else if cond {}' (from gocritic)
| @@ -15,6 +15,7 @@ | |||
| package v2 | |||
|
|
|||
| import ( | |||
| "istio.io/istio/pilot/pkg/networking/core/v1alpha3/fakes" | |||
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
|
/assign @rshriram |
hzxuzhonghu
force-pushed
the
gw-svc-name
branch
from
9145b6d
to
b97e8c9
Compare
|
@rshriram any other comments? |
|
/cc @sdake |
| if len(ingress.IP) > 0 { | ||
| lbAddrs[i] = ingress.IP | ||
| } else if len(ingress.Hostname) > 0 { | ||
| lbAddrs[i] = ingress.Hostname |
There was a problem hiding this comment.
if you assign this mixed mode IP/hostname, it will break all EDS. An EDS cluster cannot have both ip and hostname
pilot/pkg/model/service.go
Outdated
| // address(es) to access the service from outside the cluster. | ||
| // Used by the aggregator to aggregate the Attributes.ExternalAddresses | ||
| // for clusters where the service resides | ||
| ExternalAddresses map[string][]string |
There was a problem hiding this comment.
It doesn't matter what we cal this. This data structure here is an aggregation point. Its not the cleanest way to create this. Same applies to this ClusterVIPs that was created. Applicable to k8s, but it thoroughly confuses anybody else looking at the code to integrate it into their platform.
Move this into the service attribute, preferably under a map with key as k8s, within which you can do whatever you want, so that this logic does not leak into listeners/clusters, etc.
Also keep in mind that these new fields do not have a representation in the Service Entry. So what happens when Galley starts streaming service entries for all clusters? this will break badly. I suggest devising a more comprehensive solution.
There was a problem hiding this comment.
when Galley starts streaming service entries for all clusters?
Is ServiceEntry the last choice? I donot think it is easy to transform platform specific service model to ServiceEntry. If it is me, I would stream istio service model and thus reuse the convert logic.
There was a problem hiding this comment.
@rshriram Is this still a blocker?
There was a problem hiding this comment.
Unfortunately, service entry is the format we are using to stream information from Galley to Pilot. So, you would have to figure out a way to make that work with service entry. May be through annotations, etc. and then translate into the service model. Hence my suggestion to stick this as attribute in service attributes
There was a problem hiding this comment.
ok, will take a look.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu, rshriram The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test e2e-mixer-no_auth-master |
|
/retest |
|
/test e2e-mixer-no_auth-master |
|
@hzxuzhonghu: The following tests failed, say
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. I understand the commands that are listed here. |
* Sync changes to old installer Cherrypicks istio#13392, istio#13666, istio#13982, istio#14077, istio#14059 * Cherrypick istio#14301 * Cherrypick istio#13638 * cherrypick istio#14606 * istio#14171 * istio#14438 * istio#14674 * istio#14974 * 14904 * istio#14796 * istio#15346 * istio#15345 * istio#15383 * istio#14815 * istio#15690 * istio#15681 * istio#15014 * istio#15503 * istio#16084 * istio#16146 * istio#16147 * Fix tests
Continue working based on @ymesika 's work #12728