Skip to content

Use TLS for websocket and gRPC tests#11322

Merged
knative-prow-robot merged 17 commits intoknative:mainfrom
nak3:add-https-ws-grpc
Jun 2, 2021
Merged

Use TLS for websocket and gRPC tests#11322
knative-prow-robot merged 17 commits intoknative:mainfrom
nak3:add-https-ws-grpc

Conversation

@nak3
Copy link
Contributor

@nak3 nak3 commented May 10, 2021

Use TLS for websocket and gRPC tests

Current gRPC and Websocket does not use TLS, but still uses HTTP even
when it runs with HTTPS mode.

This patch fixes it.

FIx #11386

@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label May 10, 2021
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label May 10, 2021
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2021
@nak3
Copy link
Contributor Author

nak3 commented May 10, 2021

/test pull-knative-serving-https

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #11322 (6f0d9e9) into main (620cb44) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11322      +/-   ##
==========================================
+ Coverage   87.66%   87.69%   +0.02%     
==========================================
  Files         191      191              
  Lines        9224     9246      +22     
==========================================
+ Hits         8086     8108      +22     
+ Misses        882      881       -1     
- Partials      256      257       +1     
Impacted Files Coverage Δ
pkg/reconciler/configuration/configuration.go 84.61% <0.00%> (-1.33%) ⬇️
pkg/apis/serving/fieldmask.go 94.85% <0.00%> (ø)
pkg/reconciler/gc/reconciler.go 100.00% <0.00%> (ø)
pkg/reconciler/labeler/labeler.go 100.00% <0.00%> (ø)
pkg/reconciler/autoscaling/kpa/kpa.go 95.12% <0.00%> (+0.08%) ⬆️
.../reconciler/serverlessservice/serverlessservice.go 93.33% <0.00%> (+0.08%) ⬆️
pkg/reconciler/domainmapping/reconciler.go 91.66% <0.00%> (+0.11%) ⬆️
pkg/reconciler/service/service.go 85.31% <0.00%> (+0.20%) ⬆️
pkg/reconciler/route/route.go 76.68% <0.00%> (+0.24%) ⬆️
pkg/reconciler/revision/revision.go 87.87% <0.00%> (+0.37%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 620cb44...6f0d9e9. Read the comment docs.

@nak3 nak3 closed this May 10, 2021
@nak3 nak3 reopened this May 18, 2021
@nak3
Copy link
Contributor Author

nak3 commented May 18, 2021

/test pull-knative-serving-https

@nak3 nak3 force-pushed the add-https-ws-grpc branch from 249e159 to bb585f6 Compare May 18, 2021 09:23
@nak3
Copy link
Contributor Author

nak3 commented May 18, 2021

/test pull-knative-serving-https

@nak3
Copy link
Contributor Author

nak3 commented May 18, 2021

/test pull-knative-serving-https

@nak3
Copy link
Contributor Author

nak3 commented May 18, 2021

Hmm... race issue.

@nak3 nak3 force-pushed the add-https-ws-grpc branch 6 times, most recently from de8d658 to 90afe0c Compare May 20, 2021 08:40
@nak3
Copy link
Contributor Author

nak3 commented May 20, 2021

/retest

passed once. One more time.

@nak3
Copy link
Contributor Author

nak3 commented May 20, 2021

/retest

@nak3 nak3 closed this May 20, 2021
@nak3 nak3 reopened this May 20, 2021
@dprotaso
Copy link
Member

/test pull-knative-serving-https

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2021
@nak3 nak3 force-pushed the add-https-ws-grpc branch from 3316765 to c2faaaa Compare June 1, 2021 01:23
@nak3
Copy link
Contributor Author

nak3 commented Jun 1, 2021

/test pull-knative-serving-https

2 similar comments
@nak3
Copy link
Contributor Author

nak3 commented Jun 1, 2021

/test pull-knative-serving-https

@nak3
Copy link
Contributor Author

nak3 commented Jun 1, 2021

/test pull-knative-serving-https

@markusthoemmes
Copy link
Contributor

Are these HTTPS leg flakes usual?

@dprotaso
Copy link
Member

dprotaso commented Jun 1, 2021

We were never running these tests correctly so they might actually be legitimate errors?

@nak3 nak3 force-pushed the add-https-ws-grpc branch from 8b41a67 to 6f0d9e9 Compare June 2, 2021 01:46
@nak3
Copy link
Contributor Author

nak3 commented Jun 2, 2021

/test pull-knative-serving-https

@nak3
Copy link
Contributor Author

nak3 commented Jun 2, 2021

Sorry last errors were caused by my wrong commit 4119f08. I reverted it now and so it will not be failed so often.

But yes, gRPC and websocket e2e never run tests with TLS correctly.

Copy link
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@knative-prow-robot knative-prow-robot merged commit 561868d into knative:main Jun 2, 2021
@nak3 nak3 deleted the add-https-ws-grpc branch June 11, 2021 01:02
nak3 added a commit to nak3/serving that referenced this pull request Jun 11, 2021
* Support websocket https

* Use TLS for websocket and gRPC tests

Current gRPC and Websocket does not use TLS, but still uses HTTP even
when it runs with HTTPS mode.

This patch fixes it.

* Add ServerName to pseudo host with TLS.

* Decrease the length of test name

* Fix race issue

* Add workaround only https

* try dialer option

* Without parallel

* Use a single DialOption

* Revert "Without parallel"

This reverts commit 99455a6.

* Revert "Revert "Without parallel""

This reverts commit 7105cbc.

* Fix TestGRPCStreamingPingViaActivator

* Do not run gRPC with parallel

* Add TODO comment

* Use net.SplitHostPort

* make long ksvc name generic

* Revert "make long ksvc name generic"

This reverts commit 4119f08.
openshift-merge-robot pushed a commit to openshift/knative-serving that referenced this pull request Jun 15, 2021
* Do not call GetIngressEndpoint() when --resolvabledomain is enabled (knative#11318)

`GetIngressEndpoint()` is a function to get ingress endpoint for the
spoof client. So we don't need to call it when `--resolvabledomain` is
enabled. Especially when Ingresss service uses `ClusterIP`, it alwasy
fails to get endpoint and test fails.

This patch changes to call GetIngressEndpoint() only when
resolvabledomain is false in websocket test.

* Use TLS for websocket and gRPC tests (knative#11322)

* Support websocket https

* Use TLS for websocket and gRPC tests

Current gRPC and Websocket does not use TLS, but still uses HTTP even
when it runs with HTTPS mode.

This patch fixes it.

* Add ServerName to pseudo host with TLS.

* Decrease the length of test name

* Fix race issue

* Add workaround only https

* try dialer option

* Without parallel

* Use a single DialOption

* Revert "Without parallel"

This reverts commit 99455a6.

* Revert "Revert "Without parallel""

This reverts commit 7105cbc.

* Fix TestGRPCStreamingPingViaActivator

* Do not run gRPC with parallel

* Add TODO comment

* Use net.SplitHostPort

* make long ksvc name generic

* Revert "make long ksvc name generic"

This reverts commit 4119f08.
markusthoemmes pushed a commit to openshift/knative-serving that referenced this pull request Jun 22, 2021
* Do not call GetIngressEndpoint() when --resolvabledomain is enabled (knative#11318)

`GetIngressEndpoint()` is a function to get ingress endpoint for the
spoof client. So we don't need to call it when `--resolvabledomain` is
enabled. Especially when Ingresss service uses `ClusterIP`, it alwasy
fails to get endpoint and test fails.

This patch changes to call GetIngressEndpoint() only when
resolvabledomain is false in websocket test.

* Use TLS for websocket and gRPC tests (knative#11322)

* Support websocket https

* Use TLS for websocket and gRPC tests

Current gRPC and Websocket does not use TLS, but still uses HTTP even
when it runs with HTTPS mode.

This patch fixes it.

* Add ServerName to pseudo host with TLS.

* Decrease the length of test name

* Fix race issue

* Add workaround only https

* try dialer option

* Without parallel

* Use a single DialOption

* Revert "Without parallel"

This reverts commit 99455a6.

* Revert "Revert "Without parallel""

This reverts commit 7105cbc.

* Fix TestGRPCStreamingPingViaActivator

* Do not run gRPC with parallel

* Add TODO comment

* Use net.SplitHostPort

* make long ksvc name generic

* Revert "make long ksvc name generic"

This reverts commit 4119f08.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestGRPCStreamingPingViaActivator does not work with https option

4 participants