Conversation
|
@tuteng:Thanks for your contribution. For this PR, do we need to update docs? |
There was a problem hiding this comment.
Pull request overview
This PR updates the Toolset chart templates so that when Toolset is configured to use the proxy, it can target the proxy “ingress” service (when proxy ingress is enabled) instead of always using the proxy headless service, improving compatibility for pulsar-admin usage from within the Toolset pod.
Changes:
- Add a new Helm helper (
toolset.proxy.service.host) to choose the proxy service hostname (-ingressvs-headless) based on proxy ingress settings. - Update Toolset configmap proxy URLs (
webServiceUrl,brokerServiceUrl) to use the new helper. - Update
toolset.web.service.url(used bypulsarctl.config) to use the new helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| charts/sn-platform/templates/toolset/toolset-configmap.yaml | Switches proxy connection URLs to use toolset.proxy.service.host. |
| charts/sn-platform/templates/toolset/_toolset.tpl | Introduces toolset.proxy.service.host and updates toolset service URL helper to use it. |
| charts/sn-platform-slim/templates/toolset/toolset-configmap.yaml | Same change as sn-platform: proxy URLs now use toolset.proxy.service.host. |
| charts/sn-platform-slim/templates/toolset/_toolset.tpl | Same change as sn-platform: adds helper and updates toolset service URL helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- else -}} | ||
| {{- if and .Values.tls.enabled .Values.tls.proxy.enabled -}} | ||
| https://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}:{{ .Values.proxy.ports.https }} | ||
| https://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.https }} | ||
| {{- else -}} | ||
| http://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}:{{ .Values.proxy.ports.http }} | ||
| http://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.http }} | ||
| {{- end -}} |
There was a problem hiding this comment.
toolset.web.service.url switches the host to the -ingress service when ingress.proxy.enabled, but the scheme/port decision is still based only on .Values.tls.enabled && .Values.tls.proxy.enabled. If ingress.proxy.tls.enabled is true (default) while proxy TLS is disabled (TLS termination at the LB), this will render an http://...:{{ .Values.proxy.ports.http }} URL that likely won’t match the ports/protocol exposed by the ingress service (typically 443/6651). Consider basing the protocol/ports on the ingress service settings when toolset.proxy.service.host resolves to -ingress (e.g., treat ingress.proxy.tls.enabled as TLS for toolset URLs, and honor ingress.proxy.httpPortOverride if set).
| @@ -41,8 +41,8 @@ data: | |||
| tlsEnableHostnameVerification: "false" | |||
| {{- end }} | |||
| {{- if not (and .Values.tls.enabled .Values.tls.proxy.enabled) }} | |||
| webServiceUrl: "http://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}-headless:{{ .Values.proxy.ports.http }}/" | |||
| brokerServiceUrl: "pulsar://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}-headless:{{ .Values.proxy.ports.pulsar }}/" | |||
| webServiceUrl: "http://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.http }}/" | |||
| brokerServiceUrl: "pulsar://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.pulsar }}/" | |||
There was a problem hiding this comment.
These proxy URLs now always use toolset.proxy.service.host (which may be the -ingress service), but the URL scheme/ports are still keyed only off .Values.tls.enabled && .Values.tls.proxy.enabled. With ingress TLS termination (ingress.proxy.tls.enabled: true while proxy TLS is disabled), this can produce http://...:{{ .Values.proxy.ports.http }} / pulsar://...:{{ .Values.proxy.ports.pulsar }} that don’t match what the ingress service exposes. Consider aligning the scheme/ports with ingress settings when targeting the ingress service (and honoring ingress.proxy.httpPortOverride if set).
| {{- else -}} | ||
| {{- if and .Values.tls.enabled .Values.tls.proxy.enabled -}} | ||
| https://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}:{{ .Values.proxy.ports.https }} | ||
| https://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.https }} | ||
| {{- else -}} | ||
| http://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}:{{ .Values.proxy.ports.http }} | ||
| http://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.http }} | ||
| {{- end -}} |
There was a problem hiding this comment.
toolset.web.service.url uses the -ingress service name when ingress.proxy.enabled, but still chooses http vs https solely from .Values.tls.enabled && .Values.tls.proxy.enabled. If ingress is terminating TLS (ingress.proxy.tls.enabled true) while proxy TLS is disabled, toolset will be configured to use http://...:{{ .Values.proxy.ports.http }} even though the ingress service is typically exposing TLS ports. Consider factoring ingress.proxy.tls.enabled (and ingress.proxy.httpPortOverride) into the generated URL when toolset.proxy.service.host resolves to -ingress.
| @@ -41,8 +41,8 @@ data: | |||
| tlsEnableHostnameVerification: "false" | |||
| {{- end }} | |||
| {{- if not (and .Values.tls.enabled .Values.tls.proxy.enabled) }} | |||
| webServiceUrl: "http://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}-headless:{{ .Values.proxy.ports.http }}/" | |||
| brokerServiceUrl: "pulsar://{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}-headless:{{ .Values.proxy.ports.pulsar }}/" | |||
| webServiceUrl: "http://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.http }}/" | |||
| brokerServiceUrl: "pulsar://{{ template "toolset.proxy.service.host" . }}:{{ .Values.proxy.ports.pulsar }}/" | |||
There was a problem hiding this comment.
The generated proxy webServiceUrl/brokerServiceUrl now target toolset.proxy.service.host (ingress service when enabled), but the protocol/ports are still derived only from .Values.tls.enabled && .Values.tls.proxy.enabled. With ingress TLS termination (ingress.proxy.tls.enabled: true while proxy TLS is disabled), this likely produces http://...:{{ .Values.proxy.ports.http }} / pulsar://...:{{ .Values.proxy.ports.pulsar }} that won’t match the ingress service’s exposed ports. Consider generating the scheme/ports based on ingress settings when using the ingress service, and honoring ingress.proxy.httpPortOverride if set.
(If this PR fixes a github issue, please add
Fixes #<xyz>.)Fixes #
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>to link to the master issue.)Master Issue: #
Motivation
Explain here the context, and why you're making that change. What is the problem you're trying to solve.
Modifications
Describe the modifications you've done.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation
Check the box below.
Need to update docs?
doc-required(If you need help on updating docs, create a doc issue)
no-need-doc(Please explain why)
doc(If this PR contains doc changes)