Skip to content

[functions] Don't auth functions worker metrics endpoint #6801

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
Merged
codelipenghui merged 1 commit into from
Jun 7, 2020
Merged

[functions] Don't auth functions worker metrics endpoint #6801

codelipenghui merged 1 commit into from
Jun 7, 2020

Conversation

@addisonj
Copy link
Contributor

@addisonj addisonj commented Apr 23, 2020 •
edited
Loading

Motivation

The broker and proxy both allow for hitting the metrics endpoint without auth. The functions
worker should allow that to be configurable as well. This adds an option to
allow for metrics endpoint to allow the endpoint to be hit without auth

Additionally, the functions worker doesn't expose the default prometheus
metrics (such as JVM info, etc).

This commit implements and adds an option to support that

Verifying this change

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

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): n
  • The public API: n
  • The schema: n
  • The default values of configurations: n
  • The wire protocol: n
  • The rest endpoints: n
  • The admin cli options: n
  • Anything that affects deployment: n

Documentation

  • Does this pull request introduce a new feature? n

@addisonj
Copy link
Contributor Author

addisonj commented Apr 23, 2020

/pulsarbot run-failure-checks

2 similar comments
@addisonj
Copy link
Contributor Author

addisonj commented Apr 23, 2020

/pulsarbot run-failure-checks

@addisonj
Copy link
Contributor Author

addisonj commented Apr 24, 2020

/pulsarbot run-failure-checks

sijie
sijie requested changes Apr 27, 2020
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@addisonj I think we should add a flag to control this behavior. In some environments, you probably don't let users curl /metrics endpoint without authentication. The proxy case is different from the broker case because the proxy is usually used for exposing the service to the public.

@addisonj addisonj force-pushed the function_metrics branch 2 times, most recently from ac64ab7 to ef75d77 Compare May 5, 2020 20:26
@addisonj
Copy link
Contributor Author

addisonj commented May 5, 2020

@sijie I reworked this a fair bit and also expanded scope so you can get the prometheus metrics

@addisonj
Copy link
Contributor Author

addisonj commented May 5, 2020

/pulsarbot run-failure-checks

1 similar comment
@addisonj
Copy link
Contributor Author

addisonj commented May 5, 2020

/pulsarbot run-failure-checks

sijie
sijie reviewed May 7, 2020
View reviewed changes
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java Outdated
doc = "Whether the '/metrics' endpoint requires authentication. Defaults to false."
+ "'authenticationEnabled' must also be set for this to take effect."
)
private boolean authenticateMetricsEndpoint = false;
Copy link
Member

@sijie sijie May 7, 2020

Choose a reason for hiding this comment

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

Can we change the default to true so it is compatible with existing behavior?

@sijie sijie added this to the 2.6.0 milestone May 7, 2020
@addisonj addisonj force-pushed the function_metrics branch from ef75d77 to 5653aad Compare May 7, 2020 18:39
@sijie
Copy link
Member

sijie commented May 18, 2020

/pulsarbot run-failure-checks

@sijie sijie closed this May 21, 2020
@sijie sijie reopened this May 21, 2020
srkukarni
srkukarni reviewed May 22, 2020
View reviewed changes
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java Outdated
private Integer workerPortTls;
@FieldContext(
category = CATEGORY_WORKER,
doc = "Whether the '/metrics' endpoint requires authentication. Defaults to false."
Copy link
Contributor

@srkukarni srkukarni May 22, 2020

Choose a reason for hiding this comment

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

does it default to false or true? The doc says one and the default config says other

@addisonj
Copy link
Contributor Author

addisonj commented May 22, 2020 via email

@sijie
Copy link
Member

sijie commented Jun 2, 2020

@addisonj Can you address the comment?

@addisonj addisonj force-pushed the function_metrics branch from 5653aad to 7197b7e Compare June 2, 2020 22:56
@codelipenghui
Copy link
Contributor

codelipenghui commented Jun 4, 2020

@addisonj Please rebase to the master branch. There are some CI tests fix on the master branch.

[functions] Provide more options for func worker prometheus metrics
b99b4c8 
The broker and proxy both allow for hitting the metrics endpoint without auth. The functions
worker should allow that to be configurable as well. This adds an option to
allow for metrics endpoint to allow the endpoint to be hit without auth

Additionally, the functions worker doesn't expose the default prometheus
metrics (such as JVM info, etc).

This commit implements and adds an option to support that
@addisonj addisonj force-pushed the function_metrics branch from 7197b7e to b99b4c8 Compare June 4, 2020 17:15
@addisonj
Copy link
Contributor Author

addisonj commented Jun 4, 2020

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

codelipenghui commented Jun 5, 2020

@addisonj Please merge the master branch to your branch, #7173 has fix some tests issues. I can't push to your branch.

@codelipenghui codelipenghui merged commit ee0ba4c into apache:master Jun 7, 2020
cdbartholomew pushed a commit to kafkaesque-io/pulsar that referenced this pull request Jul 24, 2020
@addisonj @cdbartholomew
[functions] Provide more options for func worker prometheus metrics (a…
9c23c5a 
…pache#6801)

The broker and proxy both allow for hitting the metrics endpoint without auth. The functions
worker should allow that to be configurable as well. This adds an option to
allow for metrics endpoint to allow the endpoint to be hit without auth

Additionally, the functions worker doesn't expose the default prometheus
metrics (such as JVM info, etc).

This commit implements and adds an option to support that

Co-authored-by: Addison Higham <ahigham@instructure.com>
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@addisonj @huangdx0726
[functions] Provide more options for func worker prometheus metrics (a…
af1808a 
…pache#6801)

The broker and proxy both allow for hitting the metrics endpoint without auth. The functions
worker should allow that to be configurable as well. This adds an option to
allow for metrics endpoint to allow the endpoint to be hit without auth

Additionally, the functions worker doesn't expose the default prometheus
metrics (such as JVM info, etc).

This commit implements and adds an option to support that

Co-authored-by: Addison Higham <ahigham@instructure.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srkukarni srkukarni srkukarni left review comments

@sijie sijie sijie approved these changes

@jiazhai jiazhai jiazhai approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@addisonj addisonj

Labels

area/function area/security

Projects

None yet

Milestone

2.6.0

Development

Successfully merging this pull request may close these issues.

5 participants

@addisonj @sijie @codelipenghui @srkukarni @jiazhai