feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred #1300

embroede · 2022-05-11T06:44:01Z

Description

Enable configuration of the tls parameter for the mysql connection. An example would be setting tls to "preferred", or "true". This is separate from the tlsConfig (renamed from tls) parameter in manager.yaml, which is used to setup a custom tls config, where tls key/certs are specified. This will fix the case where the manager cannot connect to a server that requires TLS.

See the tls parameter section in the below link:
https://pkg.go.dev/github.com/go-sql-driver/mysql#section-readme

Related Issue

#1299

Motivation and Context

Currently Manager cannot connect to a MySQL database that requires TLS, without fully configuring certs/keys (functionality added in #1015). By making this configurable, TLS can be used (even with only password authentication).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation Update (if none of the other choices apply)

Checklist

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.

For communication from manager to mysql server, use `tls=preferred` parameter to enable TLS whenever possible. Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

gaius-qi · 2022-05-12T09:35:40Z

Can tls be changed to manager configuration?

https://github.com/dragonflyoss/Dragonfly2/blob/main/manager/config/config.go#L71

embroede · 2022-05-12T18:37:30Z

@gaius-qi Yes I like that better, I will make the change.

gaius-qi · 2022-05-16T06:12:17Z

@gaius-qi Yes I like that better, I will make the change.

Thx

Allow the user to specify the tls setting for the mysql connection. An example would be setting tls to "preferred", or "true". This is separate to the tlsConfig config parameter, which is used to set up a custom tls config, where tls key/certs are specified. See the tls parameter section in the below link: https://pkg.go.dev/github.com/go-sql-driver/mysql#section-readme Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

embroede · 2022-05-27T21:19:46Z

@gaius-qi I have updated the PR to add it to the configuration, and created a PR to update the docs here: dragonflyoss/d7y.io#79

gaius-qi · 2022-05-30T02:55:43Z

You can refer to harbor, TLSConfig is used to set the SSL mode, do not rename the TLS configuration.

When ssl mode is preferred and skip-verify set InsecureSkipVerify to true.

https://github.com/goharbor/harbor/blob/main/src/vendor/github.com/go-sql-driver/mysql/dsn.go#L48

manager/config/config.go

manager/config/config_test.go

manager/database/mysql.go

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

gaius-qi · 2022-06-27T12:47:07Z

Please fix unit tests. @embroede

2022-06-27T12:42:43.5657961Z === RUN   TestManagerConfig_Load
2022-06-27T12:42:43.5658567Z     config_test.go:107: 
2022-06-27T12:42:43.5659014Z         	Error Trace:	config_test.go:107
2022-06-27T12:42:43.5659405Z         	Error:      	Not equal: 
2022-06-27T12:42:43.5662766Z         	            	expected: &config.Config{Options:base.Options{Console:false, Verbose:false, PProfPort:0, Telemetry:base.TelemetryOption{Jaeger:"", ServiceName:""}}, Server:(*config.ServerConfig)(0xc00005b840), Database:(*config.DatabaseConfig)(0xc000338330), Cache:(*config.CacheConfig)(0xc000338340), ObjectStorage:(*config.ObjectStorageConfig)(0xc000012900), Metrics:(*config.MetricsConfig)(0xc00004f4c0)}
2022-06-27T12:42:43.5666560Z         	            	actual  : &config.Config{Options:base.Options{Console:false, Verbose:false, PProfPort:0, Telemetry:base.TelemetryOption{Jaeger:"", ServiceName:""}}, Server:(*config.ServerConfig)(0xc00005bb00), Database:(*config.DatabaseConfig)(0xc000339610), Cache:(*config.CacheConfig)(0xc000339b20), ObjectStorage:(*config.ObjectStorageConfig)(0xc000012fc0), Metrics:(*config.MetricsConfig)(0xc00004fc80)}
2022-06-27T12:42:43.5667595Z         	            	
2022-06-27T12:42:43.5667961Z         	            	Diff:
2022-06-27T12:42:43.5668504Z         	            	--- Expected
2022-06-27T12:42:43.5668899Z         	            	+++ Actual
2022-06-27T12:42:43.5669429Z         	            	@@ -33,3 +33,3 @@
2022-06-27T12:42:43.5669999Z         	            	    Migrate: (bool) false,
2022-06-27T12:42:43.5670858Z         	            	-   TLSConfig: (string) (len=9) "preferred",
2022-06-27T12:42:43.5671408Z         	            	+   TLSConfig: (string) "",
2022-06-27T12:42:43.5671986Z         	            	    TLS: (*config.TLSConfig)({
2022-06-27T12:42:43.5672378Z         	Test:       	TestManagerConfig_Load
2022-06-27T12:42:43.5672763Z --- FAIL: TestManagerConfig_Load (0.01s)
2022-06-27T12:42:43.5673014Z FAIL

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

embroede · 2022-06-27T20:47:55Z

@gaius-qi Sorry about that, fixed.

gaius-qi

LGTM

cndoit18

LGTM

hyy0322

LGTM

…ion. i.e. tls=preferred (#1300) * Default to tls=preferred for mysql connection For communication from manager to mysql server, use `tls=preferred` parameter to enable TLS whenever possible. Signed-off-by: Edward Broeder <eddie.broeder@intel.com> * Make mysql tls parameter configurable Allow the user to specify the tls setting for the mysql connection. An example would be setting tls to "preferred", or "true". This is separate to the tlsConfig config parameter, which is used to set up a custom tls config, where tls key/certs are specified. See the tls parameter section in the below link: https://pkg.go.dev/github.com/go-sql-driver/mysql#section-readme Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

embroede requested a review from a team as a code owner May 11, 2022 06:44

embroede requested review from ansinlee, hyy0322 and zzy987 May 11, 2022 06:44

Default to tls=preferred for mysql connection

9309fcc

For communication from manager to mysql server, use `tls=preferred` parameter to enable TLS whenever possible. Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

embroede changed the title ~~Default to tls=preferred for mysql connection~~ feat: default to tls=preferred for mysql connection May 11, 2022

jim3ma requested a review from gaius-qi May 11, 2022 09:21

gaius-qi linked an issue May 18, 2022 that may be closed by this pull request

Manager counts the surviving number of dfdaemons #1313

Closed

gaius-qi removed a link to an issue May 18, 2022

Manager counts the surviving number of dfdaemons #1313

Closed

gaius-qi force-pushed the main branch from a083d6d to 5c4d965 Compare May 24, 2022 14:33

gaius-qi requested changes May 30, 2022

View reviewed changes

manager/config/config.go Outdated Show resolved Hide resolved

gaius-qi requested changes May 30, 2022

View reviewed changes

manager/config/config_test.go Outdated Show resolved Hide resolved

manager/database/mysql.go Outdated Show resolved Hide resolved

Rename config parameters

8d0a6be

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

embroede changed the title ~~feat: default to tls=preferred for mysql connection~~ feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred Jun 9, 2022

embroede added 2 commits June 9, 2022 16:35

Use mysqlCfg.TLSConfig instead of mysqlCfg.Params

14a92c3

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

Format config_test.go with gofmt -s

3cd74ed

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

Add test data for new tlsConfig parameter

ad6094c

Signed-off-by: Edward Broeder <eddie.broeder@intel.com>

gaius-qi approved these changes Jun 28, 2022

View reviewed changes

cndoit18 approved these changes Jun 28, 2022

View reviewed changes

hyy0322 approved these changes Jun 28, 2022

View reviewed changes

gaius-qi merged commit 4830796 into dragonflyoss:main Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred #1300

feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred #1300

embroede commented May 11, 2022 •

edited

gaius-qi commented May 12, 2022

embroede commented May 12, 2022

gaius-qi commented May 16, 2022

embroede commented May 27, 2022 •

edited

gaius-qi commented May 30, 2022

gaius-qi commented Jun 27, 2022

embroede commented Jun 27, 2022

gaius-qi left a comment

cndoit18 left a comment

hyy0322 left a comment

feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred #1300

feat: enable configuration of the tls parameter for the mysql connection. i.e. tls=preferred #1300

Conversation

embroede commented May 11, 2022 • edited

Description

Related Issue

Motivation and Context

Types of changes

Checklist

gaius-qi commented May 12, 2022

embroede commented May 12, 2022

gaius-qi commented May 16, 2022

embroede commented May 27, 2022 • edited

gaius-qi commented May 30, 2022

gaius-qi commented Jun 27, 2022

embroede commented Jun 27, 2022

gaius-qi left a comment

Choose a reason for hiding this comment

cndoit18 left a comment

Choose a reason for hiding this comment

hyy0322 left a comment

Choose a reason for hiding this comment

embroede commented May 11, 2022 •

edited

embroede commented May 27, 2022 •

edited