Skip to content

feat(backend): add postgres initialization #9798

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Aug 10, 2023
Merged

feat(backend): add postgres initialization #9798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
14773d0
add postgres initialization
Linchin Jul 28, 2023
e2c0933
remove load balancer
Linchin Jul 28, 2023
e81fea2
go mod tidy
Linchin Jul 31, 2023
bd6c7f5
update license
Linchin Aug 1, 2023
408e3c2
license update for viewer
Linchin Aug 1, 2023
6c7afd2
(test) disable controller license check
Linchin Aug 1, 2023
52d6dcf
(test) disable persistence agence licence check
Linchin Aug 2, 2023
a73d6dd
(test) disable scheduled workflow license check
Linchin Aug 2, 2023
70ccd14
(test) disable cacheserver license check
Linchin Aug 2, 2023
7356e14
fix db config location
Linchin Aug 2, 2023
5b8b24a
fix mysql support
Linchin Aug 2, 2023
5787bfb
test
Linchin Aug 2, 2023
8c4530d
test
Linchin Aug 3, 2023
bc53cb1
no long set host address
Linchin Aug 3, 2023
85223dd
Merge remote-tracking branch 'origin/master' into db-init
Linchin Aug 8, 2023
84b9157
address comments
Linchin Aug 8, 2023
df43a9b
address comments and enable license check
Linchin Aug 8, 2023
fbf3d11
format
Linchin Aug 8, 2023
3764044
remove extra blank line
Linchin Aug 9, 2023
4fa2fe1
update licenses
Linchin Aug 9, 2023
1566b21
cache server license
Linchin Aug 10, 2023
b5127b1
address comments
Linchin Aug 10, 2023
bb3d1cb
centralize error message
Linchin Aug 10, 2023
af3b299
remove pv in postgres deployment
Linchin Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions backend/src/apiserver/client/sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,22 @@
package client

import (
"bytes"
"fmt"

"github.com/go-sql-driver/mysql"
)

func CreateMySQLConfig(user, password string, mysqlServiceHost string,
mysqlServicePort string, dbName string, mysqlGroupConcatMaxLen string, mysqlExtraParams map[string]string,
const (
MYSQL_TEXT_FORMAT string = "longtext not null"
MYSQL_EXIST_ERROR string = "database exists"

PGX_TEXT_FORMAT string = "text"
PGX_EXIST_ERROR string = "already exists"
)

func CreateMySQLConfig(user, password, mysqlServiceHost, mysqlServicePort,
dbName, mysqlGroupConcatMaxLen string, mysqlExtraParams map[string]string,
) *mysql.Config {
params := map[string]string{
"charset": "utf8",
Expand All @@ -44,3 +53,26 @@ func CreateMySQLConfig(user, password string, mysqlServiceHost string,
AllowNativePasswords: true,
}
}

func CreatePostgreSQLConfig(user, password, postgresHost, dbName string, postgresPort uint16,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to introduce sslmode for security support in the coming PRs? It is important to make sure we are using secure channel when reading/writing to DB.

Reference: https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L714-L743

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be great to improve security, but I wonder if it is necessary, since the DB and the backend are hosted in the same cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

This allows scenario like connecting to a managed database service. We can discuss about this in future implementation.

) string {
var b bytes.Buffer
if dbName != "" {
fmt.Fprintf(&b, "database=%s ", dbName)
}
if user != "" {
fmt.Fprintf(&b, "user=%s ", user)
}
if password != "" {
fmt.Fprintf(&b, "password=%s ", password)
}
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide an option for passfile as well? Reference: https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L704

Copy link
Contributor Author

Choose a reason for hiding this comment

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

passfile sounds like a great way to enhance security. Could you tell me more about it? My guess is this will allow users to pass a file containing the password as part of the deployment config. Maybe this can be added together with other configs users can set.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is right! So the passfile identify the location path to a password, so you don't need to expose the secret via deployment. https://www.postgresql.org/docs/current/libpq-pgpass.html When passing this parameter to postgresql, the password field can be omitted.

We can add this configuration in the coming PR, if this makes sense to you.

if postgresHost != "" {
fmt.Fprintf(&b, "host=%s ", postgresHost)
}
Comment on lines +69 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide an option for hostaddr? https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L690

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this is necessary, since the host address here is simply for communications inside the cluster. Could you tell me more why this is beneficial? Thank you!

Copy link
Contributor

@zijianjoy zijianjoy Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure thing, hostaddr is helpful when we are connecting to external database, for example: CloudSQL. Yes we can use host which is handy for Unix-domain communication, but hostaddr allows postgresql client to make TCP/IP communication instead.

Again, we can implement this feature in future PR.

if postgresPort != 0 {
fmt.Fprintf(&b, "port=%d ", postgresPort)
}
fmt.Fprint(&b, "sslmode=disable")

return b.String()
}
Loading