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

[charts/pulsar] adding flag webSocketServiceEnabled cm #653

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yuweisung
Copy link
Contributor

Motivation

  • According to the doc, proxy supports websocket but the broker and proxy cm template have no option to turn it on.*

Modifications

The proxy-configmap.yaml and broker-configmap.yaml are modified to enable websocket services.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

Documentation

Check the box below.

Need to update docs?

  • [ x ] no-need-doc

    (Please explain why)

@yuweisung yuweisung requested review from a team, sijie and addisonj as code owners May 3, 2022 20:48
@github-actions github-actions bot added the no-need-doc This pr does not need any document label May 3, 2022
@@ -42,7 +42,9 @@ data:
numHttpServerThreads: "8"
zooKeeperSessionTimeoutMillis: "30000"
statusFilePath: "{{ template "pulsar.home" . }}/status"

{{- if .Values.proxy.websocket.enabled }}
webSocketServiceEnnabled: "true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
webSocketServiceEnnabled: "true"
webSocketServiceEnabled: "true"

@ericsyh
Copy link
Member

ericsyh commented May 6, 2022

@fantapsody I think there is no need to enable ws on broker, https://github.com/streamnative/charts/blob/master/charts/pulsar/values.yaml#L1291 allows to enable websocket on proxy.

@polyzos
Copy link

polyzos commented May 6, 2022

@ericsyh what happens when we use istio and the proxy is disabled?

@ericsyh ericsyh changed the title [charts/pulsaradding flag webSocketServiceEnabled cm [charts/pulsar] adding flag webSocketServiceEnabled cm May 6, 2022
@ericsyh
Copy link
Member

ericsyh commented May 7, 2022

@ericsyh what happens when we use istio and the proxy is disabled?

@polyzos You mean the scenario that enabled Istio but wanna to use the websocket right? I think it's quite a good point, the proxy now has to been disabled cause there is a bug in the proxy that it cannot forward HTTP requests correctly when mTLS is on. If this problem is fixed, proxy can work with Istio.

So for the websocket, if currently no specific request for using websocket with Istio enabled, i prefer waiting for the proxy bug fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge no-need-doc This pr does not need any document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants