Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work In Progress: Sslpassthrough on nginx #4770

Open
wants to merge 6 commits into
base: master
from

Conversation

@dm3ch
Copy link

dm3ch commented Nov 21, 2019

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2397 closes #2380

Special notes for your reviewer:

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2019

Hi @dm3ch. Thanks for your PR.

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dm3ch
To complete the pull request process, please assign bowei
You can assign the PR to them by writing /assign @bowei in a comment when ready.

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

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

@k8s-ci-robot k8s-ci-robot requested review from cmluciano and ElvinEfendi Nov 21, 2019
@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 64668cf to 4e1db75 Nov 22, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 22, 2019

@aledbf FYI

@dm3ch dm3ch changed the title Sslpassthrough on nginx Work In Progress: Sslpassthrough on nginx Nov 22, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 22, 2019

/help

Suddenly for me locally e2e don't works even on master :(
It returns on master a lot of errors.

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Nov 22, 2019

Please post a link with a gist of the error

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Nov 22, 2019

/ok-to-test

@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 22, 2019

@aledbf How can I reproduce pull-ingress-nginx-test job locally?

make cover fails on compilation om my Mac:

make cover                                                                                           go1.12.9 Go  ✔  14:04:21
# os/user
../../../../../.gvm/gos/go1.12.9/src/os/user/getgrouplist_unix.go:16:35: warning: passing 'gid_t *' (aka 'unsigned int *') to parameter of type 'int *' converts between pointers to integer types with different sign [-Wpointer-sign]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/unistd.h:650:43: note: passing argument to parameter here
?   	k8s.io/ingress-nginx/cmd/dbg	[no test files]
Coverage test k8s.io/ingress-nginx/cmd/dbg took 1 seconds
# os/user
../../../../../.gvm/gos/go1.12.9/src/os/user/getgrouplist_unix.go:16:35: warning: passing 'gid_t *' (aka 'unsigned int *') to parameter of type 'int *' converts between pointers to integer types with different sign [-Wpointer-sign]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/unistd.h:650:43: note: passing argument to parameter here
# k8s.io/ingress-nginx/cmd/nginx.test
runtime/cgo(__TEXT/__text): relocation target x_cgo_inittls not defined
runtime/cgo(__TEXT/__text): relocation target x_cgo_threadentry not defined
FAIL	k8s.io/ingress-nginx/cmd/nginx [build failed]
make: *** [cover] Error 1

I'm very sorry for asking such dumb questions.

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Nov 25, 2019

@dm3ch right now tests require linux (for cgroups) or write in /etc/* (I will change this)

In the meantime, you can use docker, like

./build/run-in-docker.sh make test
@aledbf aledbf added this to In Progress in 0.27.0 Nov 26, 2019
@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 30e7c71 to 143cfa7 Nov 27, 2019
})
}

if len(servers) > 0 {

This comment has been minimized.

Copy link
@dm3ch

dm3ch Nov 27, 2019

Author

@aledbf Sorry for mention, but suddenly I can't understand why without this check both empty not nil slice and nil slice causes https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/ingress-nginx/4770/pull-ingress-nginx-test/1199809229490229248

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2019

Codecov Report

Merging #4770 into master will increase coverage by 0.13%.
The diff coverage is 19.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4770      +/-   ##
==========================================
+ Coverage   58.59%   58.72%   +0.13%     
==========================================
  Files          88       89       +1     
  Lines        6702     6699       -3     
==========================================
+ Hits         3927     3934       +7     
+ Misses       2349     2338      -11     
- Partials      426      427       +1
Impacted Files Coverage Δ
internal/ingress/types.go 0% <ø> (ø) ⬆️
internal/ingress/controller/template/template.go 78% <0%> (-2.24%) ⬇️
internal/ingress/annotations/annotations.go 81.94% <100%> (+0.25%) ⬆️
internal/ingress/controller/controller.go 50.55% <100%> (+0.06%) ⬆️
internal/ingress/annotations/proxyprotocol/main.go 100% <100%> (ø)
internal/ingress/controller/nginx.go 31.45% <8%> (+1.75%) ⬆️
internal/watch/file_watcher.go 80.76% <0%> (-3.85%) ⬇️
internal/ingress/metric/collectors/process.go 88.29% <0%> (-2.13%) ⬇️

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 46953cc...26ebea7. Read the comment docs.

@dm3ch dm3ch requested a review from aledbf Nov 28, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 28, 2019

Ok now all Ci tasks passes.
I haven't yet tested this manually.

It seems that there's no e2e tests for checking passthrough and proxyprotocol, so i think it would be great to add them, but suddenly I'm new with e2e testing and we need some special backend for such tests (backend that could supports proxy protocol and with built in TLS support)

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Nov 28, 2019

@dm3ch please squash the commits

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Nov 28, 2019

It seems that there's no e2e tests for checking passthrough and proxyprotocol,

Right, you can start adding a simple one only using passthrough

  1. Add flag to deployment like https://github.com/kubernetes/ingress-nginx/blob/master/test/e2e/settings/default_ssl_certificate.go#L49
  2. https://github.com/kubernetes/ingress-nginx/blob/master/test/e2e/annotations/connection.go to setup an ingress with the passthrough annotation
  3. Create a service listening for TLS connections (we don't have one now)
@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 7a79805 to 2fba36d Nov 28, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/L labels Nov 28, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 28, 2019

@dm3ch please squash the commits

Done

@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 2fba36d to db38b4c Nov 28, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels Nov 28, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Nov 30, 2019

/test pull-ingress-nginx-e2e-1-12

@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 5f53dd6 to 5c033cc Nov 30, 2019
@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 5c033cc to c1898f0 Nov 30, 2019
@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Dec 1, 2019
@dm3ch dm3ch force-pushed the dm3ch:sslpassthrough-on-nginx branch from 7bea34c to dd5d114 Dec 1, 2019
dm3ch added 2 commits Dec 1, 2019
@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Dec 1, 2019

@aledbf While I was writing e2e tests I found that currently upstreams generated empty for sslpassthrough backends. As far as I understood reading current nginx.tpl nowdays all backends uses single lua generated upstream.

Should ssl-passthrough upstreams use Lua based upstreams too?

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Dec 1, 2019

Should ssl-passthrough upstreams use Lua based upstreams too?

Yes. Same logic than TCP/UDP feature (maybe you will need to add a new struct to pass to the lua endpoint)

@dm3ch

This comment has been minimized.

Copy link
Author

dm3ch commented Dec 1, 2019

Should ssl-passthrough upstreams use Lua based upstreams too?

Yes. Same logic than TCP/UDP feature

Yeah but TCP/UDP loadbalancer doesn't operate hostnames it works only with ports.

Is it possible to use existing TCP/UDP balancer or I should create new balancer based on TCP/UDP code?

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Dec 1, 2019

Is it possible to use existing TCP/UDP balancer or I should create new balancer based on TCP/UDP code?

Not sure. If you can add the upstream information like is being done for TCP service, is fine to me

TCPEndpoints: n.getStreamServices(n.cfg.TCPConfigMapName, apiv1.ProtocolTCP),

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 12, 2019

@dm3ch: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-ingress-nginx-e2e-1-15 cf594c9 link /test pull-ingress-nginx-e2e-1-15
pull-ingress-nginx-e2e-1-12 cf594c9 link /test pull-ingress-nginx-e2e-1-12
pull-ingress-nginx-e2e-1-14 cf594c9 link /test pull-ingress-nginx-e2e-1-14
pull-ingress-nginx-e2e-1-13 cf594c9 link /test pull-ingress-nginx-e2e-1-13
pull-ingress-nginx-e2e-1-16 cf594c9 link /test pull-ingress-nginx-e2e-1-16
pull-ingress-nginx-e2e-1-17 cf594c9 link /test pull-ingress-nginx-e2e-1-17

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.

@aledbf aledbf removed this from In Progress in 0.27.0 Dec 29, 2019
@aledbf aledbf added this to In Progress in 0.28.0 Jan 17, 2020
@aledbf aledbf removed this from In Progress in 0.28.0 Jan 25, 2020
@aledbf aledbf added this to In Progress in 0.29.0 Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
0.29.0
  
In Progress
4 participants
You can’t perform that action at this time.