New ingress controller chart #3140

jschaul · 2023-03-09T14:35:15Z

Main change:

New 'ingress-nginx-controller' wrapper chart compatible with kubernetes versions [1.23 - 1.26]. The old one 'nginx-ingress-controller' (compatible only up to k8s 1.19) is now DEPRECATED.
We advise to upgrade your version of kubernetes in use to 1.23 or higher (we tested on kubernetes version 1.26), and to make use of the new ingress controller chart. Main features:

up-to-date nginx version ('1.21.6')
TLS 1.3 support (including allowing specifying which cipher suites to use)
security fixes
no more accidental logging of Wire access tokens under specific circumstances jira ticket

The 'kind: Ingress' resources installed via 'nginx-ingress-services' chart remain compatible with both the old and the new ingress controller, and k8s versions [1.18 - 1.26]. In case you upgrade an existing kubernetes cluster (not recommended), you may need to first uninstall the old controller before installing the new controller chart.

In case you have custom overrides, you need to modify the directory name and top-level configuration key:

# If you have overrides for the controller chart (such as cipher suites), ensure to rename file and top-level key:
-# nginx-ingress-controller/values.yaml
+# ingress-nginx-controller/values.yaml
-nginx-ingress:
+ingress-nginx:
   controller:
     # ...

and double-check if all overrides you use are indeed provided under the same name by the upstream chart. See also the default overrides in the default values.yaml.

In case you use helmfile change your ingress controller like this:

# helmfile.yaml
releases:
-  - name: 'nginx-ingress-controller'
+  - name: 'ingress-nginx-controller'
     namespace: 'wire'
-    chart: 'wire/nginx-ingress-controller'
+    chart: 'wire/ingress-nginx-controller'
     version: 'CHANGE_ME'

For more information read the documentation under https://docs.wire.com/how-to/install/ingress.html (or go to https://docs.wire.com and search for "ingress-nginx-controller")

Other internal changes:

integration tests on CI will use either the old or the new ingress controller; depending on which kubernetes version they run on.
upgrade kubectl to default from the nixpkgs channel (currently 1.26) by removing the manual version pin on 1.19
upgrade helmfile to default from the nixpkgs channel by removing the manual version pin
upgrade helm to default from the nixpkgs channel by removing the manual version pin
add kubelogin-oidc so the kubectl in this environment can also talk to kubernetes clusters using OIDC

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

supersven

Great job 👍

…it seems)

jschaul · 2023-03-21T15:48:52Z

CI tests now finally pass; both on a 1.19 cluster kube-ci-legacy, and (tested manually) on a 1.26 cluster (new kube-ci). Just keeping this PR hanging for potential feedback by others in the reviewer list.

On 1.26 kube-ci using the new chart wrapper:

===============
=== summary ===
===============
=== tail galley: ===

All 416 tests passed (77.44s)
=== tail cargohold: ===

All 21 tests passed (8.11s)
=== tail gundeck: ===

All 34 tests passed (155.61s)
=== tail spar: ===
Finished in 254.9702 seconds
553 examples, 0 failures, 65 pending
=== tail federator: ===
Finished in 0.4622 seconds
9 examples, 0 failures
=== tail brig: ===

All 491 tests passed (92.45s)
galley-integration passed ✅.
cargohold-integration passed ✅.
gundeck-integration passed ✅.
federator-integration passed ✅.
spar-integration passed ✅.
brig-integration passed ✅.
All integration tests passed ✅.

Following this PR, as well as #3154; I think we could also test using only TLS1.3 and test using ciphers with bigger key sizes.

akshaymankar · 2023-03-21T16:01:25Z

The federator client doesn't support TLS 1.3 yet. It requires implementing a c function in HsOpenSSL.

comawill

Awesome 👍 LGTM

hack/bin/integration-setup-federation.sh

hack/bin/integration-setup.sh

hack/bin/integration-teardown-federation.sh

hack/bin/integration-teardown.sh

Co-authored-by: Sebastian Willenborg <[email protected]>

flokli · 2023-03-22T11:24:10Z

charts/ingress-nginx-controller/values.yaml

+    # -- Use a `DaemonSet` or `Deployment`
+    kind: DaemonSet
+    service:
+      type: NodePort  # or LoadBalancer
+      externalTrafficPolicy: Local
+      nodePorts:
+        # The nginx instance is exposed on ports 31773 (https) and 31772 (http)
+        # on the node on which it runs. You should add a port-forwarding rule
+        # on the node or on the loadbalancer that forwards ports 443 and 80 to
+        # these respective ports.
+        https: 31773
+        http: 31772


A more opportunistic default here would be to use type: Deployment and type: LoadBalancer, and document how to configure this if you have to resort to NodePort.

The NodePort approach always requires manual configuration of some external load balancer/firewall to round-robin between node IPs and is error-prone. It's also a bit annoying to have to decide on some global ports that may not be used otherwise.

Most managed K8s clusters have support for LoadBalancers, you can also get this for your own clusters in hcloud etc. It's even possible to do it for pure bare metal, without any "load balancer hardware", by using BGP or some leadership election over who's announcing the "load balancer ip" via ARP (https://metallb.universe.tf/configuration/_advanced_l2_configuration/).

I swapped the defaults as you suggested and documented how to get the previous behaviour back.

## Main change: New 'ingress-nginx-controller' wrapper chart compatible with kubernetes versions [1.23 - 1.26]. The old one 'nginx-ingress-controller' (compatible only up to k8s 1.19) is now DEPRECATED. We advise to upgrade your version of kubernetes in use to 1.23 or higher (we tested on kubernetes version 1.26), and to make use of the new ingress controller chart. Main features: - up-to-date nginx version ('1.21.6') - TLS 1.3 support (including allowing specifying which cipher suites to use) - security fixes - no more accidental logging of Wire access tokens under specific circumstances [jira ticket]( https://wearezeta.atlassian.net/browse/SEC-47) The 'kind: Ingress' resources installed via 'nginx-ingress-services' chart remain compatible with both the old and the new ingress controller, and k8s versions [1.18 - 1.26]. In case you upgrade an existing kubernetes cluster (not recommended), you may need to first uninstall the old controller before installing the new controller chart. In case you have custom overrides, you need to modify the directory name and top-level configuration key: ```diff # If you have overrides for the controller chart (such as cipher suites), ensure to rename file and top-level key: -# nginx-ingress-controller/values.yaml +# ingress-nginx-controller/values.yaml -nginx-ingress: +ingress-nginx: controller: # ... ``` and double-check if all overrides you use are indeed provided under the same name by the upstream chart. See also the default overrides in [the default values.yaml](https://github.com/wireapp/wire-server/blob/develop/charts/ingress-nginx-controller/values.yaml). In case you use helmfile change your ingress controller like this: ```diff # helmfile.yaml releases: - - name: 'nginx-ingress-controller' + - name: 'ingress-nginx-controller' namespace: 'wire' - chart: 'wire/nginx-ingress-controller' + chart: 'wire/ingress-nginx-controller' version: 'CHANGE_ME' ``` For more information read the documentation under https://docs.wire.com/how-to/install/ingress.html (or go to https://docs.wire.com and search for "ingress-nginx-controller") ## Other internal changes: - integration tests on CI will use either the old or the new ingress controller; depending on which kubernetes version they run on. - upgrade `kubectl` to default from the nixpkgs channel (currently `1.26`) by removing the manual version pin on 1.19 - upgrade `helmfile` to default from the nixpkgs channel by removing the manual version pin - upgrade `helm` to default from the nixpkgs channel by removing the manual version pin - add `kubelogin-oidc` so the kubectl in this environment can also talk to kubernetes clusters using OIDC

flokli · 2023-03-22T15:57:08Z

docs/src/how-to/install/ingress.md

+You should add a port-forwarding rule on the node or on the loadbalancer that
+forwards ports 443 and 80 to these respective ports. Any traffic hitting the http port is simply getting a http 30x redirect to https.
+
+Downsides of this approach: The NodePort approach always requires manual configuration of some external load balancer/firewall to round-robin between node IPs and is error-prone. It's also a bit annoying to have to decide on some global ports that may not be used otherwise.


We now do set some ports for the NodePort approach (31773 and 31772), even though we don't use them as long as the user doesn't explicitly set type: NodePort. We might want to remove them from values.yaml, and explicitly let the user decide (and put it in the example in L63), so this paragraph makes sense.

Good point. #3173

flokli · 2023-03-22T16:03:20Z

docs/src/how-to/install/ingress.md

+
+Downsides of this approach: The NodePort approach always requires manual configuration of some external load balancer/firewall to round-robin between node IPs and is error-prone. It's also a bit annoying to have to decide on some global ports that may not be used otherwise.
+
+Most managed K8s clusters have support for LoadBalancers, you can also get this for your own clusters in hcloud etc. It's even possible to do it for pure bare metal, without any "load balancer hardware", by using BGP or some leadership election over who's announcing the "load balancer ip" via ARP (https://metallb.universe.tf/configuration/_advanced_l2_configuration/).


This is okay for a github discussion, but a bit too sloppy for public docs.

What about

Most managed K8s clusters have support for LoadBalancers.
Manually set up K8S Clusters can also support this, by using a provider/environment-specific CCM
(see hcloud and digitalocean for examples).
In case you're provisioning on bare metal, without any hardware load balancer support in front,
you might be using MetalLB, which supports BGP or Failover L2 ARP announcements.
The choice of CCM highly depends on the environment you choose to deploy wire-server in.

… too (#3138) Fix a rate-limit exemption whereby authenticated endpoints did not get the unlimited_requests_endpoint, if set, applied. This is a concern for the webapp and calls to /assets, which can happen in larger numbers on initial loading. A previous change in [this PR](#2786) had no effect. This PR also increases default rate limits, to compensate for [new ingress controller chart](#3140 default topologyAwareRouting.

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 9, 2023

jschaul force-pushed the new-ingress-controller-chart branch 4 times, most recently from a4020bc to 0340b10 Compare March 16, 2023 14:42

jschaul marked this pull request as ready for review March 16, 2023 15:56

jschaul requested review from supersven, akshaymankar and amitsagtani97 March 16, 2023 16:21

supersven approved these changes Mar 16, 2023

View reviewed changes

jschaul added 20 commits March 21, 2023 13:25

init WIP

91c1cdb

changelog

09dfeb4

more overrrides: log format, TLS 1.3 ciphers

d3e85bf

shorten names of installed helm charts

91bccf2

rename, add to makefile releases

11599e9

also add new chart to list of charts for integration tests

2a4c0e9

move dependencies to Chart.yaml

1def7e5

also download dependencies of helm charts if specified inside Chart.yaml

7c05c58

update comment

f3ab942

overrides

f7238a1

move overrides to correct location

018290e

switch helmfile over to new ingress for testing...

f7693b9

update changelog

a601a4c

add deprecated comment to old chart

744b9aa

try out conditionals in helm chart

ce294c3

add kubernetes version in manually into helmfile

281dea9

...

b4f50b7

fixup

6f6f7df

...

3fe920b

do conditional logic inside bash, not helmfile (as that doesn't work …

4501a33

…it seems)

jschaul added 4 commits March 21, 2023 13:25

helmfile: needs

3838742

don't watch all ingresses, but only those in the right class

f6b8df6

override ingressClass in CI consistently

c92ee42

disable validation webhooks in CI

db3e342

jschaul force-pushed the new-ingress-controller-chart branch from 8b6db95 to db3e342 Compare March 21, 2023 12:25

jschaul added 4 commits March 21, 2023 13:28

also adjust helmfile-single

f8196c0

adjust federation-test-helper service to match new ingress-controller

049ace7

adjust changelog

4d463ef

also add changelog for internal changes

3bb1b20

jschaul requested review from comawill and flokli March 21, 2023 15:35

jschaul mentioned this pull request Mar 21, 2023

old-ingress-stop-logging-query-parameters #3139

Merged

2 tasks

comawill approved these changes Mar 21, 2023

View reviewed changes

hack/bin/integration-setup-federation.sh Outdated Show resolved Hide resolved

hack/bin/integration-setup.sh Outdated Show resolved Hide resolved

hack/bin/integration-teardown-federation.sh Outdated Show resolved Hide resolved

hack/bin/integration-teardown.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

d471714

Co-authored-by: Sebastian Willenborg <[email protected]>

flokli approved these changes Mar 22, 2023

View reviewed changes

jschaul added 4 commits March 22, 2023 14:54

Add docs; switch defaults to Load Balancer as suggested in PR review

96d270f

tweak documentation

9d74921

more docs tweaks

5eba47a

link to docs in changelog

344d6e2

jschaul merged commit 94808d1 into develop Mar 22, 2023

jschaul deleted the new-ingress-controller-chart branch March 22, 2023 14:38

jschaul mentioned this pull request Mar 22, 2023

Ingress tls1.3 [WIP] #1914

Closed

6 tasks

flokli reviewed Mar 22, 2023

View reviewed changes

jschaul mentioned this pull request Mar 22, 2023

[WIP] reword/improve ingress documentation #3173

Closed

2 tasks

zebot mentioned this pull request Apr 17, 2023

Release 2023-04-17 - (expected chart version 4.35.0) #3230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New ingress controller chart #3140

New ingress controller chart #3140

Uh oh!

jschaul commented Mar 9, 2023 •

edited

Loading

Uh oh!

supersven left a comment

Uh oh!

jschaul commented Mar 21, 2023

Uh oh!

akshaymankar commented Mar 21, 2023

Uh oh!

comawill left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flokli Mar 22, 2023

Uh oh!

jschaul Mar 22, 2023

Uh oh!

flokli Mar 22, 2023

Uh oh!

jschaul Mar 22, 2023

Uh oh!

flokli Mar 22, 2023

Uh oh!

jschaul Mar 22, 2023

Uh oh!

Uh oh!


		Downsides of this approach: The NodePort approach always requires manual configuration of some external load balancer/firewall to round-robin between node IPs and is error-prone. It's also a bit annoying to have to decide on some global ports that may not be used otherwise.

		Most managed K8s clusters have support for LoadBalancers, you can also get this for your own clusters in hcloud etc. It's even possible to do it for pure bare metal, without any "load balancer hardware", by using BGP or some leadership election over who's announcing the "load balancer ip" via ARP (https://metallb.universe.tf/configuration/_advanced_l2_configuration/).

New ingress controller chart #3140

New ingress controller chart #3140

Uh oh!

Conversation

jschaul commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main change:

Other internal changes:

Checklist

Uh oh!

supersven left a comment

Choose a reason for hiding this comment

Uh oh!

jschaul commented Mar 21, 2023

Uh oh!

akshaymankar commented Mar 21, 2023

Uh oh!

comawill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flokli Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

flokli Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

flokli Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschaul commented Mar 9, 2023 •

edited

Loading