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.

Sign up for GitHub

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

Jump to bottom

Add bridge port to Pod and Service manifests in Helm Chart #802

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

Add bridge port to Pod and Service manifests in Helm Chart #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/802.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add bridge ports to Pod and Service manifests in Helm Chart
3 changes: 3 additions & 0 deletions helm/hookshot/templates/_pod.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ containers:
mountPath: "/data"
{{- end }}
ports:
- name: bridge
containerPort: {{ .Values.hookshot.config.bridge.port }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Heya, do you know what the bridge port corresponds to. This isn't the appservice (below)?

Copy link
Author

Choose a reason for hiding this comment

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

Originally I thouth so, but I'm not sure, TBH. There is a port 9993 defined in the bridge configuration section. But in the service and container definitions are used hardcoded ports which does not correspond with bridge port. 🤷‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

So I've been digging more and the ports are really different. The appservice here is probably serving widget stuff, but the bridge itself is running on port defined in .Values.hookshot.config.bridge.port. Therefore, there must be another service for it.
Here is what ports are actually listening in the container:
image

And Here in the values.yaml, there is appservice as listener.
So I think this PR is legit. 🤔 At least I couldn't make the hookshot work without these changes.

Copy link
Author

Choose a reason for hiding this comment

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

Hey, It's been a while. Did you manage to get into this? I want to use the Helm chart installation, but I cannot, since the bridge port is not propagated on Pod or even available in the Service.

If I am doing something wrong, I would like to know what 🤷‍♂️
Thanks

Copy link

@rhizoet rhizoet Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, can this please merged? We need to have Port 9993 opened in the cluster, so that we can reach it with the registration of synapse. Otherweise the helm chart ist useless.

protocol: TCP
- name: webhook
containerPort: 9000
protocol: TCP
Expand Down
4 changes: 4 additions & 0 deletions helm/hookshot/templates/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ metadata:
spec:
type: {{ .Values.service.type }}
ports:
- port: {{ .Values.hookshot.config.bridge.port }}
targetPort: bridge
protocol: TCP
name: bridge
- port: {{ .Values.service.webhook.port }}
targetPort: webhook
protocol: TCP
Expand Down