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

[oracle] README updates regarding DSN formats #8841

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

chrisberkhout
Copy link
Contributor

@chrisberkhout chrisberkhout commented Jan 8, 2024

Proposed commit message

[oracle] README updates regarding DSN formats (#8841)

- Remove unsupported DSN format.
- Add note about URL encoding special characters.
- Add logfmt-encoded DSN format.

Discussion

The oracle integration uses Filebeat's sql input. That uses the GO DRiver for ORacle DB to parse Oracle DSNs. When supplied with the removed format, it parses the joined username and password as only a username, with no password, as demonstrated by the following script.

DSN parsing script

Script:

package main

import (
	"fmt"

	"github.com/godror/godror"
)

func main() {
	params, err := godror.ParseDSN("sys:Oradoc_db1@0.0.0.0:1521/ORCLCDB.localdomain?sysdba=1")
	if err != nil {
		fmt.Println(err)
	} else {
		fmt.Println(params.Username)
		fmt.Println(params.Password.Secret())
	}
}

Output:

sys:Oradoc_db1

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.

@chrisberkhout chrisberkhout added Integration:oracle Oracle bugfix Pull request that fixes a bug issue labels Jan 8, 2024
@chrisberkhout chrisberkhout self-assigned this Jan 8, 2024
@chrisberkhout chrisberkhout requested a review from a team as a code owner January 8, 2024 16:08
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

LGTM.

Looks like this also needs reviewed from @elastic/obs-infraobs-integrations as codeowners.

@chrisberkhout chrisberkhout force-pushed the oracle-dsn-formats branch 2 times, most recently from d418a8b to fe40a56 Compare January 24, 2024 08:25
@chrisberkhout
Copy link
Contributor Author

/test

@narph narph added Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] and removed Team:Security-External Integrations labels Jan 29, 2024
@agithomas
Copy link
Contributor

agithomas commented Jan 30, 2024

@chrisberkhout, apologies for the delayed review. There was a related Oracle Integration issue related to the DSN format, which is now resolved, following user testing and confirmation.

This new DSN format has been available since ES version 8.7.

An example of the new format is

user="sys" password="Oradoc_db1" connectString="0.0.0.0:1521/ORCLCDB.localdomain" sysdba=true

Reference: https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-module-oracle.html

Would you like to consider including this format as part of this PR?

@chrisberkhout
Copy link
Contributor Author

Hi @agithomas, no worries, I've added that 2nd format.

This Oracle integration actually uses the SQL input, not the Oracle input. However, those inputs both use the same library for parsing DSNs.

I update the documentation for both of the inputs in elastic/beats#37590. These PRs are an attempt to clean up the documentation following a user problem with escaping while using an oracle:// URL.

@elasticmachine
Copy link

💚 Build Succeeded

History

  • 💚 Build #8309 succeeded 42d2e6a31f323cef80dfd05701e2cc07c5359b4f
  • 💔 Build #8304 failed fe40a56313310bc5f73e394a4dae888531aa8d27
  • 💔 Build #8301 failed fe40a56313310bc5f73e394a4dae888531aa8d27

cc @chrisberkhout

The following two configuration formats are supported:
```
oracle://<user>:<password>@<connection_string>
user="<user>" password="<password>" connectString="<connection_string>" sysdba=<true|false>
Copy link
Contributor

Choose a reason for hiding this comment

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

I have tested both with

  • sysdba=false
  • without sysdba=false

both works Ok

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!

@chrisberkhout chrisberkhout merged commit 96951f4 into elastic:main Jan 31, 2024
3 checks passed
@chrisberkhout chrisberkhout deleted the oracle-dsn-formats branch January 31, 2024 10:34
@elasticmachine
Copy link

Package oracle - 1.24.2 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
bugfix Pull request that fixes a bug issue Integration:oracle Oracle Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants