Issue #1579 TLSRoute Passthrough Conformance Test (normative) #1587

candita · 2022-12-08T02:01:39Z

Issue #1579 TLSRoute Passthrough Conformance Test (normative)

What type of PR is this?
/area conformance

What this PR does / why we need it:
This is the first test specified in the TLSRoute Passthrough Conformance Test suite, the normative test. This test may be used as reference to learn more about designing conformance tests, particularly for TLSRoute Passthrough.

Which issue(s) this PR fixes:
This addresses part of Issue #1579

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2022-12-08T02:01:48Z

Hi @candita. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra repository.

mikemorris · 2022-12-08T03:03:13Z

/ok-to-test

howardjohn

Did you test this on any implementations?

howardjohn · 2022-12-08T15:56:02Z

conformance/base/manifests.yaml

+        app: infra-backend-v4
+    spec:
+      containers:
+      - name: infra-backend-v4


Do we need a 4th backend (with 2 replicas)? Or can we just add TLS to the existing ones?

Agree that reusing an existing backend would be ideal here if we can.

To conveniently use the existing echoserver image, I created a new backend with the environment variables TLS_SERVER_CERT and TLS_SERVER_PRIVKEY for this image to serve certificates. See https://github.com/kubernetes-sigs/ingress-controller-conformance/blob/master/images/echoserver/echoserver.go#L109.

I also had to supply access to a new secret that is created during test Setup.

Please let me know if this is ok.

I think this could likely be added to an existing backend without breaking any conformance tests. Would be worth trying at least.

The TLS backend, infra-backend-v4, is the only one using port/targetPort 443/8443. It is also the only one setup with env variables TLS_SERVER_CERT and TLS_SERVER_PRIVKEY, which as mentioned directs it to validate certs in the echoserver app.

It works fine with 1 replica for now, and will be reused for the rest of the TLSRoute Passthrough conformance tests. I think putting all of this together, that TLSRoute Passthrough should have its own configuration with non-standard ports and env variables.

I will rename it from infra-backend-v4 to tls-backend so that it is clear that this is a specially configured backend.

I'm OK with a new backend here, but can we add this TLS listener to an existing Gateway instead of adding a new Gateway just for this test?

robscott

Thanks for all the work on this @candita! Looks like a great start.

conformance/tests/tlsroute-simple-same-namespace.go

conformance/base/manifests.yaml

robscott · 2022-12-12T06:04:51Z

conformance/base/manifests.yaml

+        app: infra-backend-v4
+    spec:
+      containers:
+      - name: infra-backend-v4


Agree that reusing an existing backend would be ideal here if we can.

conformance/utils/tls/tls.go

conformance/tests/tlsroute-simple-same-namespace.yaml

candita · 2022-12-12T22:42:03Z

Did you test this on any implementations?

Yes, I tested on Istio, but had to tweak it to make it get past some of the condition naming updates in Gateway API.

…mative) rebase

robscott

Thanks @candita! I haven't had a chance to run this test yet, but this looks promising from what I can tell.

conformance/conformance_test.go

robscott · 2022-12-20T22:28:30Z

conformance/base/manifests.yaml

+        app: infra-backend-v4
+    spec:
+      containers:
+      - name: infra-backend-v4


I think this could likely be added to an existing backend without breaking any conformance tests. Would be worth trying at least.

conformance/utils/roundtripper/roundtripper.go

conformance/utils/kubernetes/helpers.go

conformance/utils/roundtripper/roundtripper.go

skriss · 2022-12-21T22:12:21Z

JFYI, I was able to run this test and have it pass against Contour's implementation which has support for TLS passthrough. I haven't reviewed the code yet but will try to do so soon. Thanks!

robscott · 2023-01-11T02:05:26Z

Thanks @candita! This change LGTM, but would like someone else to sign off on this as well.

/cc @skriss @arkodg @shaneutt

Do any of you have time to run through this as well?

/approve

k8s-ci-robot · 2023-01-11T02:05:29Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: arkodg.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Thanks @candita! This change LGTM, but would like someone else to sign off on this as well.

/cc @skriss @arkodg @shaneutt

Do any of you have time to run through this as well?

/approve

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.

arkodg · 2023-01-11T02:23:22Z

conformance/utils/tls/tls.go

+// for MakeRequestAndExpectEventuallyConsistentResponse to consider the response "consistent"
+// before making additional assertions on the response body. If this number is not reached within
+// maxTimeToConsistency, the test will fail.
+const requiredConsecutiveSuccesses = 3


nit: should this knob live closer to where maxTimeToConsistency is defined since its applies to the conformance suite in general

Sure. It was used in http.go as well, so I made it a global parameter of timeoutConfig in timeout.go

arkodg · 2023-01-11T02:25:24Z

site-src/concepts/conformance.md


 ```shell
-go test ./conformance --gateway-class my-class
+go test ./conformance/... -args -gateway-class=istio


nit: maybe stick to the neutral my-class string ?

You're right. When I began this, I wasn't sure if I needed to create a new gatewayClass, and it was helpful for me to get the example for one implementation. I reworded the rest of this in an attempt to make it clearer to newcomers. Maybe at some point we could list all the gateway-class names.

arkodg · 2023-01-11T02:30:56Z

conformance/utils/roundtripper/roundtripper.go

@@ -43,6 +46,9 @@ type Request struct {
 	Method           string
 	Headers          map[string][]string
 	UnfollowRedirect bool
+	CertPem          []byte
+	KeyPem           []byte
+	Server           string


can the Host field be reused ?

I considered Host and Server to be different. Server is one of the TLSRoute.Spec.Hostnames that aligns with CertPem and KeyPem, and is passed as the TLSClientConfig.ServerName. Since TLSRoute.Spec.Hostnames can contain wildcards, it might not match the host. Ie, we could have a TLS server *.example.com but a request host abc.example.com.

gyohuangxin · 2023-01-11T02:59:30Z

site-src/concepts/conformance.md

+If you'd like to run a single test instead of the entire conformance suite, find your test name
+`(suite.ConformanceTest.ShortName)` and use it like this:
+```shell
+go test ./conformance/... --run TestConformance/YOURTESTNAME --gateway-class=istio


We can use flag supported-features to specify the tests to run, for example:

go test ./conformance/... -supported-features=TLSRoute -gateway-class=my-class

In this case, I wanted to explain how to run a single test case by name. I haven't quite figured out how to use the supported-features flag to my advantage, so I would ask for someone else to document that.

sunjayBhatia

test works with contour and from a quick look seems correct 👍🏽

…latest

candita · 2023-01-18T00:59:47Z

@robscott @arkodg thanks for the review. I made some more changes and responded to your comments on changes I didn't make. Could you PTAL when you get a chance ?

arkodg

LGTM thanks !

candita · 2023-01-20T20:35:41Z

@skriss @shaneutt this has been /approved by @robscott and LGTM by @arkodg. It passes for Istio and Contour. Could one of you please give it the /lgtm and allow to merge?

candita · 2023-01-20T20:41:28Z

/label tide/merge-method-squash

shaneutt

/lgtm

k8s-ci-robot · 2023-01-23T16:13:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, candita, robscott, shaneutt

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:

~~OWNERS~~ [robscott,shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2023-01-23T22:51:51Z

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

Test name	Commit	Details	Required	Rerun command
pull-gateway-api-verify	`7be6387`	link	unknown	`/test pull-gateway-api-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

shaneutt · 2023-01-23T23:01:02Z

/retest

* Issue #1579 TLSRoute Passthrough Conformance Test (normative) rebase * Issue #1579 TLSRoute Passthrough - PR review update - rebase * Issue #1579 TLSRoute Passthrough - PR review update - latest

k8s-ci-robot added the area/conformance label Dec 8, 2022

k8s-ci-robot requested review from bowei and mikemorris December 8, 2022 02:01

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 8, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 8, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 8, 2022

howardjohn reviewed Dec 8, 2022

View reviewed changes

robscott reviewed Dec 12, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2022

Issue kubernetes-sigs#1579 TLSRoute Passthrough Conformance Test (nor…

ff69601

…mative) rebase

candita force-pushed the 1452-tls-passthrough-conformance branch from d8212e6 to ab91a3f Compare December 20, 2022 00:43

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

candita force-pushed the 1452-tls-passthrough-conformance branch 3 times, most recently from ffde551 to 22f2099 Compare December 20, 2022 20:37

robscott reviewed Dec 20, 2022

View reviewed changes

This was referenced Jan 3, 2023

update to Kubernetes 1.26.x envoyproxy/gateway#682

Closed

TLSRoute Conformance Test envoyproxy/gateway#577

Closed

candita force-pushed the 1452-tls-passthrough-conformance branch 2 times, most recently from 0aecd97 to c2db3c5 Compare January 3, 2023 23:05

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 3, 2023

candita force-pushed the 1452-tls-passthrough-conformance branch from dd79b10 to 6a39c14 Compare January 4, 2023 00:01

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2023

k8s-ci-robot 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 Jan 9, 2023

k8s-ci-robot requested review from skriss and shaneutt January 11, 2023 02:05

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023

arkodg reviewed Jan 11, 2023

View reviewed changes

gyohuangxin reviewed Jan 11, 2023

View reviewed changes

sunjayBhatia reviewed Jan 12, 2023

View reviewed changes

Issue kubernetes-sigs#1579 TLSRoute Passthrough - PR review update - …

7be6387

…latest

arkodg approved these changes Jan 18, 2023

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 20, 2023

shaneutt requested a review from youngnick January 20, 2023 21:15

shaneutt approved these changes Jan 23, 2023

View reviewed changes

k8s-ci-robot assigned shaneutt Jan 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2023

k8s-ci-robot merged commit 75a2b64 into kubernetes-sigs:main Jan 23, 2023

shaneutt added this to the v0.6.1 milestone Feb 6, 2023

arkodg mentioned this pull request Mar 7, 2023

REQUEST: New membership for arkodg kubernetes/org#4081

Closed

9 tasks

candita mentioned this pull request Sep 22, 2023

REQUEST: New membership for candita kubernetes/org#4477

Closed

9 tasks

Issue #1579 TLSRoute Passthrough Conformance Test (normative) #1587

Issue #1579 TLSRoute Passthrough Conformance Test (normative) #1587

Conversation

candita commented Dec 8, 2022

k8s-ci-robot commented Dec 8, 2022

mikemorris commented Dec 8, 2022

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented Dec 12, 2022

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Dec 21, 2022

robscott commented Jan 11, 2023

k8s-ci-robot commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

candita commented Jan 18, 2023

arkodg left a comment

Choose a reason for hiding this comment

candita commented Jan 20, 2023

candita commented Jan 20, 2023

shaneutt left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 23, 2023

k8s-ci-robot commented Jan 23, 2023

shaneutt commented Jan 23, 2023

candita Dec 12, 2022 •

edited

Loading