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

expose admin port of pulsar manager in service #440

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

Mortom123
Copy link
Contributor

See #438 / #438 (comment) 2.PR

Motivation

exposing Admin Port of Manager is needed in the future

Modifications

expose Admin Port of Manager

Verifying this change

@lhotari lhotari merged commit 8cd3a04 into apache:master Jan 25, 2024
27 checks passed
@lhotari
Copy link
Member

lhotari commented Jan 25, 2024

I already merged this, but I do have a concern. Since the admin port was added to the existing service that is a load balancer, I guess this expands the attack surface since admin operations are now accessible on the load balancer. @Mortom123 any thoughts on that?

@Mortom123
Copy link
Contributor Author

@lhotari Mhh. You might be right. However I think the admij port gets deactivated / does not listen anymore once the account for the manager is created. But we could also make a second Node Port service I guess?

@lhotari
Copy link
Member

lhotari commented Jan 25, 2024

@lhotari Mhh. You might be right. However I think the admij port gets deactivated / does not listen anymore once the account for the manager is created. But we could also make a second Node Port service I guess?

Yes I think it's better to reduce to possible exposure. One fact is that the Pulsar Helm Chart isn't following a "secure-by-default" principle currently. Anyhow we should attempt to improve the situation. I created #444 to cover other services, but I think that this admin port case could be handled outside of that.

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.

2 participants