istio
In echo test framework: make HasSidecar() a function of an Echo instance. Fix HasL7() filter
Please provide a description of this PR:
In the context of echo test framework :
When the namespace of an echo instance is not injected, Instance.Config.HasSidecar() function checks for the truth value "sidecar.istio.io/inject" label at the pod level . Since this label is not set to TRUE explicitly for echo instances a, b, c, the check returns false even though the pods of these services have sidecar enabled. Further Instance.Config only keeps track of labels defined in the test code and not what is inside the pods, thereby making a direct check on "istio.io/rev" label impossible.
We can rather implement this check as Instance.HasSidecar() function - using the result of Workload.Sidecar().
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: ArpithaSMalavalli (1037d324285d85b7bbf1b279364324ff6e75d056, bed46e07a4aa58d9ab0a6efaf6b4633bcbb6cd3e, e122b9ec3c1f07800b86971715058afc663cf8fc, dc8342597164d2ab8207f4db2c6b5fe2560f5010, 66657f1c2434a55f94438ba39ce90c83866a26aa)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
😊 Welcome @ArpithaSMalavalli! This is either your first contribution to the Istio istio repo, or it's been a while since you've been here.
You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines by referring to Contributing to Istio.
Thanks for contributing!
Courtesy of your friendly welcome wagon.
Hi @ArpithaSMalavalli. Thanks for your PR.
I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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.
This current implementation causes failures for multiversion/upgrade tests or tests which use revision based injection. Though those tests are running, they are currently supported by the test framework.
In such a case, namespace is not injected and echo instances like a, b and c for which sidecar.istio.io/inject" label is not set get filtered out from being possible destinations - after application of HasL7() combination filter which internally checks for Sidecar, but returns erroneous results.
https://github.com/istio/istio/blob/fb65c33b603ff4d039c60b76b765e8c406fdd539/pkg/test/framework/components/echo/echotest/run.go#L295
This creates a problem for all traffic tests that use RuntoN() i.e test that need to be run for N destinations. Majority of the destination echo instances get filtered out for failing the HasSidecar() condition.
https://github.com/istio/istio/blob/fb65c33b603ff4d039c60b76b765e8c406fdd539/tests/integration/pilot/common/traffic.go#L191
@howardjohn Please look into this.
Context:
The change in the HasSidecar() implementation seems to be affecting the ambient tests as well by causing ~550 subtest in TestTraffic to fail, which seem to be passing otherwise.
Upon further exploration of the current implementation however, these 500+ subtests in TestTraffic are being actually skipped with the error message : cases from <source echo instance> in primary-0 with <destination echo instance> as destination are removed by filters.
In the case that namespace has not been injected, the current implementation of the hasL7() filter is filtering out all echo instances in which "sidecar.istio.io/inject" is not set to true, from the list of valid destinations:
https://github.com/istio/istio/blob/fb65c33b603ff4d039c60b76b765e8c406fdd539/pkg/test/framework/components/echo/common/deployment/echos.go#L324
Question: In case of ambient mode, are these skips expected?
- If yes i.e. a,b,c, tproxy echo instances should indeed be filtered out because of the hasL7() filter in ambient mode, could you suggest a way to modify the hasL7() filter such that it doesnt filter out the echo instances in non-ambient test cases. It is required in writing tests where namespace is not injected, but we want to use these echo insatnces as destinations.
- If not, this change in verifying sidecar, is exposing the skipped tests in ambient also and is worth looking into. However its unclear why a,b,c and other instances are passing the hasSidecar() verification in ambient mode?
@ArpithaSMalavalli: 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 | 1037d324285d85b7bbf1b279364324ff6e75d056 | link | true | /test integ-telemetry |
| integ-ambient_istio | 1037d324285d85b7bbf1b279364324ff6e75d056 | link | true | /test integ-ambient |
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. I understand the commands that are listed here.
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-11-15. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.
Created by the issue and PR lifecycle manager.