[installer]: grant option for disabling proxy load balancer #7280

mrsimonemms · 2021-12-17T08:38:51Z

Description

This will allow users to create their own ingress controller. An example for NGINX Ingress is shown.

The thinking behind this is due to the number of corporate users who have said that using ServiceType: LoadBalancer is not possible for them. If the ingress is disabled, the ServiceType: ClusterIP is used and the expectation is that the user will configure their own ingress controller

An example for NGINX Ingress is provided (in a gist)

This introduced the meta config object (similar to workspace) and ingress.serviceType, which defaults to LoadBalancer

How to test

Create a deployment and set the config:

components:
  proxy:
    serviceType: ClusterIP

Deploy your installation of Gitpod

Install ingress-nginx:

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/docs/examples/psp/psp.yaml
helm upgrade \
    --atomic \
    --cleanup-on-fail \
    --create-namespace \
    --install \
    --namespace ingress-nginx \
    --reset-values \
    --repo https://kubernetes.github.io/ingress-nginx \
    --set defaultBackend.enabled=false \
    --set controller.extraArgs.enable-ssl-passthrough=true \
    --wait \
    ingress-nginx \
    ingress-nginx

Create your ingress controller:

# Replace $DOMAIN with your domain

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: gitpod
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/ssl-passthrough: "true"
spec:
  tls:
    - hosts:
        - "$DOMAIN"
        - "*.$DOMAIN"
        - "*.ws.$DOMAIN"
      secretName: https-certificates
  rules:
    - host: "$DOMAIN"
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: proxy
                port:
                  number: 443
    - host: "*.$DOMAIN"
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: proxy
                port:
                  number: 443
    - host: "*.ws.$DOMAIN"
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: proxy
                port:
                  number: 443

Release Notes

[installer]: grant option for disabling proxy load balancer

Documentation

roboquat · 2021-12-17T08:38:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from mrsimonemms and additionally assign jankeromnes after the PR has been reviewed.
You can assign the PR to them by writing /assign @jankeromnes in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these files:

installer/OWNERS
installer/pkg/components/proxy/OWNERS

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

mrsimonemms · 2021-12-17T08:41:01Z

@csweichel @corneliusludmann would be interested in your thoughts on this. We definitely need to do SOMETHING, but open to suggestions if this is the correct something.

I've put it on hold as want to get community feedback if this will work across the board

mrsimonemms · 2021-12-17T08:41:17Z

/hold

codecov · 2021-12-17T08:50:26Z

Codecov Report

Merging #7280 (091ec0a) into main (55549e8) will decrease coverage by 1.67%.
The diff coverage is n/a.

❗ Current head 091ec0a differs from pull request most recent head ff69a02. Consider uploading reports for the commit ff69a02 to get more accurate results

@@           Coverage Diff            @@
##            main   #7280      +/-   ##
========================================
- Coverage   7.44%   5.76%   -1.68%     
========================================
  Files         15      13       -2     
  Lines       1330    1162     -168     
========================================
- Hits          99      67      -32     
+ Misses      1228    1094     -134     
+ Partials       3       1       -2

Flag	Coverage Δ
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`
installer-raw-app	`5.76% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

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 55549e8...ff69a02. Read the comment docs.

mrsimonemms · 2021-12-17T11:13:12Z

installer/README.md

+```
+
+This will set the `ServiceType` to `ClusterIP` allowing you to create your own
+ingress controller - click [here](https://gist.github.com/MrSimonEmms/480e88c149f6e725c727cc1f20b21474)


Do we have a better, GP-owned place to put this?

How about somewhere in this repo under the contrib/ folder?

That's a good idea

aledbf · 2021-12-17T11:23:46Z

meta:
ingress:
enabled: false

@mrsimonemms this config gives the impression that Gitpod uses Ingress, and that's not the case.
Maybe use serviceType: ClusterIP, saying the default is LoadBalancer?

meta:
  ingress:
    serviceType: ClusterIP

mrsimonemms · 2021-12-17T11:56:19Z

I've done the change @aledbf suggested. This will require some additional validation to ensure that the serviceType is acceptable - I'll do that if we agree that this is a change we want to make

I can also see the potential preference of setting it as meta.proxy.serviceType - open to suggestions on that one too

iQQBot · 2021-12-19T17:46:12Z

installer/README.md

+```yaml
+meta:
+  ingress:
+    enabled: false


Is there doc out of date?

csweichel · 2021-12-20T07:30:45Z

installer/README.md

+your Installer configuration:
+
+```yaml
+meta:


Unlike workspace - which refers workspaces rather than the team - the term meta does not make sense from a user's perspective.

Fair point. Have you got a preferred name?

csweichel · 2021-12-20T07:32:50Z

installer/README.md

+```yaml
+meta:
+  ingress:
+    enabled: false


We should make the behaviour of this flag more obvious in its naming, e.g. directly refer to the type of the proxy service.

appcoders · 2021-12-21T07:18:32Z

@mrsimonemms
Could be something wrong with the ingress?
I tried your change but it leads to this #7324

appcoders · 2021-12-21T10:13:41Z

Ingress can be ruled out, it is a imagebuild issue.

{"@type":"type.googleapis.com/google.devtools.clouderrorreporting.v1beta1.ReportedErrorEvent","error":"fork/exec /proc/self/exe: operation not permitted","level":"error","message":"failed to start ring0","ring":0,"serviceContext":{"service":"workspacekit","version":"commit-41879d65678692b46eac760932c3bb4b8c8714a2"},"severity":"ERROR","time":"2021-12-21T10:08:08Z"}

appcoders · 2021-12-21T12:22:32Z

With the right distribution (Ubuntu 20.04 instead of Debian 10) everything works. To get the ingress working I added the following annotations:

annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/ssl-passthrough: "true"
    nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
    nginx.ingress.kubernetes.io/ssl-redirect: "true"
    nginx.ingress.kubernetes.io/proxy-body-size: 1024m

I am not quite sure how big the proxy-body-size must be set, but 1G works. So, this could be used as a starting point.

aledbf · 2021-12-21T14:20:27Z

@mrsimonemms please do not include references or examples to Ingress configurations.
By doing that, you implicitly are saying it is a supported configuration.

The main reason is related to the side effects of reload in ingress controllers like ingress-nginx and the impact on workspaces and reconnections.

aledbf · 2021-12-21T14:23:29Z

@appcoders if you configure nginx.ingress.kubernetes.io/ssl-passthrough: "true" the annotation
nginx.ingress.kubernetes.io/backend-protocol: "HTTPS" has no effect because there is no NGINX upstream, only a connection pipe

aledbf · 2021-12-21T14:30:25Z

installer/README.md

+```yaml
+components:
+  proxy:
+    serviceType: ClusterIP


Maybe add a validation checking that serviceType is LoadBalancer or ClusterIP only?
Both, proxy and ws-proxy are configured on ports 80 and 443

appcoders · 2021-12-21T19:02:55Z

@aledbf You are right. This was a leftover from previous tests...

corneliusludmann · 2021-12-21T20:19:13Z

Thanks, @mrsimonemms for opening this pull request. I am currently trying to better understand the use case as well as the implications/risks for this change.

Use Case

Currently, we create a service for the proxy of type LoadBalancer. With this change, the users have the possibility to change the proxy service to be of type ClusterIP. With that, you can create your own ingress controller (e.g. nginx, Traefik, etc.) that routes the traffic to the proxy service. This allows deploying Gitpod next to other apps with a single IP where the routing is based on the domain, path, etc.

Limitations

The proxy still needs to have SSL certs. With this change, you still cannot move the SSL handling/termination to the ingress controller, right?

Implications

Introducing this option in the installer indicates also that this is a valid use case we fully support somehow. That's why it's important to understand the implications and risks of this change. Or in other words, is the use of an ingress controller something we can support with confidence without regular testing? Or should we expect that there will be problems with it from time to time as users try it out and find cases that we haven't considered/tested? (Which ultimately leads to more variance that we need to test and support.)

@aledbf's comment above indicates that using a custom ingress controller could bring side-effect:

The main reason is related to the side effects of reload in ingress controllers like ingress-nginx and the impact on workspaces and reconnections.

This means that we would probably have to test the case thoroughly with different ingress controllers if we want to officially support this.

What's next?

Surely there are or will be users who have a need for this use case. However, at the moment there seems to be no one we know for sure (especially with the limitation mentioned above), right?

This pull request is, at its core, a small change in the load balancer that allows changing the type from LoadBalancer to ClusterIP. For me, at this point, it's not 100% clear if it is a use case that we can and want fully support yet.

I don't want to be a wet blanket. But especially because we can re-implement this change pretty quickly once we have the actual need from a customer for such a feature, I would vote for putting this pull request on hold for now. When we close this pull request now, we still have the code snippets available here once we decide that we need to implement (and test) this.

And if people from the community want to experiment with ingress controllers, they can still patch the output from the installer after the rendering pretty easily to change the service type of their installation.

What do you think?

I'm happy enough saying to people that post-processing the YAML is the recommended solution, but if we do that then we need to have better documentation on this - I would suggest as well that we would need to then have a "post-processable" output for the render function. This might mean that the output would be an array of objects with the apiVersion, kind, name and contents (ie, outputting the slice not the formatted data in here). Expecting users to post-process may not be popular thought (I don't know, just a word of caution).

I know that the single-hosted instance would still require a cert at this stage - in my mind, that's a separate issue which, whilst related, I would like to do work on improving in the new year

Holding whilst we get different use-cases is a legitimate option.

@community - please add your use-cases down below I guess

This will allow users to create their own ingress controller. An example for NGINX Ingress is shown

appcoders · 2021-12-22T14:48:28Z

After having issues bringing up the workspace at all, after removing the annotation
nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
I added the annotation again and everything works again.

aledbf · 2021-12-22T15:19:56Z

I added the annotation again and everything works again.

Do you have the flag --enable-ssl-passthrough in the ingress-nginx deployment?
https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#ssl-passthrough

If not, it means nginx.ingress.kubernetes.io/ssl-passthrough: "true" is not going to do anything.

appcoders · 2021-12-23T08:11:18Z

@aledbf This is how my ingress-nginx is installed:

helm upgrade --install ingress-nginx ingress-nginx \
  --repo https://kubernetes.github.io/ingress-nginx \
  --namespace ingress-nginx --create-namespace \
  --set defaultBackend.enabled=false \
  --set controller.extraArgs.enable-ssl-passthrough=true \
  --reset-values \
  --wait

mrsimonemms · 2021-12-26T20:15:50Z

Replaced with #7364

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Dec 17, 2021

roboquat added team: webapp Issue belongs to the WebApp team team: delivery Issue belongs to the self-hosted team size/M labels Dec 17, 2021

mrsimonemms force-pushed the sje/installer-custom-ingress branch from 0b67612 to 5c7fe79 Compare December 17, 2021 08:39

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Dec 17, 2021

mrsimonemms marked this pull request as ready for review December 17, 2021 08:41

roboquat removed the do-not-merge/work-in-progress label Dec 17, 2021

roboquat added the do-not-merge/hold label Dec 17, 2021

mrsimonemms requested review from a team December 17, 2021 08:50

mrsimonemms commented Dec 17, 2021

View reviewed changes

mrsimonemms force-pushed the sje/installer-custom-ingress branch from 5c7fe79 to 091ec0a Compare December 17, 2021 11:55

iQQBot reviewed Dec 19, 2021

View reviewed changes

csweichel reviewed Dec 20, 2021

View reviewed changes

mrsimonemms force-pushed the sje/installer-custom-ingress branch from 091ec0a to ff69a02 Compare December 20, 2021 14:50

appcoders mentioned this pull request Dec 21, 2021

[Installer] GitPod workspace does not come up with 'Error: image build did not produce a workspace image' #7324

Closed

mrsimonemms added the installer: needs interface change Change required to input, output or configuration file(s) label Dec 21, 2021

aledbf reviewed Dec 21, 2021

View reviewed changes

mrsimonemms force-pushed the sje/installer-custom-ingress branch from ff69a02 to a064784 Compare December 22, 2021 14:31

mrsimonemms marked this pull request as draft December 22, 2021 14:33

roboquat added the do-not-merge/work-in-progress label Dec 22, 2021

[installer]: grant option for disabling proxy load balancer

762b350

This will allow users to create their own ingress controller. An example for NGINX Ingress is shown

mrsimonemms force-pushed the sje/installer-custom-ingress branch from a064784 to 762b350 Compare December 22, 2021 14:35

appcoders mentioned this pull request Dec 23, 2021

Bugs when open gitpod workspaces #7343

Closed

mrsimonemms mentioned this pull request Dec 26, 2021

[installer]: add example for post-processing the YAML #7364

Merged

mrsimonemms closed this Dec 26, 2021

mrsimonemms deleted the sje/installer-custom-ingress branch December 26, 2021 20:15

corneliusludmann mentioned this pull request Jan 21, 2022

[installer] Add support for Ingress traffic in cloud providers #7734

Closed

nandajavarma mentioned this pull request Mar 14, 2022

[installer] set the ServiceType as LB for ws-proxy #8759

Merged

[installer]: grant option for disabling proxy load balancer #7280

[installer]: grant option for disabling proxy load balancer #7280

Uh oh!

Conversation

mrsimonemms commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test

Release Notes

Documentation

Uh oh!

roboquat commented Dec 17, 2021

Uh oh!

mrsimonemms commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrsimonemms commented Dec 17, 2021

Uh oh!

codecov bot commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aledbf commented Dec 17, 2021

Uh oh!

mrsimonemms commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

appcoders commented Dec 21, 2021

Uh oh!

appcoders commented Dec 21, 2021 • edited by mrsimonemms Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

appcoders commented Dec 21, 2021

Uh oh!

aledbf commented Dec 21, 2021

Uh oh!

aledbf commented Dec 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

appcoders commented Dec 21, 2021

Uh oh!

corneliusludmann commented Dec 21, 2021

Use Case

Limitations

Implications

What's next?

See also

Uh oh!

appcoders commented Dec 22, 2021

Uh oh!

mrsimonemms commented Dec 22, 2021

Uh oh!

appcoders commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aledbf commented Dec 22, 2021

Uh oh!

appcoders commented Dec 23, 2021

Uh oh!

mrsimonemms commented Dec 26, 2021

Uh oh!

Uh oh!

mrsimonemms commented Dec 17, 2021 •

edited

Loading

mrsimonemms commented Dec 17, 2021 •

edited

Loading

codecov bot commented Dec 17, 2021 •

edited

Loading

mrsimonemms commented Dec 17, 2021 •

edited

Loading

appcoders commented Dec 21, 2021 •

edited by mrsimonemms

Loading

appcoders commented Dec 22, 2021 •

edited

Loading