Skip to content

[Oracle] Mark password field as secret #9707

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

Merged
merged 5 commits into from
May 10, 2024
Merged

[Oracle] Mark password field as secret #9707

merged 5 commits into from
May 10, 2024

Conversation

devamanv
Copy link
Contributor

@devamanv devamanv commented Apr 25, 2024

Proposed commit message

The PR contains changes to mark the password field as secret.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • Mark the password field as secret
  • Upgrading the package doesn't break the existing integration and metrics still come through
  • No dashboards are broken

How to test this PR locally

  • Spin up the Elastic stack and point your elastic-package to the running Kibana instance
  • Build the package locally and install the package using elastic-package install --zip oracle-1.25.1.zip
  • Upgrade the integration to the latest version i.e. 1.25.1, there shouldn't be any errors. If you find any errors while upgrading, please report it by adding a comment to this PR.
  • You should now see separate fields for username and password under advanced options while configuring the Integration for metrics collection
  • Take a look at the dashboards, they shouldn't be broken and metrics should still be coming in just fine

Related issues

Screenshots

These are after enabling the Performance and Memory metricsets.

image
Oracle Metrics Overview Dashboard

image
Performance Metrics Dashboard

image
Memory Metrics Dashboard

@devamanv devamanv added the enhancement New feature or request label Apr 25, 2024
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@devamanv devamanv marked this pull request as ready for review May 8, 2024 10:04
@devamanv devamanv requested a review from a team as a code owner May 8, 2024 10:04
@devamanv devamanv self-assigned this May 8, 2024
@shmsr shmsr changed the title [Oracle] Mark password field as secret [Oracle] Mark password field as secret May 9, 2024
Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

Review shared.

@ishleenk17
Copy link
Member

@devamanv : I did read your comment on the upgrade scenario testing in the Issue.
Are we saying that both formats will work in the latest integration?

  1. The DSN having the host information along with the username/passowrd info.
  2. The DSN having the host information and username/password being separate fields? (If the need passwords as secrets).

@devamanv
Copy link
Contributor Author

devamanv commented May 10, 2024

Are we saying that both formats will work in the latest integration?

@ishleenk17 Yes, I think I have pointed that in my previous comments as well. The Beats code gives preference to params(i.e. username and passwords are part of the DSN itself) if present. That ensures the backwards compatibility and with the new changes, the password is taken as secret which covers the second point.

Copy link
Member

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Please take care of the Kibana version. Upgrade to 1.26.0.
Otherwise looks good!

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ritalwar ritalwar left a comment

Choose a reason for hiding this comment

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

LGTM!

@devamanv devamanv requested a review from shmsr May 10, 2024 15:36
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @devamanv

@devamanv devamanv merged commit b67e647 into elastic:main May 10, 2024
@devamanv devamanv deleted the mask-fields-oracle branch May 10, 2024 18:23
@elasticmachine
Copy link

Package oracle - 1.26.0 containing this change is available at https://epr.elastic.co/search?package=oracle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:oracle Oracle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants