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

Use K8 ingress for TLS termination #1443

merged 13 commits into from Feb 8, 2020

Use K8 ingress for TLS termination #1443

merged 13 commits into from Feb 8, 2020


Copy link

gdbelvin commented Jan 28, 2020

Use K8 ingress for TLS termination

Kubernetes ingress objects support

  • Managed TLS Certificates
  • Central monitoring and metrics
  • Static IP address, which allows DNS setup for

The backend protocol from the ingress reverse proxy to the binaries use HTTP/2 + TLS

This PR uses kustomize to configure k8 configs for both GCE and baremetal deployments. The baremetal deployment is used for local testing and the kubernetes test in travis. Baremetal deployments must locally supply an ingress implementation, this PR uses nginx. The GCE deployment uses the cloud provided GCE ingress implementation.

I chose 'kustomizeoverhelm` because kustomize has better support for multiple deployment environments. We can revisit this decision later if needed.

The directory structure:

├── kubernetes
│   ├── base
│   └── overlays
│       ├── gke // Google Cloud
│       └── local // Baremetal for local and travis tests
│           └── ingress-nginx
│               ├── baremetal // nginx configs required for non-cloud deployments
│               ├── cloud-generic // Basic nginx config required for most deployments
│               └── static  // Basic nginx config required for all nginx deployments

To verify that this PR worked:

$ ./scripts/
$ curl -w '%{http_version}\n'
$ go install
$ grpcurl list

Fixes #1396

@googlebot googlebot added the cla: yes label Jan 28, 2020

This comment has been minimized.

Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #1443 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1443      +/-   ##
+ Coverage   66.39%   66.41%   +0.02%     
  Files          54       54              
  Lines        4026     4026              
+ Hits         2673     2674       +1     
  Misses        960      960              
+ Partials      393      392       -1
Impacted Files Coverage Δ
core/sequencer/server.go 73.61% <0%> (-0.33%) ⬇️
core/sequencer/trillian_client.go 61.42% <0%> (+2.85%) ⬆️

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 112863f...bd3af4e. Read the comment docs.

@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch 7 times, most recently from 9eb8c83 to ffeb5e8 Jan 28, 2020
The simplest setup ingress is to only proxy HTTP2 traffic.
Multiplexing gRPC and HTTP is possible, but it requires two ingress
objects and explicit path / gRPC service specific forwarding rules.

- Supply a default backend to prevent the ingress controller from
  creating it's own
- Supply path routes. Without path routing NGINX won't use our TLS certs
  and will supply it's own "default" TLS cert.
@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch 3 times, most recently from 6c9502f to bdedd90 Jan 28, 2020
gdbelvin added 7 commits Jan 21, 2020
- Link to a static IP resource.
- Disable HTTP to slightly simplify the firewall rules.
- Set the backend protocol to HTTP2.
  - Requiries an HTTP2 healthcheck at '/'.
  - Requires TLS (HTTP2 + TLS is incompatible with cmux).
  - Supports GRPC.

This makes debugging slightly easier since NodePorts will be stable
Using kustomize is required because `kubectl -k` doesn't support
directories to as resources in kustomize file.
This makes the ingress object routable by Kubernetes in Docker (KIND)

The current kind config routes localhost traffic on 443 to specific node ports (80443)
@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch from bdedd90 to 33c4833 Feb 3, 2020
@gdbelvin gdbelvin marked this pull request as ready for review Feb 4, 2020
@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Feb 4, 2020
@gdbelvin gdbelvin requested review from AlCutter and NatalieDoduc Feb 4, 2020

This comment has been minimized.

Copy link
Collaborator Author

gdbelvin commented Feb 4, 2020

After researching Helm, I've decided to stick with Kustomize for the following reasons

  1. Helm supports environment specific customization through Go Templating. This makes small changes possible, but larger changes quickly become convoluted.
  2. I couldn't see a way to optionally depend on subcharts in an environment specific manner.
  • To support local testing, we need bare metal features that cloud service providers typically provide.
  1. The more complex uses cases for setting up let's encrypt certs can be managed by relying on GCE. This reduces the effectiveness of local testing, but the local testing story probably deserves a re-think.
Copy link

NatalieDoduc left a comment

For your main PR description, can you make the first line more descriptive? These guidelines are really useful:

Could you also add in the PR description more details about the additional files and structure under ingress-nginx?

Also, i'd recommend if you would explain the use of Kustomize (+investigation into Helm results in the PR description itself, rather than inline in the comments).

Finally if this adds a mechanism to route from outside to our cluster, can you add sample commands or reference to how one might validate that the setup works?

* master:
  Portable docker images cleanup script (#1445)
@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch from e79fb4e to ad0aae0 Feb 5, 2020
gdbelvin added 2 commits Feb 5, 2020
Also deletes unreferenced yaml configs.
@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch from e8c3a63 to 383b3c4 Feb 5, 2020

This comment has been minimized.

Copy link
Collaborator Author

gdbelvin commented Feb 5, 2020

Thanks for your suggestions @NatalieDoduc

Please see

  • Updated
    • Dir structure
    • Testing instructions
  • A for the ingress-nginx directory
@NatalieDoduc NatalieDoduc removed their assignment Feb 6, 2020
Copy link

NatalieDoduc left a comment

LGTM - can you just update the PR title to match your first line of the description.

Also, a few questions below, but only for clarification, no changes required. Thanks!

@gdbelvin gdbelvin changed the title GCE Ingress Use K8 ingress for TLS termination Feb 6, 2020
@gdbelvin gdbelvin force-pushed the gdbelvin:lb2 branch from 383b3c4 to bd3af4e Feb 8, 2020
@gdbelvin gdbelvin merged commit 57ba06e into google:master Feb 8, 2020
5 checks passed
5 checks passed
GolangCI No issues found!
Travis CI - Pull Request Build Passed
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing 112863f...bd3af4e
codecov/project 66.41% (+0.02%) compared to 112863f
@gdbelvin gdbelvin deleted the gdbelvin:lb2 branch Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.