search

kcp-dev / helm-charts

Helm chart repo for KCP
Apache License 2.0
5 stars 21 forks source link

server-deployment: add .Values.externalPort to customize EXTERNAL_PORT #88

Closed darkmuggle closed 5 months ago

darkmuggle commented 6 months ago

Before this PR, the external port of the kcp server is set to 8443 for a LoadBalancer service type and 443 otherwise. The later is the case where an ingress is expected to be in-front of kcp. But when the actual usecase is to talk to the frontproxy directly via the service, this logic is wrong. Adding .Values.externalPort allows the customize the port, overriding upper logic.

kcp-ci-bot commented 6 months ago

Hi @darkmuggle. Thanks for your PR.

I'm waiting for a kcp-dev 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
embik commented 6 months ago

Hi @darkmuggle, thank you for the contribution! Can you please give some more context on what bug you are seeing and what configuration triggers it?

The original idea here was to use port 443 if an Ingress is used to expose the front-proxy, since then access to it is not available on port 8443. Maybe the if condition here is faulty though and should be checking for that directly (and not for the service type).

sttts commented 6 months ago

@mjudeikis compare https://github.com/kcp-dev/helm-charts/blob/main/charts/kcp/templates/front-proxy-deployment.yaml#L18. The frontproxy service uses also 8443, not 443. So to make the service work as entry-point the deployment must use 8443 also in non-LB mode.

sttts commented 6 months ago

Conversely, we could change the service's port.

embik commented 6 months ago

@mjudeikis compare https://github.com/kcp-dev/helm-charts/blob/main/charts/kcp/templates/front-proxy-deployment.yaml#L18. The frontproxy service uses also 8443, not 443. So to make the service work as entry-point the deployment must use 8443 also in non-LB mode.

But that is confusing to me because the EXTERNAL_HOSTNAME variable is set to Helm values' .externalHostname. That should be the external DNS name (so kcp.beckers.dev) usually and not the kcp-front-proxy.kcp.svc.cluster.local service DNS name.

For me there are two deployment scenarios:

  1. You use the LoadBalancer service type. Then I get an LB from e.g. AWS, I set a CNAME on kcp.beckers.dev and my kcp front proxy endpoint is available as kcp.beckers.dev:8443.
  2. You expose via Ingress or OpenShift Route. Then traffic flows through the ingress controller via HTTPS, which usually listens on port 443. So my kcp front proxy endpoint is available as kcp.beckers.dev:443.

As far as I understand, the point made is that there is a third option, where you don't expose kcp at all and just make it available via its front-proxy Service endpoint within the cluster. Is that correct? If so, we need to refine this PR because it will break scenario (1).

To verify my understanding: You are setting .externalHostname=kcp-front-proxy.kcp.svc.cluster.local?

sttts commented 6 months ago

Right, kcp is installed in-cluster and only accessed from there.

To verify my understanding: You are setting .externalHostname=kcp-front-proxy.kcp.svc.cluster.local?

yes

mjudeikis commented 6 months ago

Shouldn't this be "templatable destination port on service object:?

targetPort: "{{ if eq .Values.kcpFrontProxy.service.type "LoadBalancer" }}8443{{ else }}443{{- end }}"

? I think @embik is right, and this will break case he mentions.

embik commented 5 months ago

/ok-to-test

kcp-ci-bot commented 5 months ago

LGTM label has been added.

Git tree hash: 22e54a6a93e891f7e1eb51e75f73db0102d87fde

kcp-ci-bot commented 5 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kcp-dev/helm-charts/blob/main/OWNERS)~~ [embik] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment