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

docs: clarify the usage of the server information endpoint #11881

Merged
merged 2 commits into from
Oct 24, 2023
Merged

docs: clarify the usage of the server information endpoint #11881

merged 2 commits into from
Oct 24, 2023

Conversation

rodrigc
Copy link
Contributor

@rodrigc rodrigc commented Oct 17, 2023

Discussed in case 01501264

Signed-off-by: Craig Rodrigues rodrigc@crodrigues.org

Motivation/summary

I found the existing documentation for the server information endpoint to be a bit confusing.
I didn't understand that GET and POST behave differently.

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Related issues

@rodrigc rodrigc requested a review from a team as a code owner October 17, 2023 21:57
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2023

This pull request does not have a backport label. Could you fix it @rodrigc? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Oct 17, 2023
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks for sending the PR @rodrigc! I can't see your support case, but I guess there was some miscommunication: the endpoint's response body shouldn't differ based on whether it's a GET or POST method, what matters is whether the client is authorized. This is to minimise leaking information about the service to unauthorized (and potentially malicious) clients.

docs/api-info.asciidoc Outdated Show resolved Hide resolved
@axw axw enabled auto-merge (squash) October 23, 2023 00:30
@axw axw added docs Team:Docs Label for the Observability docs team labels Oct 23, 2023
@axw
Copy link
Member

axw commented Oct 23, 2023

@rodrigc sorry, could you please sign your commit and force push again?

Discussed in [case 01501264](https://support.elastic.co/cases/5008X00002UnA9UQAV)

Signed-off-by: Craig Rodrigues <rodrigc@crodrigues.org>
auto-merge was automatically disabled October 23, 2023 00:50

Head branch was pushed to by a user without write access

@rodrigc
Copy link
Contributor Author

rodrigc commented Oct 23, 2023

Done

@axw axw enabled auto-merge (squash) October 23, 2023 00:55
@rodrigc
Copy link
Contributor Author

rodrigc commented Oct 23, 2023

@axw can you trigger the docs CI? I'm not sure, but I think something like:

@elasticsearchmachine run elasticsearch-ci/docs

is required

@axw
Copy link
Member

axw commented Oct 24, 2023

/run elasticsearch-ci/docs

@axw
Copy link
Member

axw commented Oct 24, 2023

/run elasticsearch-ci/docs

@axw axw merged commit 794cdd4 into elastic:main Oct 24, 2023
7 checks passed
@rodrigc rodrigc deleted the apm-clarify branch October 24, 2023 01:40
@rodrigc
Copy link
Contributor Author

rodrigc commented Oct 25, 2023

@axw Thanks for the review and merge of this PR.

I have one question. Would it be possible to include a default role in the apm-server code which is similar to
the one mentioned here:
https://www.elastic.co/guide/en/apm/guide/current/api-key.html#create-api-key-user

That way, the end user would not need to worry so much about creating a role
to use an API key which works with APM

@axw
Copy link
Member

axw commented Oct 26, 2023

@rodrigc we could predefine a role in Elasticsearch (not in apm-server), but you would still need to create/assign a user that role, so it doesn't seem entirely ideal either.

I think the long-term vision is that this should be built into a standard, but broader, role like "editor". The idea being that you have users that are admins, viewers, or editors; admins and editors should be able to create API Keys for agents, whereas viewers should not. If you wanted a more fine-grained role, then you would still have the option to create one.

Does that sound sensible to you?

@rodrigc
Copy link
Contributor Author

rodrigc commented Oct 26, 2023

That seems like a reasonable approach. I agree that more thought needs to be put into this.

@axw
Copy link
Member

axw commented Oct 26, 2023

@rodrigc I've opened elastic/apm#831 to track this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify docs Team:Docs Label for the Observability docs team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants