Add option to run all e2e tests with different http and https settings #5157

taragu · 2019-08-14T20:43:05Z

/lint

Part of #5148

Proposed Changes

Add option to run all e2e tests with different http and https settings

Release Note

NONE

/cc @JRBANCEL
/cc @ZhiminXiang

knative-prow-robot · 2019-08-14T20:43:20Z

Hi @taragu. Thanks for your PR.

I'm waiting for a knative 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.

JRBANCEL · 2019-08-14T20:49:43Z

test/config-network/config-network-http-https.yaml

+  httpProtocol: "Enabled"
+  autoTLS: "Enabled"
+
+  _example: |


Do we need to keep the _example?
It makes things less clear I feel.

JRBANCEL · 2019-08-14T20:51:30Z

test/e2e-common.sh

+  if (( HTTP_ENABLED )) &&  (( AUTO_TLS_ENABLED )); then
+    ko apply ${KO_FLAGS} -f test/config-network/config-network-http-https.yaml || return 1
+  elif (( HTTP_ENABLED )) && (( ! AUTO_TLS_ENABLED )); then
+    ko apply ${KO_FLAGS} -f test/config-network/config-network-http.yaml || return 1


http-only might be more explicit than http.
Same with https-only vs. https.

@JRBANCEL originally I was thinking of having flags that corresponded directly to the yaml values:

--http --> httpProtocol: "Enabled" --no-http --> httpProtocol: "Disabled" --auto-tls --> autoTLS: "Enabled" --no-auto-tls --> autoTLS: "Disabled"

If we have flags http-only and https-only, do we need to introduce a third flag to indicate http and https?

I would prefer not introducing auto TLS into this PR if we just want to test different HTTP and HTTPS setting because:

auto TLS needs more setup (DNS server, real domain, static IP, etc.)

If we run all E2E tests with auto TLS enabled, we can quickly run out of TLS certificates quota.

In this PR, I think we are targeting to test the scenario that HTTP server (https://github.com/knative/serving/blob/master/config/202-gateway.yaml#L27) is removed, and HTTPS server with certificates is added (https://github.com/knative/serving/blob/master/config/202-gateway.yaml#L34). So probably one flag is enough: --https-only. WDYT?

In addition, instead of customizing config-network.yaml, I think we just need to customize https://github.com/knative/serving/blob/master/config/202-gateway.yaml because httpProtocol:Disabled only works when we turn on either flag autoTLS or reconcileExternalGateway which is bit more complex to set up.

Thanks @ZhiminXiang ! I've added a gateway yaml for https only at test/https-gateway.yaml. I'm not sure where to put it so please let me know if you think there's a better destination.

JRBANCEL

This is good, but AutoTLS won't work magically right?
We need to set up some domain name and hook it up with Let'sEncrypt for example.
We will probably hit the Let'sEncrypt limits. @ZhiminXiang can you comment on how to set this up?

JRBANCEL · 2019-08-14T20:54:08Z

/ok-to-test

ZhiminXiang · 2019-08-15T00:26:41Z

test/config-network/config-network-http-https.yaml

+
+data:
+  httpProtocol: "Enabled"
+  autoTLS: "Enabled"


We cannot turn on auto TLS in our E2E tests until 1) either we set up DNS servers for the real domain to get real CA signed TLS certificates or 2) we use CA issuers #4066 (comment) to get self-signed certificates.

At this point, in order to test HTTPS enabled, we may need to manually provide a self-signed certificate by using openssl and configure Gateway to support TLS.

adrcunha · 2019-08-15T01:14:43Z

Script-wise, the approach LGTM.
/approve

mattmoor-sockpuppet

Found go linting violations, please merge: taragu#5

taragu · 2019-08-15T14:24:20Z

/test pull-knative-serving-build-tests

ZhiminXiang · 2019-08-16T20:52:42Z

test/e2e-common.sh

+
+  # Depending on the flag `HTTPS_ONLY`, reapply the gateway yaml with https only
+  if (( HTTPS_ONLY )); then
+    ko apply ${KO_FLAGS} -f test/https-gateway.yaml || return 1


/cc @adrcunha
I think we probably want to put the yaml under test/config/https-gateway/ directory.

ZhiminXiang · 2019-08-16T21:06:10Z

test/https-gateway.yaml

+    hosts:
+    - "*"
+    tls:
+      mode: PASSTHROUGH


PASSTHOUGH actually does not work. Can we change it to:

tls: mode: SIMPLE credentialName: "tls-certificate" hosts: - "*"

We can think about how to generate the certificate as a TODO.

@ZhiminXiang I just applied this config and tried running the TestHelloWorld e2e test, but looking at the logs, routes were ready but they were not https. The test failed with. Am I missing something here?

kubelogs.go:152: I 13:03:08.438 [route-controller] [serving-tests/hello-world-uyuibzpu] Updating targeted revisions. kubelogs.go:152: I 13:03:08.438 [route-controller] [serving-tests/hello-world-uyuibzpu] Creating placeholder k8s services kubelogs.go:152: I 13:03:08.455 [route-controller] [serving-tests/hello-world-uyuibzpu] Creating Ingress. kubelogs.go:152: I 13:03:08.482 [route-controller] [serving-tests/hello-world-uyuibzpu] Updating placeholder k8s services with clusterIngress information kubelogs.go:152: I 13:03:08.482 [route-controller] [serving-tests/hello-world-uyuibzpu] Route successfully synced kubelogs.go:152: I 13:03:08.482 [route-controller] [serving-tests/hello-world-uyuibzpu] Reconcile succeeded. Time taken: 44.180415ms. service.go:133: Checking to ensure Service Status is populated for Ready service hello-world-uyuibzpu service.go:139: Getting latest objects Created by Service hello-world-uyuibzpu kubelogs.go:152: D 13:03:09.241 [kpa-class-podautoscaler-controller] [serving-tests/hello-world-uyuibzpu-qm8r9] No data to scale on yet service.go:142: Successfully created Service hello-world-uyuibzpu helloworld_test.go:66: The endpoint for Route hello-world-uyuibzpu at domain hello-world-uyuibzpu.serving-tests.169.46.25.10.xip.io didn't serve the expected text "Hello World! How about some tasty noodles?": response: status: 404, body: , headers: map[Content-Length:[0] Date:[Mon, 19 Aug 2019 13:03:09 GMT] Server:[istio-envoy] Zipkin_trace_id:[34de09e4cccf986c15324e8dc95c5bc9]] did not pass checks: status = 404, want one of: [200]

EDIT: Actually I missed setting up the CA issuer (thanks @rmoe for the instructions). The TestHelloWorld test passes now

adrcunha

This option won't be automatically tested by presubmits or CI flows. Is it intended?

taragu · 2019-08-19T14:35:30Z

@adrcunha that's correct. The option won't be enabled until we add the command with --https-only to https://github.com/knative/test-infra/blob/master/ci/prow/config_knative.yaml#L15

ZhiminXiang · 2019-08-19T14:39:49Z

I am not sure if all of the E2E tests will be passed under this environment. We may want to start with making it as post-submit, and then consider to make it presubmit after we fix all of E2E tests.

taragu · 2019-10-30T14:43:28Z

/assign @tcnghia

taragu · 2019-10-31T21:42:31Z

@adrcunha @ZhiminXiang @tcnghia could you please take a look when you get a chance?

adrcunha

/lgtm

LGTM for the test-infra, but I would like yet another pair of eyes on the PR before approval.

test/e2e/destroypod_test.go

ZhiminXiang · 2019-11-01T17:38:38Z

test/v1alpha1/service.go

+	if err != nil {
+		t.Fatalf("Failed to get Gateway %s/%s", Namespace, GatewayName)
+	}
+	curGateway.Spec.Servers = oldGateway.Spec.Servers


nit: we probably want to check if the Spec.Servers are the same btw currGateway and oldGateway. If they are the same, we don't need update.

ZhiminXiang · 2019-11-01T17:50:41Z

test/v1alpha1/service.go

+			},
+			TLS: tlsOptions,
+		}}
+		setupHTTPS(t, clients.KubeClient, []string{names.Service + "." + GetDomain(t, clients)})


Can we use names.URL (

serving/test/crd.go

Line 36 in fdcec3b

URL *url.URL

) here?

Hmm names.URL actually won't be set until after the service is created, so I think this is the best way.

I am not sure if we can move the part of configuring https (https://github.com/knative/serving/pull/5157/files#diff-cd1624ee494bbca6fd2e1e1a908d0dd3R131) after validateCreatedServiceStatus function (https://github.com/knative/serving/pull/5157/files#diff-cd1624ee494bbca6fd2e1e1a908d0dd3R174).
If we can, then names.URL will be available after validateCreatedServiceStatus function. And then we can use names.URL instead of using GetDomain.

@ZhiminXiang yes that does work. Thank you for the suggestion!

test/v1alpha1/service.go

taragu · 2019-11-04T16:14:59Z

Thanks @ZhiminXiang @adrcunha @JRBANCEL for the review! I made a change to add transport option as a return arg for CreateRunLatestServiceReady as Zhimin brought up the problem of running tests in parallel. Please let me know what you think.

JRBANCEL · 2019-11-04T17:16:17Z

/lgtm

…n with https flag on

taragu · 2019-11-05T23:13:39Z

/retest

ZhiminXiang · 2019-11-06T19:06:56Z

test/v1alpha1/service.go

+			},
+			TLS: tlsOptions,
+		}}
+		domainName := strings.SplitN(names.URL.Host, ".", 2)[1]


Do we still need to do the splitN here?
From what I saw, we get the suffix of names.URL.Host, and then we combine it with the service name, which is essentially the same as names.URL.Host. So I think we can use names.URL.Host directly here.

Yeah I tried using names.URL.Host but it didn't work because it has the service name. domain name is serving-tests.169.48.185.45.xip.io but names.URL.Host is hello-world-gzzyzjel.serving-tests.169.48.185.45.xip.io

what I mean here is we don't need to do SplitN and then combine the domain with service name in https://github.com/knative/serving/pull/5157/files/72501eed94be585c3d9060af4eabf2183b480185#diff-cd1624ee494bbca6fd2e1e1a908d0dd3R485. Instead, we can directly pass names.URL.Host to setUpHTTPS function.

Ah got it. That makes sense. I've just pushed up the simplification.

ZhiminXiang · 2019-11-06T19:35:44Z

/lgtm

taragu · 2019-11-06T21:45:44Z

@ZhiminXiang thanks for the review! @adrcunha would you take another look when you get a chance?

adrcunha · 2019-11-06T22:00:21Z

/lgtm
/approve

tcnghia · 2019-11-06T23:23:06Z

/approve

knative-prow-robot · 2019-11-06T23:23:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, taragu, tcnghia

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:

~~pkg/reconciler/OWNERS~~ [tcnghia]
~~test/OWNERS~~ [adrcunha,tcnghia]

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

taragu · 2019-11-06T23:30:09Z

/retest

knative-test-reporter-robot · 2019-11-06T23:35:17Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests	pull-knative-serving-unit-tests	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-unit-tests

knative-prow-robot requested review from JRBANCEL and ZhiminXiang August 14, 2019 20:43

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 14, 2019

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2019

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Aug 14, 2019

JRBANCEL reviewed Aug 14, 2019

View reviewed changes

knative-prow-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 Aug 14, 2019

ZhiminXiang reviewed Aug 15, 2019

View reviewed changes

ZhiminXiang requested a review from adrcunha August 15, 2019 00:27

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2019

taragu force-pushed the e2e-http-https branch from 2c6bc69 to 285b380 Compare August 15, 2019 14:03

mattmoor-sockpuppet reviewed Aug 15, 2019

View reviewed changes

taragu force-pushed the e2e-http-https branch from 285b380 to 1752526 Compare August 16, 2019 19:40

knative-prow-robot 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 Aug 16, 2019

ZhiminXiang reviewed Aug 16, 2019

View reviewed changes

taragu force-pushed the e2e-http-https branch from 1752526 to f227b2b Compare August 18, 2019 14:18

adrcunha reviewed Aug 19, 2019

View reviewed changes

knative-prow-robot 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 Aug 21, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot assigned tcnghia Oct 30, 2019

adrcunha reviewed Nov 1, 2019

View reviewed changes

test/e2e/destroypod_test.go Show resolved Hide resolved

ZhiminXiang reviewed Nov 1, 2019

View reviewed changes

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2019

taragu force-pushed the e2e-http-https branch from b03952e to e66eb18 Compare November 4, 2019 15:11

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 4, 2019

Tara Gu added 2 commits November 5, 2019 17:19

Add https and gateway setup logic and make helloworld test able to ru…

ce76824

…n with https flag on

Remove GetDomain

72501ee

taragu force-pushed the e2e-http-https branch from 9c9b613 to 72501ee Compare November 5, 2019 22:19

ZhiminXiang reviewed Nov 6, 2019

View reviewed changes

More simplification

84fd9cd

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2019

knative-prow-robot merged commit 6d165dd into knative:master Nov 6, 2019

taragu deleted the e2e-http-https branch November 7, 2019 12:34

mvinkler mentioned this pull request Nov 12, 2021

Introduce --https flag which will add option to run all e2e tests with different http and https settings knative/client#1520

Open

Add option to run all e2e tests with different http and https settings #5157

Add option to run all e2e tests with different http and https settings #5157

Conversation

taragu commented Aug 14, 2019

Proposed Changes

knative-prow-robot commented Aug 14, 2019

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

JRBANCEL left a comment

Choose a reason for hiding this comment

JRBANCEL commented Aug 14, 2019

Choose a reason for hiding this comment

adrcunha commented Aug 15, 2019

mattmoor-sockpuppet left a comment

Choose a reason for hiding this comment

taragu commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taragu Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

adrcunha left a comment

Choose a reason for hiding this comment

taragu commented Aug 19, 2019

ZhiminXiang commented Aug 19, 2019

taragu commented Oct 30, 2019

taragu commented Oct 31, 2019

adrcunha left a 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

taragu commented Nov 4, 2019

JRBANCEL commented Nov 4, 2019

taragu commented Nov 5, 2019

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

ZhiminXiang commented Nov 6, 2019

taragu commented Nov 6, 2019

adrcunha commented Nov 6, 2019

tcnghia commented Nov 6, 2019

knative-prow-robot commented Nov 6, 2019

taragu commented Nov 6, 2019

knative-test-reporter-robot commented Nov 6, 2019

taragu Aug 19, 2019 •

edited

Loading