Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

PMM-5364 SSL support for Mongo. #528

Merged
merged 63 commits into from
Dec 29, 2020
Merged

PMM-5364 SSL support for Mongo. #528

merged 63 commits into from
Dec 29, 2020

Conversation

JiriCtvrtka
Copy link
Contributor

@JiriCtvrtka JiriCtvrtka commented Nov 20, 2020

PMM-5364


  • Tests passed.
  • Feature build pass.
  • (Re)requested review.
  • Fix conflicts with target branch.
  • Update API dependency.
  • Update jira ticket description if needed.
  • Attach screenshots/console output to confirm new behavior to jira ticket, if applicable.

@JiriCtvrtka JiriCtvrtka changed the title PMM-5364 Deps. PMM-5364 SSL support for Mongo Nov 20, 2020
@JiriCtvrtka JiriCtvrtka changed the title PMM-5364 SSL support for Mongo PMM-5364 SSL support for Mongo. Nov 20, 2020
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #528 (d7e037b) into PMM-2.0 (3139917) will decrease coverage by 0.19%.
The diff coverage is 47.86%.

Impacted file tree graph

@@             Coverage Diff             @@
##           PMM-2.0     #528      +/-   ##
===========================================
- Coverage    49.66%   49.46%   -0.20%     
===========================================
  Files          123      123              
  Lines        15453    15547      +94     
===========================================
+ Hits          7674     7690      +16     
- Misses        7067     7148      +81     
+ Partials       712      709       -3     
Flag Coverage Δ
all 48.87% <47.86%> (-0.20%) ⬇️
cover 49.45% <52.97%> (-0.14%) ⬇️
crosscover 49.46% <47.86%> (-0.20%) ⬇️
update 16.76% <1.19%> (-0.31%) ⬇️

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

Impacted Files Coverage Δ
models/database.go 58.46% <ø> (ø)
services/agents/registry.go 0.00% <0.00%> (ø)
services/management/grpc/actions_server.go 0.00% <0.00%> (ø)
services/management/mongodb.go 0.00% <0.00%> (ø)
models/agent_helpers.go 58.02% <30.00%> (-0.66%) ⬇️
services/inventory/agents.go 39.81% <50.00%> (-0.04%) ⬇️
models/dsn_helpers.go 72.22% <57.14%> (+11.11%) ⬆️
models/delimiter.go 71.42% <66.66%> (ø)
services/checks/checks.go 48.13% <66.66%> (+0.16%) ⬆️
services/agents/mysql.go 82.60% <71.42%> (-0.90%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3139917...db45059. Read the comment docs.

TLSSkipVerify bool `reform:"tls_skip_verify"`
TLSCertificateKey string `reform:"tls_certificate_key"`
TLSCertificateKeyFilePassword string `reform:"tls_certificate_key_file_password"`
TLSCaKey string `reform:"tls_ca_key"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@JiriCtvrtka @BupycHuk I think it might be a time for us to start adding those new properties into a single JSON column

Copy link
Member

Choose a reason for hiding this comment

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

yeah, it might be good reason for that and probably we will need one more task to move other additional fields to single json column.
Other question is should we use the same column for all fields or split them by groups? Using separate columns for each service type will be easier to unmarshal and keep only required field. what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

separate columns for each service type

Sounds good

@AlekSi
Copy link
Contributor

AlekSi commented Nov 26, 2020

We should also update

case QANMongoDBProfilerAgentType, MongoDBExporterType:
q := make(url.Values)
if dialTimeout != 0 {
q.Set("connectTimeoutMS", strconv.Itoa(int(dialTimeout/time.Millisecond)))
}
// https://docs.mongodb.com/manual/reference/connection-string/
// > If the connection string does not specify a database/ you must specify a slash (/)
// between the last host and the question mark (?) that begins the string of options.
path := database
if database == "" {
path = "/"
}
if s.TLS {
q.Add("ssl", "true")
if s.TLSSkipVerify {
q.Add("tlsInsecure", "true")
}
}
address := socket
if socket == "" {
address = net.JoinHostPort(host, strconv.Itoa(int(port)))
}
u := &url.URL{
Scheme: "mongodb",
Host: address,
Path: path,
RawQuery: q.Encode(),
}
switch {
case password != "":
u.User = url.UserPassword(username, password)
case username != "":
u.User = url.User(username)
}
return u.String()

@BupycHuk BupycHuk closed this Dec 23, 2020
@BupycHuk BupycHuk reopened this Dec 23, 2020
@BupycHuk BupycHuk marked this pull request as ready for review December 23, 2020 23:24
@askomorokhov
Copy link
Contributor

Please resolve conflict

@@ -275,7 +310,10 @@ func (s *Agent) DSN(service *Service, dialTimeout time.Duration, database string
case username != "":
u.User = url.User(username)
}
return u.String()
dsn := u.String()
dsn = strings.Replace(dsn, url.QueryEscape(tdp.Left), tdp.Left, -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be ReplaceAll()

@BupycHuk BupycHuk requested a review from AlekSi December 24, 2020 19:09
@@ -169,13 +188,17 @@ func (s *Agent) UnifiedLabels() (map[string]string, error) {
}

// DSN returns DSN string for accessing given Service with this Agent (and implicit driver).
func (s *Agent) DSN(service *Service, dialTimeout time.Duration, database string) string {
func (s *Agent) DSN(service *Service, dialTimeout time.Duration, database string, tdp *DelimiterPair) string {

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
Function 'DSN' has too many statements (96 > 40) (funlen)

}
require.Len(t, actualAgents, 11)

// TODO: fix protobuf equality https://jira.percona.com/browse/PMM-6743

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
services/inventory/agents_test.go:254: Line contains TODO/BUG/FIXME: "TODO: fix protobuf equality https://jira..." (godox)

@@ -110,21 +116,27 @@ func TestAgent(t *testing.T) {
ProxySQLExporterType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?timeout=1s&tls=true",
QANMySQLPerfSchemaAgentType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?clientFoundRows=true&parseTime=true&timeout=1s&tls=true",
QANMySQLSlowlogAgentType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?clientFoundRows=true&parseTime=true&timeout=1s&tls=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&sslCaFile={{.TextFiles.caFilePlaceholder}}&sslCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&sslCertificateKeyFilePassword=pass",

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 282 characters (lll)

MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&sslCaFile={{.TextFiles.caFilePlaceholder}}&sslCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&sslCertificateKeyFilePassword=pass",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&sslCaFile={{.TextFiles.caFilePlaceholder}}&sslCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&sslCertificateKeyFilePassword=pass",

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 282 characters (lll)


assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true", agent.DSN(service, time.Second, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?ssl=true", agent.DSN(service, 0, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true&sslCaFile={{.TextFiles.caFilePlaceholder}}&sslCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}", agent.DSN(service, time.Second, "", nil))

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 268 characters (lll)


dsn, agent, err := models.FindDSNByServiceIDandPMMAgentID(q, "S4", "PA2", "test")
require.NoError(t, err)
expected := "mongodb://pmm-user%7B%7B@127.0.0.1:27017/test?connectTimeoutMS=1000&ssl=true&sslCaFile=[[.TextFiles.caFilePlaceholder]]&sslCertificateKeyFile=[[.TextFiles.certificateKeyFilePlaceholder]]&sslCertificateKeyFilePassword=passwordoftls"

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 252 characters (lll)

@@ -887,13 +893,18 @@ func (r *Registry) StartPostgreSQLShowIndexAction(ctx context.Context, id, pmmAg
}

// StartMongoDBExplainAction starts MongoDB query explain action on pmm-agent.
func (r *Registry) StartMongoDBExplainAction(ctx context.Context, id, pmmAgentID, dsn, query string) error {
func (r *Registry) StartMongoDBExplainAction(ctx context.Context, id, pmmAgentID, dsn, query string, files map[string]string, tdp *models.DelimiterPair) error {

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
(*Registry).StartMongoDBExplainAction - ctx is unused (unparam)

@@ -996,12 +1007,17 @@ func (r *Registry) StartPostgreSQLQuerySelectAction(ctx context.Context, id, pmm
}

// StartMongoDBQueryGetParameterAction starts MongoDB getParameter query action on pmm-agent.
func (r *Registry) StartMongoDBQueryGetParameterAction(ctx context.Context, id, pmmAgentID, dsn string) error {
func (r *Registry) StartMongoDBQueryGetParameterAction(ctx context.Context, id, pmmAgentID, dsn string, files map[string]string, tdp *models.DelimiterPair) error {

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
(*Registry).StartMongoDBQueryGetParameterAction - ctx is unused (unparam)

@@ -1017,12 +1033,17 @@ func (r *Registry) StartMongoDBQueryGetParameterAction(ctx context.Context, id,
}

// StartMongoDBQueryBuildInfoAction starts MongoDB buildInfo query action on pmm-agent.
func (r *Registry) StartMongoDBQueryBuildInfoAction(ctx context.Context, id, pmmAgentID, dsn string) error {
func (r *Registry) StartMongoDBQueryBuildInfoAction(ctx context.Context, id, pmmAgentID, dsn string, files map[string]string, tdp *models.DelimiterPair) error {

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
(*Registry).StartMongoDBQueryBuildInfoAction - ctx is unused (unparam)

@@ -1038,12 +1059,17 @@ func (r *Registry) StartMongoDBQueryBuildInfoAction(ctx context.Context, id, pmm
}

// StartMongoDBQueryGetCmdLineOptsAction starts MongoDB getCmdLineOpts query action on pmm-agent.
func (r *Registry) StartMongoDBQueryGetCmdLineOptsAction(ctx context.Context, id, pmmAgentID, dsn string) error {
func (r *Registry) StartMongoDBQueryGetCmdLineOptsAction(ctx context.Context, id, pmmAgentID, dsn string, files map[string]string, tdp *models.DelimiterPair) error {

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
(*Registry).StartMongoDBQueryGetCmdLineOptsAction - ctx is unused (unparam)

@@ -110,21 +116,27 @@ func TestAgent(t *testing.T) {
ProxySQLExporterType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?timeout=1s&tls=true",
QANMySQLPerfSchemaAgentType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?clientFoundRows=true&parseTime=true&timeout=1s&tls=true",
QANMySQLSlowlogAgentType: "username:s3cur3 p@$$w0r4.@tcp(1.2.3.4:12345)/database?clientFoundRows=true&parseTime=true&timeout=1s&tls=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&tlsCertificateKeyFilePassword=pass",

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 282 characters (lll)

MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true",
MongoDBExporterType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&tlsCertificateKeyFilePassword=pass",
QANMongoDBProfilerAgentType: "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/database?connectTimeoutMS=1000&ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}&tlsCertificateKeyFilePassword=pass",

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 282 characters (lll)


assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true", agent.DSN(service, time.Second, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?ssl=true", agent.DSN(service, 0, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}", agent.DSN(service, time.Second, "", nil))

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 268 characters (lll)

assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true", agent.DSN(service, time.Second, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?ssl=true", agent.DSN(service, 0, ""))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?connectTimeoutMS=1000&ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}", agent.DSN(service, time.Second, "", nil))
assert.Equal(t, "mongodb://username:s3cur3%20p%40$$w0r4.@1.2.3.4:12345/?ssl=true&tlsCaFile={{.TextFiles.caFilePlaceholder}}&tlsCertificateKeyFile={{.TextFiles.certificateKeyFilePlaceholder}}", agent.DSN(service, 0, "", nil))

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 236 characters (lll)


dsn, agent, err := models.FindDSNByServiceIDandPMMAgentID(q, "S4", "PA2", "test")
require.NoError(t, err)
expected := "mongodb://pmm-user%7B%7B@127.0.0.1:27017/test?connectTimeoutMS=1000&ssl=true&tlsCaFile=[[.TextFiles.caFilePlaceholder]]&tlsCertificateKeyFile=[[.TextFiles.certificateKeyFilePlaceholder]]&tlsCertificateKeyFilePassword=passwordoftls"

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
line is 252 characters (lll)

@BupycHuk BupycHuk merged commit 0db06f7 into PMM-2.0 Dec 29, 2020
@BupycHuk BupycHuk deleted the PMM-5364-ssl-mongo branch December 29, 2020 09:22
askomorokhov pushed a commit that referenced this pull request Mar 22, 2021
* PMM-5364 Deps.

* PMM-5364 Add new fields to add request.

* PMM-5364 Deps.

* PMM-5364 Deps.

* PMM-5364 Add new fields into DB.

* PMM-5364 Reform.

* PMM-5364 Another fields changes.

* PMM-5364 Deps.

* PMM-5364 Deps.

* PMM-5364 Fields.

* PMM-5364 Deps.

* PMM-7026 One more revert.

* PMM-7026 Deps.

* PMM-5364 Changes fields into struct.

* PMM-5364 Fix.

* PMM-5364 Changes.

* PMM-5364 Deps.

* PMM-5364 Gen.

* PMM-5364 Deps.

* PMM-5364 Add TLS keys to MongoDBExplainAction request.

* PMM-5364 Remove old code.

* PMM-5364 Changes.

* PMM-5364 Fix new db fields VM problem.

* PMM-5364 Naming changes.

* PMM-5364 Changes.

* PMM-5364 Deps.

* PMM-5364 Deps.

* PMM-5364 Fix build.

* PMM-5364 Fix test.

* PMM-5367 Changes.

* PMM-5364 Deps.

* PMM-5364 Set default prefix to same.

* PMM-5364 Places for paste creating certs.

* PMM-5364 Fix test.

* PMM-5364 Fix test.

* PMM-5364 Make MongoDBOptions struct.

* PMM-5364 Fix tests.

* PMM-5364 Use text files parameters as .TextFiles. .

* PMM-5364 Use text files parameters as .TextFiles .

* PMM-5364 Refactoring .

* PMM-5364 Fix connection checker.

* PMM-5364 Fix tests.

* PMM-5364 Fix QAN MongoDB Profiler.

* PMM-5364 Fix Explain Action for MongoDB SSL.

* PMM-5364 Fix merge conflicts.

Co-authored-by: Nurlan Moldomurov <nurlan.moldomurov@percona.com>
Co-authored-by: Alexey Palazhchenko <alexey.palazhchenko@percona.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants