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

tests: create ServiceEntry with multiple VIPs in DNSTestCases #51995

Merged
merged 1 commit into from
Jul 24, 2024
Merged

tests: create ServiceEntry with multiple VIPs in DNSTestCases #51995

merged 1 commit into from
Jul 24, 2024

Conversation

jewertow
Copy link
Member

@jewertow jewertow commented Jul 10, 2024 •
edited
Loading

Please provide a description of this PR:

It turned out that #30282 was fixed in #51967. However, I found another bug (?) related to ServiceEntry with multiple VIPs and described it in a comment. If we agree that the described bug should be fixed, I can create an issue with reproducing steps.

@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 Jul 10, 2024
@istio-policy-bot istio-policy-bot added area/test and release release-notes-none Indicates a PR that does not require release notes. labels Jul 10, 2024
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 10, 2024
@jewertow
Copy link
Member Author

/test all

2 similar comments
@jewertow
Copy link
Member Author

/test all

@jewertow
Copy link
Member Author

/test all

@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote-mc

@istio-testing
Copy link
Collaborator

@jewertow: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test bookinfo-build
  • /test gencheck
  • /test integ-ambient
  • /test integ-basic-arm64
  • /test integ-cni
  • /test integ-distroless
  • /test integ-ds
  • /test integ-helm
  • /test integ-ipv6
  • /test integ-operator-controller
  • /test integ-pilot
  • /test integ-pilot-istiodremote
  • /test integ-pilot-istiodremote-mc
  • /test integ-pilot-multicluster
  • /test integ-security
  • /test integ-security-istiodremote
  • /test integ-security-multicluster
  • /test integ-telemetry
  • /test integ-telemetry-discovery
  • /test integ-telemetry-istiodremote
  • /test integ-telemetry-mc
  • /test lint
  • /test macbuildcheck
  • /test release-notes
  • /test release-test
  • /test unit-tests
  • /test unit-tests-arm64

The following commands are available to trigger optional jobs:

  • /test benchmark
  • /test experimental-tracing
  • /test integ-ambient-calico
  • /test integ-ambient-dual
  • /test integ-ambient-ipv6
  • /test integ-assertion

Use /test all to run the following jobs that were automatically triggered:

  • gencheck_istio
  • integ-ambient_istio
  • integ-basic-arm64_istio
  • integ-cni_istio
  • integ-distroless_istio
  • integ-ds_istio
  • integ-helm_istio
  • integ-ipv6_istio
  • integ-operator-controller_istio
  • integ-pilot-istiodremote-mc_istio
  • integ-pilot-istiodremote_istio
  • integ-pilot-multicluster_istio
  • integ-pilot_istio
  • integ-security-istiodremote_istio
  • integ-security-multicluster_istio
  • integ-security_istio
  • integ-telemetry-discovery_istio
  • integ-telemetry-istiodremote_istio
  • integ-telemetry-mc_istio
  • integ-telemetry_istio
  • lint_istio
  • macbuildcheck_istio
  • release-notes_istio
  • release-test_istio
  • unit-tests-arm64_istio
  • unit-tests_istio

In response to this:

/test integ-pilot-mc

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-sigs/prow repository.

@jewertow
Copy link
Member Author

/test integ-pilot-multicluster

@jewertow
Copy link
Member Author

/test integ-pilot-istiodremote-mc

2 similar comments
@jewertow
Copy link
Member Author

/test integ-pilot-istiodremote-mc

@jewertow
Copy link
Member Author

/test integ-pilot-istiodremote-mc

@jewertow
Copy link
Member Author

/test all

1 similar comment
@jewertow
Copy link
Member Author

/test all

@jewertow jewertow changed the title tests: configure multiple VIPs in DNSTestCases dns: fix building name table for external services with multiple VIPs Jul 11, 2024
@jewertow
Copy link
Member Author

/test all

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2024
@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 Jul 17, 2024
@jewertow jewertow force-pushed the enable-test-dns branch 2 times, most recently from 4ece551 to bab7c41 Compare July 17, 2024 12:17
@jewertow
Copy link
Member Author

/test all

@jewertow
Copy link
Member Author

/test all

@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

1 similar comment
@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

1 similar comment
@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

@jewertow
Copy link
Member Author

/test all

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 24, 2024
@jewertow
Copy link
Member Author

/test all

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2024
@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

2 similar comments
@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

@jewertow
Copy link
Member Author

/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster

@jewertow
Copy link
Member Author

/test all

@jewertow jewertow changed the title dns: fix building name table for external services with multiple VIPs tests: create ServiceEntry with multiple VIPs in DNSTestCases Jul 24, 2024
@jewertow jewertow marked this pull request as ready for review July 24, 2024 12:41
@jewertow jewertow requested a review from a team as a code owner July 24, 2024 12:41
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 24, 2024
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

howardjohn
howardjohn reviewed Jul 24, 2024
View reviewed changes
tests/integration/pilot/common/routing.go
Comment on lines 3505 to 3511
} else if v4 {
expectedIPv4 = []string{"1.2.3.4", "1.2.3.5"}
expectedIPv6 = []string{"1234:1234:1234::1234:1234:1234"}
} else {
expectedIPv4 = []string{"1.2.3.4"}
expectedIPv6 = []string{"1234:1234:1234::1234:1234:1234", "1235:1235:1235::1235:1235:1235"}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the expected behavior? It seems a bit un-intuitive that when we are in "v4 only mode" we still return IPv6 addresses, just not all of them?

It may be correct, just not clear why -- a comment would help

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it is not intuitive. These semantics comes from GetAllAddressesForProxy. That function tries to return all available addresses if ambient or dual-stack mode is enabled. Otherwise, it returns all of the addresses for the supported IP family. If there is no IP for the supported family, it fallbacks to the default (first) address.

DNS proxy uses GetAllAddressesForProxy to build name table, so it has the same semantics, i.e. in IPv4-only stack it will return all IPv4 addresses for A query and first IPv6 address for AAAA query.

It may be correct - to be honest, I am not sure if this is correct, but this is backward compatible.

Copy link
Member

Choose a reason for hiding this comment

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

TBH this behavior seems totally broken. Can we open an issue to track it?

For now doesn't need to block the PR though, good to test our behavior even if its wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we open an issue to track it?

Of course, I will do it.

@istio-testing istio-testing merged commit 26637f4 into istio:master Jul 24, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
area/test and release 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
4 participants
@jewertow @istio-testing @istio-policy-bot @howardjohn