[v2.4] Add more tests for TCPMappings #4493

LukeShu · 2022-09-07T20:43:36Z

Description

This PR adds a bunch of tests for TCPMappings, many of them marked xfail. Even so, it isn't quite as complete as I would like it to be; see the "TODO" comments at the bottom of t_tcpmapping.py.

This builds on the tests that are verified to work with 1.14 from #4477.

Cherry-picks some reliability improvements from #4308

Related Issues

Motivating issue: https://github.com/datawire/apro/issues/3036
Found issue while writing the tests: Can't use a TLSContext to configure origination without a secret #4476
Sibling PR: [v1.14] Add more tests for TCPMappings #4477
Cherry-picks from Improve KAT test reliability #4308

Testing

That's all this is.

Checklist

I made sure to update CHANGELOG.md. - no applicable changes

Remember, the CHANGELOG needs to mention:
- Any new features
- Any changes to our included version of Envoy
- Any non-backward-compatible changes
- Any deprecations
This is unlikely to impact how Ambassador performs at scale. - not a runtime change

Remember, things that might have an impact at scale include:
- Any significant changes in memory use that might require adjusting the memory limits
- Any significant changes in CPU use that might require adjusting the CPU limits
- Anything that might change how many replicas users should use
- Changes that impact data-plane latency/scalability
My change is adequately tested. - that's all this is

Remember when considering testing:
- Your change needs to be specifically covered by tests.
  - Tests need to cover all the states where your change is relevant: for example, if you add a behavior that can be enabled or disabled, you'll need tests that cover the enabled case and tests that cover the disabled case. It's not sufficient just to test with the behavior enabled.
- You also need to make sure that the entire area being changed has adequate test coverage.
  - If existing tests don't actually cover the entire area being changed, add tests.
  - This applies even for aspects of the area that you're not changing – check the test coverage, and improve it if needed!
- We should lean on the bulk of code being covered by unit tests, but...
- ... an end-to-end test should cover the integration points
I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently. - no tricks
The changes in this PR have been reviewed for security concerns and adherence to security best practices.

These two bumps are required for pytest to work with Python 3.10. This doesn't matter for builds, or for in CI, but this does matter for developers whose system Python is up-to-date. Since Emissary 1.14 is in maintenance mode, I did the smallest upgrades possible to get things working. For typed-ast, that means including python/typed_ast#158, which was first included in 1.4.3. For pytest, that means 6.2.5, since that's when the changelog mentions "Python 3.10 is now supported." https://docs.pytest.org/en/stable/changelog.html#pytest-6-2-5-2021-08-29 Signed-off-by: Luke Shumaker <[email protected]>

…v2.4-tcpmapping

Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit fc40a4d)

Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit 03be3e9)

…v2.4-tcpmapping2

Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit e3af51a) This cherry pick is needed because several of the certs are now expired; I don't care about the SAN, I care about the timestamps. I also had to touch up t_tls.py, since it's checking for some details of the certs, and those details changed. I didn't have to do that in the original commit because I guess those tests had been marked as xfail. Yikes! Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>

…v2.4-tcpmapping2

… [ci-skip] Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>

When splitting KAT tests among multiple jobs, we need a listing of all of the KAT tests; we'll assign each entry in this list to a different job. The problem with that right now is that the expressions in that listing are sometimes ambiguous. For example, "AuthenticationTest" shows up as `kat[AuthenticationTest`. Notice that it doesn't include a trailing `]`; this causes it to also match the "AuthenticationTestV1". If both of those tests were going to be scheduled on the same job, that's fine; but if they were going to be scheduled on different jobs, then that "AuthenticationTestV1" test will run in *both* jobs, potentially overloading the one where it wasn't supposed to run. This is probably less of a problem for that one test, and a bigger problem for common prefixes like "TLS". Here is a listing of all such ambiguities: $ grep -f <(<build-aux/pytest-kat.txt sed -e 's/\[/\\[/g' -e 's/$/./') build-aux/pytest-kat.txt kat[AuthenticationTestV1 kat[ClientCertificateAuthenticationContext kat[ClientCertificateAuthenticationContextCRL kat[DnsTtlDnsType kat[DnsTtlEndpoint kat[ErrorResponseMappingBypassAlternate kat[ErrorResponseOnStatusCodeMappingCRD kat[HostCRDDoubleCrossNamespace kat[HostCRDManualContextCRL kat[LogicalDnsTypeEndpoint kat[RedirectTestsInvalidSecret kat[RedirectTestsWithProxyProto kat[TLSCoalescing kat[TLSContextCipherSuites kat[TLSContextIstioSecretTest kat[TLSContextProtocolMaxVersion kat[TLSContextProtocolMinVersion kat[TLSContextTest kat[TLSContextsTest kat[TLSIngressTest kat[TLSInheritFromModule kat[TLSInvalidSecret kat[TLSOriginationSecret kat[TracingTestLongClusterName kat[TracingTestSampling kat[TracingTestShortTraceId kat[TracingTestZipkinV1 kat[TracingTestZipkinV2 Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit 67279ec)

Yep, because of the horror that is self.skip_node! Eradicating skip_node seems like a bigger task, so just accept the horror as is. Signed-off-by: Luke Shumaker <[email protected]>

python/tests/kat/t_tcpmapping.py

haq204 · 2022-09-14T14:42:41Z

python/tests/kat/t_tcpmapping.py

+    def manifests(self) -> str:
+        return format('''
+---
+apiVersion: getambassador.io/v3alpha1


Shouldn't we also be testing with https and port 80 to ensure that v3alpha1 honors the scheme similar to TCPMappingTLSOriginationV2SchemeTest? Seems like that's the intention as extra_ports has two elements but only one is used here.

Oops, it was a mistake that I left 6790 in extra_ports.

The difference is that in v2 it didn't support saying "originate TLS by adding https:// to the service; but in v3alpha1 this is the primary way of originating TLS without a client cert (since v3alpha1 doesn't support tls: true, which is how you did that in v2.)

I've added a commit removing 6790 from extra_ports.

haq204 · 2022-09-14T14:46:04Z

python/tests/kat/t_tcpmapping.py

+  ambassador_id: [ {self.ambassador_id} ]
+  port: 6789
+  service: {self.target.path.fqdn}:443
+  tls: true


mostly just curious what tls==true means since I thought you were supposed to supply a TLSContext to use.

tls: true was how you said "originate TLS without a client cert (and with the default settings)" in v2; in v3alpha1 you do this by adding https:// to the service (since allowing tls to be either a bool or a string is a violation of Kubernetes structural typing, which is something that v3alpha1 was going for).

haq204 · 2022-09-14T14:56:03Z

python/tests/kat/t_tcpmapping.py

+  ambassador_id: [ {self.ambassador_id} ]
+  port: 6789
+  service: {self.target.path.fqdn}:443
+  tls: {self.name.k8s}-tlsclient


What happens if you have two TLSContexts with the same name but in different namespaces?

The answer is poorly defined, and I hate it, but it's OK that it's poorly defined since it's always been poorly defined.

Good question though.

@haq204

I'd accidentally included an extra `extra_port` in one of the tests because I'd copied it from a different test. Thanks to @haq204 for pointing out this mistake. Signed-off-by: Luke Shumaker <[email protected]>

ddymko

looks fine overall

LukeShu added 7 commits August 31, 2022 20:03

Merge commit '2a8ffbfc3ff2445ce7570dcceb9b42187c3a0497' into lukeshu/…

15d2973

…v2.4-tcpmapping

Merge build-aux-local/ in to build-aux/

d7c4a29

Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit fc40a4d)

Modify test_shadow.py to use selfsigned.py

1016904

Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit 03be3e9)

Merge commit '1016904a5f0b82fb414a9acc337585f534785324' into lukeshu/…

eb00717

…v2.4-tcpmapping2

KAT: Flesh out the TCPMapping tests

02a69a9

Signed-off-by: Luke Shumaker <[email protected]>

LukeShu mentioned this pull request Sep 7, 2022

[v1.14] Add more tests for TCPMappings #4477

Merged

5 tasks

LukeShu changed the title ~~Lukeshu/v2.4 tcpmapping~~ [v2.4] Add more tests for TCPMappings Sep 7, 2022

LukeShu added 2 commits September 7, 2022 15:12

Merge commit '87487f826ac5dad7df96fbde9356d39c0ca04ec8' into lukeshu/…

28418fc

…v2.4-tcpmapping2

Merge branch 'lukeshu/v1.14-tcpmapping' into lukeshu/v2.4-tcpmapping2…

6dbafcc

… [ci-skip] Signed-off-by: Luke Shumaker <[email protected]>

LukeShu force-pushed the lukeshu/v2.4-tcpmapping branch from 863bc86 to ad1e2f2 Compare September 7, 2022 21:16

LukeShu mentioned this pull request Sep 7, 2022

[v2.4] Fix TCPMappings #4495

Closed

5 tasks

LukeShu force-pushed the lukeshu/v2.4-tcpmapping branch 3 times, most recently from c250272 to f3d5237 Compare September 12, 2022 02:20

LukeShu added 3 commits September 13, 2022 16:58

t_tcpmapping.py: Update for 2.x

0a88cb2

Signed-off-by: Luke Shumaker <[email protected]>

builder.mk: Handle pytest-kat.txt being different depending on xDS ver

edc01e7

Yep, because of the horror that is self.skip_node! Eradicating skip_node seems like a bigger task, so just accept the horror as is. Signed-off-by: Luke Shumaker <[email protected]>

LukeShu force-pushed the lukeshu/v2.4-tcpmapping branch from f3d5237 to edc01e7 Compare September 13, 2022 23:24

LukeShu mentioned this pull request Sep 13, 2022

[v2.4] Fix TCPMappings #4507

Merged

5 tasks

LukeShu marked this pull request as ready for review September 13, 2022 23:36

LanceEa requested review from LanceEa and haq204 September 14, 2022 13:43

LanceEa reviewed Sep 14, 2022

View reviewed changes

python/tests/kat/t_tcpmapping.py Show resolved Hide resolved

python/tests/kat/t_tcpmapping.py Show resolved Hide resolved

haq204 reviewed Sep 14, 2022

View reviewed changes

LukeShu mentioned this pull request Sep 16, 2022

Add yet more tests for TCPMappings #4516

Open

fixup! t_tcpmapping.py: Update for 2.x

746a052

I'd accidentally included an extra `extra_port` in one of the tests because I'd copied it from a different test. Thanks to @haq204 for pointing out this mistake. Signed-off-by: Luke Shumaker <[email protected]>

LukeShu requested review from LanceEa and haq204 September 16, 2022 05:11

LanceEa approved these changes Sep 16, 2022

View reviewed changes

ddymko approved these changes Sep 16, 2022

View reviewed changes

haq204 approved these changes Sep 16, 2022

View reviewed changes

LukeShu merged commit 0cecbde into release/v2.4 Sep 16, 2022

LukeShu deleted the lukeshu/v2.4-tcpmapping branch September 16, 2022 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2.4] Add more tests for TCPMappings #4493

[v2.4] Add more tests for TCPMappings #4493

LukeShu commented Sep 7, 2022 •

edited

Loading

haq204 Sep 14, 2022

LukeShu Sep 16, 2022

LukeShu Sep 16, 2022

haq204 Sep 14, 2022

LukeShu Sep 16, 2022

haq204 Sep 14, 2022

LukeShu Sep 16, 2022

ddymko left a comment

[v2.4] Add more tests for TCPMappings #4493

[v2.4] Add more tests for TCPMappings #4493

Conversation

LukeShu commented Sep 7, 2022 • edited Loading

Description

Related Issues

Testing

Checklist

haq204 Sep 14, 2022

Choose a reason for hiding this comment

LukeShu Sep 16, 2022

Choose a reason for hiding this comment

LukeShu Sep 16, 2022

Choose a reason for hiding this comment

haq204 Sep 14, 2022

Choose a reason for hiding this comment

LukeShu Sep 16, 2022

Choose a reason for hiding this comment

haq204 Sep 14, 2022

Choose a reason for hiding this comment

LukeShu Sep 16, 2022

Choose a reason for hiding this comment

ddymko left a comment

Choose a reason for hiding this comment

LukeShu commented Sep 7, 2022 •

edited

Loading