Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dual stack intra-mesh integ tests #38892

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Dual stack intra-mesh integ tests #38892

wants to merge 6 commits into from
+464 −0

Conversation

Kmoneal
Copy link
Contributor

@Kmoneal Kmoneal commented May 12, 2022 •
edited by istio-policy-bot

Please provide a description of this PR:
Integration tests to validate dual-stack intra-mesh communication. Mainly focusing on IPv4 to IPv6 and IPv6 to IPv4 traffic. These two tests are skipped for now but should not be in the future.

@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 12, 2022
@istio-policy-bot istio-policy-bot added area/networking area/test and release release-notes-none Indicates a PR that does not require release notes. labels May 12, 2022
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 12, 2022
@Kmoneal Kmoneal marked this pull request as ready for review May 19, 2022 05:27
@Kmoneal Kmoneal requested a review from a team as a code owner May 19, 2022 05:27
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 19, 2022
@Kmoneal
Introduce intra-mesh communication dual-stack test.
e6d46d7 
* Echo pods communicate directly
* IPv4 and IPv6 single stack pods should not be able to communicate
@Kmoneal
Minor fixes
5e60415
@Kmoneal
Skip failing integration tests for now
837aca0
@Kmoneal
Fix linter issues
5b6728c
@Kmoneal Kmoneal force-pushed the dual-stack-intra-mesh-integ-tests branch from 1abd1c2 to 5b6728c Compare May 27, 2022 18:33
@Kmoneal
Copy link
Contributor Author

Kmoneal commented May 31, 2022

Step for #37533

stevenctl
stevenctl reviewed May 31, 2022
View reviewed changes
pkg/test/framework/suite.go
@@ -302,6 +307,39 @@ func (s *suiteImpl) RequireMaxVersion(minorVersion uint) Suite {
return s
}

func (s *suiteImpl) RequireDualStackCluster() Suite {
fn := func(ctx resource.Context) error {
for _, c := range ctx.Clusters().Kube() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: can do this in the Environment setup as we create kube clusters or cache this value on the Cluster struct somehow, just to avoid adding warmup time to each suite, even though it's probably very minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into simplifying it. Thanks for the recommendation!

tests/integration/dualstack/common/apps.go
{Name: "dual-ipv6", Service: "echo-dual-ipv6", IPFamily: "IPv6, IPv4", IPFamilyPolicy: "RequireDualStack"},
}

var EchoPorts = []echo.Port{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we re-use (a portion) of common echo deployments from other suites?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that here. We did have our own version for other tests that we haven't upstreamed yet but for this case it makes sense to delay adding this.

tests/integration/dualstack/common/traffic.go
skip bool
}

func (c TrafficTestCase) Run(ctx framework.TestContext, namespace string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part, I think tests/integration/pilot/common/ can support this. If there is a reason we re-implement it here, can you add a comment explaining why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part it's the same. We do want to verify proxies are not sending http requests at the same time which is why I have a similar function: https://github.com/istio/istio/pull/38892/files#diff-8d1061ab02227bf02916079cac53e632843d46dcd82ce42a24e7ef20eeec6f89R75

howardjohn
howardjohn reviewed May 31, 2022
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be on the IPv6 experimental branch? Or on master?

@Kmoneal
Copy link
Contributor Author

Kmoneal commented Jun 1, 2022

Should this be on the IPv6 experimental branch? Or on master?

Yep, I'll move it over to experimental-dual-stack

@Kmoneal Kmoneal changed the base branch from master to experimental-dual-stack June 1, 2022 21:57
@Kmoneal Kmoneal requested review from a team, linsun, nmittler and ericvn as code owners June 1, 2022 21:57
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 1, 2022
@Kmoneal Kmoneal changed the base branch from experimental-dual-stack to master June 1, 2022 21:57
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 1, 2022
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 1, 2022
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 16, 2022
@istio-testing
Copy link
Collaborator

@Kmoneal: PR needs rebase.

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.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 2, 2022
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Jul 17, 2022
@craigbox craigbox added the lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed label Jul 19, 2022
@craigbox craigbox reopened this Jul 19, 2022
@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 19, 2022
@istio-testing
Copy link
Collaborator

@Kmoneal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integ-telemetry_istio 74f9882 link true /test integ-telemetry_istio
integ-cni_istio 74f9882 link true /test integ-cni_istio
integ-distroless_istio 74f9882 link true /test integ-distroless_istio
integ-ipv6_istio 74f9882 link true /test integ-ipv6_istio
unit-tests_istio 74f9882 link true /test unit-tests_istio
release-test_istio 74f9882 link true /test release-test_istio
integ-telemetry-mc_istio 74f9882 link true /test integ-telemetry-mc_istio
integ-telemetry-istiodremote_istio 74f9882 link true /test integ-telemetry-istiodremote_istio
lint_istio 74f9882 link true /test lint_istio
analyze-tests_istio 74f9882 link true /test analyze-tests_istio
gencheck_istio 74f9882 link true /test gencheck_istio
integ-security_istio 74f9882 link true /test integ-security_istio
integ-pilot_istio 74f9882 link true /test integ-pilot_istio
integ-operator-controller_istio 74f9882 link true /test integ-operator-controller_istio
integ-helm_istio 74f9882 link true /test integ-helm_istio
release-notes_istio 74f9882 link true /test release-notes_istio
integ-security-multicluster_istio 74f9882 link true /test integ-security-multicluster_istio
integ-pilot-multicluster_istio 74f9882 link true /test integ-pilot-multicluster_istio
integ-security-istiodremote_istio 74f9882 link true /test integ-security-istiodremote_istio
integ-pilot-istiodremote_istio 74f9882 link true /test integ-pilot-istiodremote_istio
integ-pilot-istiodremote-mc_istio 74f9882 link true /test integ-pilot-istiodremote-mc_istio
integ-ambient_istio 74f9882 link true /test integ-ambient
bookinfo-build_istio 74f9882 link true /test bookinfo-build

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.

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

@howardjohn howardjohn howardjohn left review comments

@stevenctl stevenctl stevenctl left review comments

@costinm costinm Awaiting requested review from costinm

@jacob-delgado jacob-delgado Awaiting requested review from jacob-delgado

@Monkeyanator Monkeyanator Awaiting requested review from Monkeyanator

@linsun linsun Awaiting requested review from linsun

@nmittler nmittler Awaiting requested review from nmittler

@ericvn ericvn Awaiting requested review from ericvn

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/networking area/test and release lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed needs-rebase Indicates a PR needs to be rebased before being merged release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Kmoneal @istio-testing @howardjohn @stevenctl @craigbox @istio-policy-bot