Force usage of envoyv2 ports in bootstrap.go by consuming config option for discoveryAddress#6452
Force usage of envoyv2 ports in bootstrap.go by consuming config option for discoveryAddress#6452sdake wants to merge 2 commits intoistio:masterfrom
Conversation
The Helm chart has v1 proxy ports which is incorrect.
| # | ||
| # Address where istio Pilot service is running | ||
| discoveryAddress: istio-pilot.{{ .Release.Namespace }}:15005 | ||
| discoveryAddress: istio-pilot.{{ .Release.Namespace }}:15011 |
There was a problem hiding this comment.
was tested with the e2e tests. I do not know if proxyv2 will not work with port 15005 or 15007. This problem was found in a different review.
There was a problem hiding this comment.
Obtain the values from the Envoy v2 transition.yaml. Also I think Costin has some ports fixed in the bootstrap/server.go
There was a problem hiding this comment.
@rshriram after un-hardcoding the port, the gate implodes. Any insight?
There was a problem hiding this comment.
@tiswanso afte the sidecar was injected, the /etc/istio/proxy/envoy-rev0.json is already "socket_address": {"address": "istio-pilot.istio-system", "port_value": 15010}, but why the injected container generated by istioctl kube-inject still using 15007?
spec:
containers:
- image: istio/examples-bookinfo-details-v1:1.5.0
imagePullPolicy: IfNotPresent
name: details
ports:
- containerPort: 9080
resources: {}
- args:
- proxy
- sidecar
- --configPath
- /etc/istio/proxy
- --binaryPath
- /usr/local/bin/envoy
- --serviceCluster
- details
- --drainDuration
- 45s
- --parentShutdownDuration
- 1m0s
- --discoveryAddress
- istio-pilot.istio-system:15007 <<<<
- --discoveryRefreshDelay
There was a problem hiding this comment.
Gateways don’t read the global sidecar injector config. So if you want to make pilot port parameterizable, add option to global config and make pilot and pilot agent read it during startup
There was a problem hiding this comment.
There was a problem hiding this comment.
@tiswanso so seems the parameter discoveryAddress in injected in sidecar container has no use? Then why do we need to set this parameter?
Codecov Report
@@ Coverage Diff @@
## master #6452 +/- ##
=======================================
- Coverage 68% 68% -<1%
=======================================
Files 357 352 -5
Lines 31094 30862 -232
=======================================
- Hits 21018 20856 -162
+ Misses 9246 9160 -86
- Partials 830 846 +16
Continue to review full report at Codecov.
|
|
/assign @nmittler |
|
@sdake: 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. |
costinm
left a comment
There was a problem hiding this comment.
/lgtm
It was hardcoded because I didn't want to add another option or env variable - we don't have tests or really support changing the ports, and with the v1 going away I was planning to do something like this PR.
BTW - not sure how 'remote' is configured now, but I suspect the entire address should be customizable, so we can pass "https://istio-gateway.mydomain.com:15011" as discovery address in remote clusters (and even in primary cluster)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm, sdake 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 |
|
@rshriram indicated this would be taken care of by the v1 removal. |
The Helm chart has v1 proxy ports which is incorrect.