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

Ysql database plugin #2

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

Ysql database plugin #2

wants to merge 19 commits into from

Conversation

jayant07-yb
Copy link
Owner

Ysql database plugin will allow users for managing the roles for yugabyteDB(ysql) database via the HashiCorp Vault.
Details regarding the implementation are provided here
Details regarding using the plugin are provided here

jayant anand added 2 commits May 5, 2022 08:20
Added the plugin for ysql database
@jayant07-yb jayant07-yb changed the title Ysql database Ysql database plugin May 5, 2022
jayant anand added 3 commits May 5, 2022 09:36
@jayant07-yb jayant07-yb requested a review from ashetkar May 5, 2022 12:10
@jayant07-yb jayant07-yb added the enhancement New feature or request label May 5, 2022
Copy link
Collaborator

@amit-yb amit-yb left a comment

Choose a reason for hiding this comment

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

LGTM.

ImageRepo: "yugabytedb/yugabyte",
Cmd: []string{"./bin/yugabyted", "start", "--daemon=false"},
ImageTag: version,
Env: []string{"YSQL_DB=database", "YSQL_PASSWORD=secret"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about naming the default database as something like testdb?

Choose a reason for hiding this comment

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

Is this referring to ConatinerName? @jayant07-yb

Copy link
Owner Author

Choose a reason for hiding this comment

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

The database name in this line
Env: []string{"YSQL_DB=database", "YSQL_PASSWORD=secret"},
needs to be changed to
Env: []string{"YSQL_DB= testdb", "YSQL_PASSWORD=secret"},

Comment on lines +21 to +23
defaultExpirationStatement = `
ALTER ROLE "{{name}}" VALID UNTIL '{{expiration}}';
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Can't we keep the entire string on the same line?

defaultExpirationStatement = `ALTER ROLE ...`

Same for the statement on line 24?

connProducer.Type = yugabyteDBType

yugabyte := &ysql{
YugabyteConnectionProducer: connProducer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The connProducer object (that has lock object inside it) is being copied here. This is not recommended. Consider making the named YugabyteConnectionProducer a pointer field.

// the role
// This isn't done in a transaction because even if we fail along the way,
// we want to remove as much access as possible
stmt, err := db.PrepareContext(ctx, "SELECT DISTINCT table_schema FROM information_schema.role_column_grants WHERE grantee=$1;")
Copy link
Owner Author

Choose a reason for hiding this comment

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

Replace the query with

SELECT schema_name FROM information_schema.schemata;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants