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

[kibana] support api_key #40360

Merged
merged 9 commits into from
Sep 20, 2024
Merged

[kibana] support api_key #40360

merged 9 commits into from
Sep 20, 2024

Conversation

sachin-frayne
Copy link
Contributor

Summary

Adds support for api_key authentication in the kibana module

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

  • create an api key and grab the beats format[1]
  • configure kibana module to use that api_key:
metricbeat.modules:
  - module: kibana
    xpack.enabled: true
    hosts: ["http://localhost:9200"]
    api_key: <key in beats format>
    period: 10s
  • kibana metricsets are successfully collecting documents

[1]
Screenshot 2023-08-09 at 17 23 25

Related issues

Use cases

Screenshots

Logs

@sachin-frayne sachin-frayne requested review from a team as code owners July 26, 2024 10:41
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 26, 2024
Copy link
Contributor

mergify bot commented Jul 26, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @sachin-frayne? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jul 26, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@pierrehilbert pierrehilbert added the Team:Monitoring Stack Monitoring team label Jul 26, 2024
Copy link
Contributor

mergify bot commented Sep 11, 2024

backport-v8.x has been added to help with the transition to the new branch 8.x.

Copy link
Contributor

mergify bot commented Sep 11, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 11, 2024
@VihasMakwana
Copy link
Contributor

@sachin-frayne can you take care of CI failures. LGTM otherwise.

@VihasMakwana
Copy link
Contributor

I've rebased main.

@sachin-frayne
Copy link
Contributor Author

I've rebased main.

Thank you

@v1v v1v removed the backport-v8.x label Sep 11, 2024
@sachin-frayne
Copy link
Contributor Author

sachin-frayne commented Sep 12, 2024

Hello @VihasMakwana.

I have fixed the CI, I have also changed the setting in: .github/workflows/golangci-lint.yml removing:

skip-go-installation: true

I don't think this was causing any failures, but in debugging I found that the setting is removed/being removed: https://github.com/golangci/golangci-lint-action

Can you please review one more time?

@sachin-frayne
Copy link
Contributor Author

Hello @VihasMakwana

When you get a chance can you please review this?

@pierrehilbert
Copy link
Collaborator

@alexsapran / @pazone could you please review here?

@alexsapran
Copy link
Contributor

@@ -53,8 +53,5 @@ jobs:
# into fixing all linting issues in the whole file instead
args: --timeout=30m --whole-files

# Optional: if set to true then the action will use pre-installed Go.
skip-go-installation: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This was deprecated in

v4.0.0+ requires an explicit actions/setup-go installation step before using this action: uses: actions/setup-go@v5. The skip-go-installation option has been removed.

But we are running version v1.55.2, so I am not sure removing this achieves what you want.
Why we wan to remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert this change now.

@pierrehilbert
Copy link
Collaborator

Yes this is where your review is needed, the rest of the PR has been reviewed by @belimawr

@sachin-frayne
Copy link
Contributor Author

I have added skip-go-installation: true back into .github/workflows/golangci-lint.yml.

I removed it while trying to fix some CI issues since it will be deprecated anyways, but since current version is still using it, I have no reason to change it.

@sachin-frayne
Copy link
Contributor Author

@pierrehilbert and @belimawr

Since I have reverted the change with the skip-go-installation can this move onto the next stage, I believe I am waiting for a reviewer approval.

Thank you.

metricbeat/module/kibana/kibana.go Outdated Show resolved Hide resolved
metricbeat/module/kibana/status/status.go Outdated Show resolved Hide resolved
@sachin-frayne
Copy link
Contributor Author

Hey @belimawr, :)

Thank you for your review and the suggestions, you are correct the header should only be set if the apikey is not an empty string, I have implemented an if statement around the function.

Please review and let me know if you are happy with the changes?

@sachin-frayne
Copy link
Contributor Author

Hello @VihasMakwana:

I am not sure, but I believe I am still waiting for your approval.

@pierrehilbert
Copy link
Collaborator

@sachin-frayne as @VihasMakwana and @belimawr are in the same team, one of them you should be enough.
Looks like you need someone from @elastic/stack-monitoring

@consulthys
Copy link
Contributor

@sachin-frayne is @elastic/stack-monitoring :-)

@pierrehilbert
Copy link
Collaborator

@sachin-frayne is @elastic/stack-monitoring :-)

I still think you need someone from your team to approve the change according to the codeowners.

@sachin-frayne sachin-frayne merged commit f934ab8 into main Sep 20, 2024
29 checks passed
@sachin-frayne sachin-frayne deleted the kibana-api-key branch September 20, 2024 07:11
mergify bot pushed a commit that referenced this pull request Sep 20, 2024
* [kibana] support api_key

* changes username and password to generic values

* make check formatting changes

* removes skip-go-installation option

* fixes: naked return in func

* adds skip-go-installation back

* only sets header if apikey is not empty

---------

Co-authored-by: VihasMakwana <[email protected]>
(cherry picked from commit f934ab8)
pierrehilbert added a commit that referenced this pull request Sep 30, 2024
* [kibana] support api_key

* changes username and password to generic values

* make check formatting changes

* removes skip-go-installation option

* fixes: naked return in func

* adds skip-go-installation back

* only sets header if apikey is not empty

---------

Co-authored-by: VihasMakwana <[email protected]>
(cherry picked from commit f934ab8)

Co-authored-by: Sachin Frayne <[email protected]>
Co-authored-by: Pierre HILBERT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify enhancement Module:kibana Kibana Beats modules Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Monitoring Stack Monitoring team Team:Opex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants