-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[metricbeat][mysql] Add SSL support #37997
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look more or less good only but I feel I need to do another comprehensive review. I'll do it ASAP.
Can you share the minimum steps to test the changes made in the "How to test this PR locally" section of the PR description? |
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
Co-authored-by: Aman <38116245+devamanv@users.noreply.github.com>
/test |
@@ -31,7 +31,7 @@ import ( | |||
func TestNewDB(t *testing.T) { | |||
service := compose.EnsureUp(t, "mysql") | |||
|
|||
db, err := NewDB(GetMySQLEnvDSN(service.Host())) | |||
db, err := NewDB(GetMySQLEnvDSN(service.Host()), nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we introduce a test that validates the added SSL support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good. Are you planning to add integration tests (what Denis also asked)?
4fe8058
to
162a21c
Compare
💚 Build Succeeded
History
cc @gpop63 |
💚 Build Succeeded
History
cc @gpop63 |
💚 Build Succeeded
History
cc @gpop63 |
💔 Build Failed
Failed CI Steps
History
cc @gpop63 |
💔 Build Failed
Failed CI StepsHistory
cc @gpop63 |
💚 Build Succeeded
History
cc @gpop63 |
#ssl.certificate: "/etc/pki/client/cert.crt" | ||
|
||
# Client certificate key file | ||
#ssl.key: "/etc/pki/client/cert.key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pasted text should've had a new line character at the end. The missing character led to the broken documentation. See the fixing PR #38367
Overview
Adds SSL support for following metricsets:
The
query
metricset uses a different SQL client (helper/sql
). Others use the client from the module itself.Added a config struct as part of this change.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
genroot.sh
genserver.sh
genclient.sh
gencerts.sh
Then run
sh +x gencerts.sh
to generate required certificates.Update certificates ownership and permissions so the
mysql
container can use them.docker-compose.up.yml
Related issues
Use cases
Screenshots
Logs