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 the broker stats with different environments #570

Merged
merged 4 commits into from
May 6, 2024

Conversation

bpereto
Copy link
Contributor

@bpereto bpereto commented May 6, 2024

Motivation

the current state of the master branch does not work for broker stats (they are empty) and collecting is failing with exceptions.
Probably because of the implementation how the BrokerStatsImp finds the broker for a specified cluster.

Currently, there are multiple problems:

  • If the Service URL differs from the broker urls, then the corresponding token for authentification is not found. ex: service url https://brokers.pulsar.internal:8443 found brokers: http://broker01.pulsar.internal:8080 even the brokers are configured with tls. the environment token is then not found for broker01, it matches only brokers.pulsar.internal.
  • enabling TLS with tls.enabled enables not only TLS-Transport-Encryption, it configures TLS Client Auth, which is not in all scenarios needed or wanted. I would like to have TLS Cmmunication and Token-Auth.
  • Broker URL Schema is not handled well - addition of http:// or https:// is scattered.

the changes contained in this pull request should sufficiently fix broker metric collection for TLS+TokenAuth (not TLS Client Auth) environments.

Modifications

  • allow specification of environment at broker stats collection to get the token
  • build the broker url with schema, port from specified service url. this enables the use of TLS.
  • specify environment and environment token when collecting stats with collectStatsToDB
  • fix axios parameter serialization which default behaviour was changed in 1.0
  • updated hikari parameters for attempt to limit pool exhaustion exceptions

Verifying this change

  • Make sure that the change passes the ./gradlew build checks.

@lhotari
Copy link
Member

lhotari commented May 6, 2024

  • If the Service URL differs from the broker urls, then the corresponding token for authentification is not found. ex: service url https://brokers.pulsar.internal:8443 found brokers: http://broker01.pulsar.internal:8080 even the brokers are configured with tls. the environment token is then not found for broker01, it matches only brokers.pulsar.internal.

btw. This is related to some gaps in the Pulsar API. In the past brokers didn't have a stable unique identifier. In PR apache/pulsar#21894, the "lookupServiceAddress" was renamed to be "brokerId" so that there would be a single unique identifier for a broker in a cluster. This is WIP and apache/pulsar#22647 adds brokerId to lookup results. There are a few more APIs to cover to make the brokerId the unique id for a broker.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution @bpereto

@lhotari lhotari merged commit 5b4eda2 into apache:master May 6, 2024
1 check passed
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