Skip to content

[Elasticsearch] Added ssl configuration to ES module#4946

Merged
mohamedhamed-ahmed merged 1 commit intoelastic:mainfrom
mohamedhamed-ahmed:4666-add-ssl-config-to-elasticsearch-module
Jan 9, 2023
Merged

[Elasticsearch] Added ssl configuration to ES module#4946
mohamedhamed-ahmed merged 1 commit intoelastic:mainfrom
mohamedhamed-ahmed:4666-add-ssl-config-to-elasticsearch-module

Conversation

@mohamedhamed-ahmed
Copy link
Copy Markdown
Contributor

What does this PR do?

Add ssl configuration options for elasticsearch integration module

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that the new ssl configuration is being passed properly to the metricbeat module

How to this PR was tested

  • Applied changes locally, and used elastic-package to spawn up a complete stack locally.
  • Made sure the new fields are visible in the UI as below:

Screenshot 2022-12-22 at 15 21 23

  • Checked that the policy has the new fields mapped properly

Screenshot 2022-12-22 at 15 46 55

  • Spawned an Elastic agent locally and checked the matribeat logs to make sure that it initially fails when connecting to ES and then succeeds to establish a connection to ES when ssl configuration are passed properly

Related issues

@mohamedhamed-ahmed mohamedhamed-ahmed added bug Something isn't working, use only for issues Integration:elasticsearch Elasticsearch v8.7.0 Team:Infra Monitoring UI - DEPRECATED Label for the Infrastructure Monitoring UI team. - DEPRECATED - Use Team:obs-ux-infra_services labels Jan 9, 2023
@mohamedhamed-ahmed mohamedhamed-ahmed self-assigned this Jan 9, 2023
@mohamedhamed-ahmed mohamedhamed-ahmed requested a review from a team as a code owner January 9, 2023 09:44
@mohamedhamed-ahmed
Copy link
Copy Markdown
Contributor Author

Did we verify that api keys where accepted by metricbeat module ? This opened issue says otherwise elastic/beats#29271

After taking a look at metricbeat module it seems that it doesn't support api_key authorization which is also proven by the ticket Kevin linked above.

The change seems to be a bit bigger and separate from the scope of this ticket and I would suggest having a separate ticket for it.

The necessary changes as per my understanding are as follows:

  1. Add APIKey string prop here which will hold the raw decoded api_key value.
  2. Read the config api_key here similar to username and pass.
  3. checking here if both Username, Password & api_key are both set then reject similar to this.
  4. Keep cascading the api_key prop through the different functions similar to username and password so that you are able to access it here.
  5. Add Auth header with the encoded api_key value, similar to what is done here and here.
  6. We need to verify what should happen if both BearerTokenFile & APIKey are set, should we just overwrite the value with what we have in the api_key prop or reject similar to step 3.
  7. Update affected tests

@elasticmachine
Copy link
Copy Markdown

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-09T09:44:15.605+0000

  • Duration: 32 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 55
Skipped 0
Total 55

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Copy Markdown

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (5/5) 💚
Files 100.0% (9/9) 💚
Classes 100.0% (9/9) 💚
Methods 88.073% (96/109) 👎 -5.26
Lines 91.98% (562/611) 👎 -8.02
Conditionals 100.0% (0/0) 💚

@klacabane
Copy link
Copy Markdown
Contributor

LGTM. Let's create a follow up (or update the existing ticket with the findings) for adding api key support in metricbeat module

@mohamedhamed-ahmed mohamedhamed-ahmed merged commit 167a0ed into elastic:main Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working, use only for issues Integration:elasticsearch Elasticsearch Team:Infra Monitoring UI - DEPRECATED Label for the Infrastructure Monitoring UI team. - DEPRECATED - Use Team:obs-ux-infra_services v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants