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

Support multiple network interfaces(#9441) #9688

Merged
merged 3 commits into from Dec 14, 2018

Conversation

@zhaohuabing
Copy link
Contributor

zhaohuabing commented Nov 1, 2018

Allows proxy to send multiple IP addresses to Pilot, Pilot uses all the
IP addresses to tell which services are located with the proxy and build
the corresponding inbound listeners for that proxy.

Fixes #9441

Signed-off-by: Huabing Zhao zhaohuabing@gmail.com

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #9688 into release-1.1 will increase coverage by 8%.
The diff coverage is 66%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-1.1   #9688      +/-   ##
==============================================
+ Coverage           63%     70%      +8%     
==============================================
  Files              565     436     -129     
  Lines            52255   40566   -11689     
==============================================
- Hits             32519   28321    -4198     
+ Misses           17872   10893    -6979     
+ Partials          1864    1352     -512
Impacted Files Coverage Δ
security/pkg/nodeagent/sds/sdsservice.go 59% <ø> (ø) ⬆️
pilot/pkg/proxy/agent.go 100% <ø> (ø) ⬆️
pilot/pkg/model/authentication.go 86% <ø> (ø) ⬇️
pilot/pkg/networking/plugin/health/health.go 60% <0%> (+6%) ⬆️
pilot/pkg/proxy/net.go 0% <0%> (ø) ⬆️
pilot/pkg/serviceregistry/memory/discovery.go 60% <0%> (-26%) ⬇️
pilot/pkg/proxy/envoy/proxy.go 33% <0%> (+2%) ⬆️
pilot/pkg/proxy/envoy/v2/mem.go 63% <100%> (+3%) ⬆️
pilot/pkg/networking/core/v1alpha3/cluster.go 77% <100%> (ø) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 77% <100%> (+2%) ⬆️
... and 260 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1ae42f...70615f8. Read the comment docs.

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Nov 1, 2018

/assign @nmittler

@fleeto

This comment has been minimized.

Copy link
Member

fleeto commented Nov 1, 2018

/ok-to-test

@ozevren ozevren requested review from costinm and removed request for ozevren Nov 1, 2018

@rshriram
Copy link
Member

rshriram left a comment

/hold
Does this mean the IP encoded in teh sidecar ID is ignored? How does that IP interact with the rest?
Can a kubernetes pod have multiple IPs? If not, what is the use case for this?

Lets please not start creating ad-hoc metadata variables that control pilot behavior. If someone creates yet another ISTIO_META_NODE_IPs, there is no way to spot the collision or the ensuing confusion.

I would like to see some use case for this first.

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Nov 2, 2018

@rshriram Thanks for your comments.

Actually, this is an issue I'm running into when integrating Istio with our current PaaS platform and is a show stopper for us right now.

The use case is described here: #9441

@rshriram

This comment has been minimized.

Copy link
Member

rshriram commented Nov 2, 2018

Fair enough. In that case, please dont add yet another array of IPs.. Convert the existing one into an array..

Show resolved Hide resolved pilot/pkg/model/context.go Outdated
@rshriram
Copy link
Member

rshriram left a comment

see comments

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Nov 6, 2018

@zhaohuabing: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-unit-tests.sh 0d496a2 link /test istio-unit-tests
prow/istio-integ-k8s-tests.sh 0d496a2 link /test istio-integ-k8s-tests
prow/istio-pilot-e2e-envoyv2-v1alpha3-mcp.sh 0d496a2 link /test istio-pilot-e2e-envoyv2-v1alpha3-mcp
prow/e2e-mixer-no_auth.sh 0d496a2 link /test e2e-mixer-no_auth
prow/e2e-mixer-no_auth-mcp.sh 0d496a2 link /test e2e-mixer-no_auth-mcp
prow/e2e-dashboard.sh 0d496a2 link /test e2e-dashboard
prow/e2e-bookInfoTests-v1alpha3.sh 0d496a2 link /test e2e-bookInfoTests-envoyv2-v1alpha3
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh 0d496a2 link /test istio-pilot-e2e-envoyv2-v1alpha3
prow/istio-pilot-multicluster-e2e.sh 0d496a2 link /test istio-pilot-multicluster-e2e
prow/e2e-simpleTests.sh 0d496a2 link /test e2e-simpleTests
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.

@rshriram These test failures seem not relevant to my PR, should I look into them?

Show resolved Hide resolved pilot/pkg/model/context.go Outdated
Show resolved Hide resolved pilot/cmd/pilot-agent/main.go Outdated
Show resolved Hide resolved pilot/cmd/pilot-agent/main.go Outdated
@hzxuzhonghu

This comment has been minimized.

Copy link
Member

hzxuzhonghu commented Nov 7, 2018

BTW, is there any e2e cases cover this pr?

@rshriram
Copy link
Member

rshriram left a comment

@hzxuzhonghu any other comments?

@hzxuzhonghu

This comment has been minimized.

Copy link
Member

hzxuzhonghu commented Dec 10, 2018

That's it.

@zhaohuabing zhaohuabing changed the base branch from master to release-1.1 Dec 10, 2018

@zhaohuabing zhaohuabing force-pushed the DexMesh:fix-multi-interfaces branch from 831cc33 to d24f5d8 Dec 10, 2018

@rshriram

This comment has been minimized.

Copy link
Member

rshriram commented Dec 10, 2018

/lgtm

@istio-testing istio-testing added the lgtm label Dec 10, 2018

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 10, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu, rshriram, zhaohuabing

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing istio-testing added needs-rebase and removed lgtm labels Dec 10, 2018

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 11, 2018

New changes are detected. LGTM label has been removed.

Support multiple network interfaces(#9441)
Allows proxy to send multiple IP addresses to Pilot, Pilot use all the
IP addresses to tell which services are located with the proxy and build
the inbound listeners for that proxy.

Fixes #9441

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Fix unit test

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Convert IPAddress into an array

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Proxy have to carry valid IP addressess

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Fail back to ip from node id if ISTIO_META_INSTANCE_IPS set wrong format
Add unit test for multiple IP Addresses

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

@zhaohuabing zhaohuabing force-pushed the DexMesh:fix-multi-interfaces branch from 16036a5 to 19c1dd5 Dec 11, 2018

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Dec 11, 2018

@zhaohuabing: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/build-tests.sh 5a9b5f2 link /test build-tests

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.

Fix test
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 13, 2018

Hi guys, anything else?

Show resolved Hide resolved pilot/cmd/pilot-agent/main.go Outdated
Show resolved Hide resolved pilot/pkg/model/context.go Outdated
func ParseServiceNode(s string) (*Proxy, error) {
// ParseServiceNodeWithMetadata parse the Envoy Node from the string generated by ServiceNode
// fuction and the metadata.
func ParseServiceNodeWithMetadata(s string, metadata map[string]string) (*Proxy, error) {

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 13, 2018

Member

Would not change this name, but pass in a *core.Node param.

Maybe a following up .

This comment has been minimized.

@zhaohuabing

zhaohuabing Dec 13, 2018

Author Contributor

OK, I'll leave it as it right now.

Show resolved Hide resolved pilot/pkg/model/context.go Outdated
@hzxuzhonghu

This comment has been minimized.

Copy link
Member

hzxuzhonghu commented Dec 13, 2018

@zhaohuabing @rshriram I also have a doubt: can this pr create multi-outbound-clusters for an instances with multi-ipaddresses?

Fix comments
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 13, 2018

@zhaohuabing @rshriram I also have a doubt: can this pr create multi-outbound-clusters for an instances with multi-ipaddresses?

No, it doesn't impact the outbound clusters.

@hzxuzhonghu

This comment has been minimized.

Copy link
Member

hzxuzhonghu commented Dec 13, 2018

Then can I simply conclude: this pr

  1. allows services outside of the servicemesh accessing services with multi-addresses.

  2. Has no impact on inner mesh communication。

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 13, 2018

This PR is not about service access outside of mesh, it helps pilot build correct inbound listeners when there are multiple IP addresses for a single node. Please refer to this issue for details: #9441

@hzxuzhonghu

This comment has been minimized.

Copy link
Member

hzxuzhonghu commented Dec 13, 2018

I noticed the inbound clusters. And no outbound clusters, so i say it does not influence inner mesh communicarion.

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 14, 2018

I noticed the inbound clusters. And no outbound clusters, so i say it does not influence inner mesh communicarion.

Yes, it does not influence outbound clusters.

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 14, 2018

/retest

@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Dec 14, 2018

Have any idea why ci/circleci: test failed? It seems not relevant to this PR.

FAIL istio.io/istio/mixer/cmd/mixs/cmd [build failed]

@rshriram rshriram merged commit 52152ae into istio:release-1.1 Dec 14, 2018

31 of 32 checks passed

tide Not mergeable. Needs lgtm label.
Details
GolangCI No issues found!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: codecov Your tests passed on CircleCI!
Details
ci/circleci: e2e-dashboard Your tests passed on CircleCI!
Details
ci/circleci: e2e-galley Your tests passed on CircleCI!
Details
ci/circleci: e2e-mixer-noauth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-auth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-cloudfoundry-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-noauth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-simple Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: racetest Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test-integration-local Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
prow/e2e-bookInfoTests-v1alpha3.sh Job succeeded.
Details
prow/e2e-bookInfoTests.sh Skipped
prow/e2e-dashboard.sh Job succeeded.
Details
prow/e2e-mixer-no_auth.sh Job succeeded.
Details
prow/e2e-simpleTests-cni.sh Skipped
prow/e2e-simpleTests-minProfile.sh Job succeeded.
Details
prow/e2e-simpleTests.sh Job succeeded.
Details
prow/istio-integ-k8s-tests.sh Skipped
prow/istio-integ-local-tests.sh Job succeeded.
Details
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh Job succeeded.
Details
prow/istio-pilot-e2e.sh Skipped
prow/istio-pilot-multicluster-e2e.sh Job succeeded.
Details
prow/istio-presubmit.sh Job succeeded.
Details
prow/istio-unit-tests.sh Job succeeded.
Details
prow/release-test.sh Job succeeded.
Details
@zhaohuabing

This comment has been minimized.

Copy link
Contributor Author

zhaohuabing commented Jan 18, 2019

@rshriram Will this PR be merged into master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment