Fix: Do not redact password for conn pool connection uri while connecting to DB #2203

priyanshi-yb · 2025-01-17T10:34:45Z

Describe the changes in this pull request

Fixing the bug where import data always fails to connect to DB via the connection pool with a password authentication as the connection URLs are getting redacted while passing to the connection pool.
#2204
redaction still happens-

2025-01-18 12:46:14.483348 INFO yugabytedb.go:252 Initialized connection pool with settings: (tgtdb.ConnectionParams) {
 NumConnections: (int) 2,
 NumMaxConnections: (int) 4,
 ConnUriList: ([]string) (len=1 cap=1) {
  (string) (len=81) "postgresql://ybvoyager:[email protected]:5433/pg_datatypes_offline?sslmode=prefer"
 },
 SessionInitScript: ([]string) (len=3 cap=4) {
  (string) (len=29) "SET client_encoding TO 'UTF8'",
  (string) (len=39) "SET session_replication_role TO replica",
  (string) (len=53) "SET default_transaction_isolation = 'repeatable read'"
 }
}

Deep copying the connection params for redacting while logging using the package - https://github.com/tiendc/go-deepcopy because of following reasons-

It is quite active as recent change is a month ago.
It claims to have better benchmarks for deep copying.

Describe if there are any user-facing changes

Its a fix, no user-facing changes

How was this pull request tested?

automation tests are enough

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	No
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

makalaaneesh · 2025-01-17T11:03:39Z

yb-voyager/src/tgtdb/yugabytedb.go

@@ -243,8 +243,12 @@ func (yb *TargetYugabyteDB) InitConnPool() error {
 		SessionInitScript: getYBSessionInitScript(yb.tconf),
 	}
 	yb.connPool = NewConnectionPool(params)
-	redactedParams := params
-	redactedParams.ConnUriList = utils.GetRedactedURLs(redactedParams.ConnUriList)
+	redactedParams := ConnectionParams{


We're logging the full struct below, so this will mean we'll have to come back to keep updating this copy here whenever we add a new field.

Let's go with redactedParams := *params ?

makalaaneesh · 2025-01-17T11:04:14Z

.github/workflows/pg-9-migtests.yml

@@ -85,7 +85,7 @@ jobs:
          docker run -d --name yugabytedb \
            -p7000:7000 -p9000:9000 -p15433:15433 -p5433:5433 -p9042:9042 \
            yugabytedb/yugabyte:${{ matrix.version }} \
-            bin/yugabyted start --background=false --ui=false
+            bin/yugabyted start --tserver_flags="ysql_hba_conf_csv={host all yugabyte all trust,host all all all md5}" --background=false --ui=false 


Shall we just have host all all all md5 ? Just want to avoid "trust" altogether 😅

yeah I started with that only, but somehow yugabyted wasn't coming up with that conf as yugabyte is a default user maybe trust is required for that, I can take that up later after figuring out how to do that

yb-voyager/src/tgtdb/yugabytedb.go

This reverts commit b54c79a.

makalaaneesh

LGTM

makalaaneesh · 2025-01-20T07:19:44Z

yb-voyager/src/tgtdb/yugabytedb.go

@@ -37,6 +37,7 @@ import (
 	_ "github.com/jackc/pgx/v5/stdlib"
 	"github.com/samber/lo"
 	log "github.com/sirupsen/logrus"
+	"github.com/tiendc/go-deepcopy"


Just as a general discussion, not suggesting you change it:
I'm guessing you picked this because it seems to be the fastest. In our case, though, we don't really care so much about speed (because we call it like once and log it). In such cases, it makes more sense to prioritize dependencies that are popular/ well maintained.

Not just fast, I also looked at activity on the repo and only this one seems quite active (last month), rest had last commit years ago

Plus to @makalaaneesh point. Popular ones can be safer.
Maybe if there is one with a lot of stars and some recent activities.

@priyanshi-yb @makalaaneesh lets fix a specific version of the library? v1/v2 whatever it is..

"github.com/tiendc/go-deepcopy/vX"

sanyamsinghal · 2025-01-20T07:59:38Z

yb-voyager/src/tgtdb/yugabytedb.go

-	redactedParams := params
+	redactedParams := &ConnectionParams{}
+	//Whenever adding new fields to CONNECTION PARAMS check if that needs to be redacted while logging
+	deepcopy.Copy(redactedParams, params)


handle the error if its returning..

…ting to DB (#2203) Fixing the bug where import data always fails to connect to DB via the connection pool with password authentication as the connection URLs are redacted while passing to the connection pool. Deep copying the connection params for redacting while logging using the package - https://github.com/tiendc/go-deepcopy.

…ting to DB (#2203) (#2208) Fixing the bug where import data always fails to connect to DB via the connection pool with password authentication as the connection URLs are redacted while passing to the connection pool. Deep copying the connection params for redacting while logging using the package - https://github.com/tiendc/go-deepcopy.

priyanshi-yb force-pushed the priyanshi/fix-conn-pool-pass branch from c4b1b2b to 2f05de5 Compare January 17, 2025 10:36

add md5 conf for passwords for yb cluster

a83acb2

priyanshi-yb force-pushed the priyanshi/fix-conn-pool-pass branch from 2f05de5 to a83acb2 Compare January 17, 2025 10:49

fix for not redacting connection uris for conn pool

00ada30

priyanshi-yb requested review from makalaaneesh and sanyamsinghal and removed request for makalaaneesh January 17, 2025 11:00

makalaaneesh reviewed Jan 17, 2025

View reviewed changes

review comment

b54c79a

priyanshi-yb requested a review from makalaaneesh January 17, 2025 11:58

sanyamsinghal requested changes Jan 17, 2025

View reviewed changes

yb-voyager/src/tgtdb/yugabytedb.go Outdated Show resolved Hide resolved

Revert "review comment"

4b36842

This reverts commit b54c79a.

priyanshi-yb requested a review from sanyamsinghal January 18, 2025 07:25

priyanshi-yb marked this pull request as ready for review January 20, 2025 04:03

priyanshi-yb added 3 commits January 20, 2025 11:56

deepcopy

b738426

add 2.25 to supported preview versions

f91cd7a

Merge branch 'main' into priyanshi/fix-conn-pool-pass

6aa224f

makalaaneesh approved these changes Jan 20, 2025

View reviewed changes

sanyamsinghal reviewed Jan 20, 2025

View reviewed changes

sanyamsinghal approved these changes Jan 20, 2025

View reviewed changes

handle error

979c379

priyanshi-yb force-pushed the priyanshi/fix-conn-pool-pass branch from b04d148 to 979c379 Compare January 20, 2025 08:09

priyanshi-yb merged commit 222b9d8 into main Jan 20, 2025
67 checks passed

priyanshi-yb deleted the priyanshi/fix-conn-pool-pass branch January 20, 2025 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Do not redact password for conn pool connection uri while connecting to DB #2203

Fix: Do not redact password for conn pool connection uri while connecting to DB #2203

priyanshi-yb commented Jan 17, 2025 •

edited

Loading

makalaaneesh Jan 17, 2025

makalaaneesh Jan 17, 2025

priyanshi-yb Jan 17, 2025

makalaaneesh left a comment

makalaaneesh Jan 20, 2025 •

edited

Loading

priyanshi-yb Jan 20, 2025 •

edited

Loading

sanyamsinghal Jan 20, 2025

sanyamsinghal Jan 20, 2025 •

edited

Loading

sanyamsinghal Jan 20, 2025

Fix: Do not redact password for conn pool connection uri while connecting to DB #2203

Fix: Do not redact password for conn pool connection uri while connecting to DB #2203

Conversation

priyanshi-yb commented Jan 17, 2025 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

makalaaneesh Jan 17, 2025

Choose a reason for hiding this comment

makalaaneesh Jan 17, 2025

Choose a reason for hiding this comment

priyanshi-yb Jan 17, 2025

Choose a reason for hiding this comment

makalaaneesh left a comment

Choose a reason for hiding this comment

makalaaneesh Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

priyanshi-yb Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

sanyamsinghal Jan 20, 2025

Choose a reason for hiding this comment

sanyamsinghal Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

sanyamsinghal Jan 20, 2025

Choose a reason for hiding this comment

priyanshi-yb commented Jan 17, 2025 •

edited

Loading

makalaaneesh Jan 20, 2025 •

edited

Loading

priyanshi-yb Jan 20, 2025 •

edited

Loading

sanyamsinghal Jan 20, 2025 •

edited

Loading