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

Fix tls ports for FunctionWorkers and Zookeeper #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dave2wave
Copy link

@dave2wave dave2wave commented Jul 28, 2023

Haxx tests discovered issues running function workers with the kaap operator and also some issues have started to appear with running pulsar 3.0 and master tests using the kaap operator and tls.

This is a naive attempt to fix the issue, but it is not tested.

@@ -299,7 +299,7 @@ public void patchConfigMap() {
"pulsarRootDir", "/pulsar",
"submittingInsidePod", true,
"pulsarServiceUrl", brokerServiceUrl,
"pulsarAdminUrl", "https://%s.%s:6750/"
"pulsarAdminUrl", "https://%s.%s:6751/"
Copy link
Member

Choose a reason for hiding this comment

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

Given that line 301 has brokerServiceUrl, I think we might need to dynamically compute this value.

Copy link
Author

Choose a reason for hiding this comment

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

It seems to me that are a lot of little things to do in this class like making use of constants for the ports provided as ints at the top, but likely better as strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it has to be computed based on tls configuration

@dave2wave
Copy link
Author

Some tests are broken.

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

The value must be computed dinamically

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants