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

PMM-12880 Proper skip for MySQL TLS certs. #2909

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

JiriCtvrtka
Copy link
Contributor

@JiriCtvrtka JiriCtvrtka commented Mar 19, 2024

PMM-12880

Link to the Feature Build: Percona-Lab/pmm-submodules#3593

case len(s.Files()) != 0:
cfg.Params["tls"] = "custom"
case s.TLSSkipVerify:
cfg.Params["tls"] = skipVerify
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mandatory to have "custom" as the first case.

Copy link
Member

Choose a reason for hiding this comment

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

could you please add it as a comment there?

Copy link
Member

Choose a reason for hiding this comment

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

and also add that TLSSkipVerify for custom is handled in pmm-agent side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

case len(s.Files()) != 0:
cfg.Params["tls"] = "custom"
case s.TLSSkipVerify:
cfg.Params["tls"] = skipVerify
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mandatory to have "custom" as the first case.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 43.31%. Comparing base (b4a4c6d) to head (f671454).
Report is 2 commits behind head on main.

Files Patch % Lines
agent/tlshelpers/mysql.go 0.00% 5 Missing ⚠️
agent/agents/mysql/perfschema/perfschema.go 0.00% 1 Missing ⚠️
agent/agents/mysql/slowlog/slowlog.go 0.00% 1 Missing ⚠️
agent/connectionchecker/connection_checker.go 50.00% 1 Missing ⚠️
agent/runner/actions/common.go 50.00% 1 Missing ⚠️
agent/serviceinfobroker/service_info_broker.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2909      +/-   ##
==========================================
+ Coverage   43.28%   43.31%   +0.03%     
==========================================
  Files         399      399              
  Lines       41394    41395       +1     
==========================================
+ Hits        17916    17930      +14     
+ Misses      21477    21465      -12     
+ Partials     2001     2000       -1     
Flag Coverage Δ
agent 52.81% <50.00%> (+0.13%) ⬆️
managed 44.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

err = mysql.RegisterTLSConfig("custom", &tls.Config{
RootCAs: ca,
Certificates: []tls.Certificate{cert},
InsecureSkipVerify: tlsSkipVerify, // #nosec G402
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally missed there for "custom" ones at all.

@JiriCtvrtka JiriCtvrtka marked this pull request as ready for review March 19, 2024 11:27
@JiriCtvrtka JiriCtvrtka requested a review from a team as a code owner March 19, 2024 11:27
@JiriCtvrtka JiriCtvrtka requested review from BupycHuk, idoqo and ademidoff and removed request for a team March 19, 2024 11:27
case len(s.Files()) != 0:
cfg.Params["tls"] = "custom"
case s.TLSSkipVerify:
cfg.Params["tls"] = skipVerify
Copy link
Member

Choose a reason for hiding this comment

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

could you please add it as a comment there?

case len(s.Files()) != 0:
cfg.Params["tls"] = "custom"
case s.TLSSkipVerify:
cfg.Params["tls"] = skipVerify
Copy link
Member

Choose a reason for hiding this comment

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

and also add that TLSSkipVerify for custom is handled in pmm-agent side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants