Support Operator to Add/delete User for PostgreSQL database #1097
Conversation
buhongw7583c
requested review from
davefellows,
frodopwns,
jananivMS and
WilliamMortlMicrosoft
as code owners
…azure-service-operator-1 into UserManagerPgreSQL
buhongw7583c
changed the title
|
Hi Hong! I think we are missing a few files for adding a new operator. Kubebuilder generates these files for us so we don't have to manually create them! Here is our documentation for creating a new operator with kubebuilder. Here are the files we need to declare the PostgreSQLUser in from KubeBuilder:
In step 12 of the documentation I mentioned above, we update the helm chart to include the new operator (PostgreSqlUser). We also need these two files as well
|
|
We also need to add the following inside the controller files:
|
There was a problem hiding this comment.
Hi Hong!
I tested the Happy Path and it worked! Successfully created a user on top of the database, as well as deleting successfully! I tested this out by commenting out our optional fields of adminsecret, adminsecretkeyvault and username.
I am requesting changes so we can address the missing files from KubeBuilder, add tests in the controller, and address the following edge case I ran into.
If we create a PostgreSqlUser with a improperly connected IP address, we run into the following zapperr in the reconciler. It continues to loop with the following message.
"}, "reason": "FailedReconcile", "message": "Failed to reconcile resource: 1 error occurred:\n\t* pq: no pg_hba.conf entry for host \"73.181.19.189\", user \"yc5jvKen\", database \"postgresqldatabase-sample-m\", SSL on\n\n"}
2020-05-27T16:38:44.958-0600 ERROR controller-runtime.controller Reconciler error {"controller": "postgresqluser", "request": "default/psqluser-sample-meltwo", "error": "1 error occurred:\n\t* pq: no pg_hba.conf entry for host \"73.181.19.189\", user \"yc5jvKen\", database \"postgresqldatabase-sample-m\", SSL on\n\n"}
github.com/go-logr/zapr.(*zapLogger).Error
I tested this out in AzureSqlUser to see if we would have the same behavior, and it does not have this zaperr in the reconcile. Here are the two status messages for comparison:
PostgreSqlUser:
psqluser-sample-meltwo pq: no pg_hba.conf entry for host "73.181.19.189", user "yc5jvKen", database "postgresqldatabase-sample-m", SSL on
AzureSqlUser
sqluser-sample Login error: mssql: Cannot open server 'sqlservermeltest' requested by the login. Client with IP address '73.181.19.189' is not allowed to access the server. To enable access, use the Windows Azure Management Portal or run sp_set_firewall_rule on the master database to create a firewall rule for this IP address or address range. It may take up to five minutes for this change to take effect.
We should try to handle this err in the reconciler somehow
Let me know if this makes sense or not! Thanks!
@melonrush13 Mel, thanks so much for the great comments! I do missed several important files and the document mentioned is definitely a great help! All the comments and suggestions have been addressed and just one left: |
@melonrush13 Mel, sure it makes sense! I did encounter the similar issue during my test but somehow it slipped from my mind and forgot to add handlings. Now the error could be caught up and wont produce the zapperr. But the error message returned from the sdk was quite confusing and indeed it indicate that your client IP address is not permitted to access, therefore I added more information in the returned message. |
…; add controller test for postgresqluser; add handling for firewall access erro.
…ser_controller_test.go
Hi Hong! So these two rbac files are generated by KubeBuilder when you do the Also, we began discussing this on Teams as Janani also noticed not every controller has these rbac files like you mentioned. We think this might be a difference in kubebuilder versions. Currently if you create a new operator with the latest version of kubebuilder, it will create these rbac files. When you created the operator with Kubebuilder did it generate these files for you? |
There was a problem hiding this comment.
Awesome! Thanks for all your work on this one, Hong!
- Was able to successfully create the user/delete the user
- Create successfully with keyvault set from adminSecret/adminSecretKeyVault
- Create successfully with keyVaultToStoreSecrets
Looks good to me. :) Passing on to an approved reviewer so we can get merged and another set of eyes!
this is superfluous
|
Also one other thing, @buhongw7583c , we should update the docs here to account for this new operator. Feel free to address this in another PR if you like (same comment for MySql, too!) |
| //for US government <server>.postgres.database.usgovcloudapi.net | ||
| fullServerAddress := fmt.Sprintf("%s.postgres."+psqldbdnssuffix, server) | ||
|
|
||
| connString := fmt.Sprintf("host=%s user=%s password=%s port=%d dbname=%s sslmode=require connect_timeout=30", fullServerAddress, user, password, port, database) |
There was a problem hiding this comment.
we should consider putting a conn string in the secret too
| // delete all the custom formatted secrets if keyvault is in use | ||
| keyVaultEnabled := reflect.TypeOf(secretClient).Elem().Name() == "KeyvaultSecretClient" | ||
| if keyVaultEnabled { | ||
| customFormatNames := []string{ | ||
| "adonet", | ||
| "adonet-urlonly", | ||
| "jdbc", | ||
| "jdbc-urlonly", | ||
| "odbc", | ||
| "odbc-urlonly", | ||
| "server", | ||
| "database", | ||
| "username", | ||
| "password", | ||
| } | ||
|
|
||
| for _, formatName := range customFormatNames { | ||
| key := types.NamespacedName{Namespace: secretKey.Namespace, Name: instance.Name + "-" + formatName} | ||
|
|
||
| err = secretClient.Delete( | ||
| ctx, | ||
| key, | ||
| ) | ||
| if err != nil { | ||
| instance.Status.Message = "failed to delete secret, err: " + err.Error() | ||
| return false, err | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We should remove the concept of custom secret formatting. It probably should not have been included in AzureSQLUser and is in need of some work (some is already done in a PR that is awaiting review)
There was a problem hiding this comment.
I only left this comment to confirm with you since the code change is big. if we remove the custom secret formating, so we would not allow user to use keyVaultSecretPrefix but we still allow customer to use keyvault to store the secret, right?
| keyVaultEnabled := reflect.TypeOf(secretClient).Elem().Name() == "KeyvaultSecretClient" | ||
|
|
||
| if keyVaultEnabled { | ||
| // For a keyvault secret store, check for supplied namespace parameters | ||
| var dbUserCustomNamespace string | ||
| if instance.Spec.KeyVaultSecretPrefix != "" { | ||
| dbUserCustomNamespace = instance.Spec.KeyVaultSecretPrefix | ||
| } else { | ||
| dbUserCustomNamespace = "psqluser-" + instance.Spec.Server + "-" + instance.Spec.DbName | ||
| } | ||
|
|
||
| namespacedName = types.NamespacedName{Namespace: dbUserCustomNamespace, Name: instance.Name} | ||
| } else { | ||
| namespacedName = types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace} | ||
| } | ||
|
|
There was a problem hiding this comment.
This isn't going to work. The operator should have no concept of what type of secret store is being used. This is a problem in AzureSQL User too and that is why there is a PR out to refactor it.
I think we might be able to remove the prefix idea from this operator.
There was a problem hiding this comment.
same question: we would remove both the keyVaultToStoreSecrets and keyVaultSecretPrefix?
| // Preformatted special formats are only available through keyvault as they require separated secrets | ||
| keyVaultEnabled := reflect.TypeOf(sqlUserSecretClient).Elem().Name() == "KeyvaultSecretClient" | ||
| if keyVaultEnabled { | ||
| // Instantiate a map of all formats and flip the bool to true for any that have been requested in the spec. | ||
| // Formats that were not requested will be explicitly deleted. | ||
| requestedFormats := map[string]bool{ | ||
| "adonet": false, | ||
| "adonet-urlonly": false, | ||
| "jdbc": false, | ||
| "jdbc-urlonly": false, | ||
| "odbc": false, | ||
| "odbc-urlonly": false, | ||
| "server": false, | ||
| "database": false, | ||
| "username": false, | ||
| "password": false, | ||
| } | ||
| for _, format := range instance.Spec.KeyVaultSecretFormats { | ||
| requestedFormats[format] = true | ||
| } | ||
|
|
||
| // Deleted items will be processed immediately but secrets that need to be added will be created in this array and persisted in one pass at the end | ||
| formattedSecrets := make(map[string][]byte) | ||
|
|
||
| for formatName, requested := range requestedFormats { | ||
| // Add the format to the output map if it has been requested otherwise call for its deletion from the secret store | ||
| if requested { | ||
| switch formatName { | ||
| case "adonet": | ||
| formattedSecrets["adonet"] = []byte(fmt.Sprintf( | ||
| "Server=tcp:%v,1433;Initial Catalog=%v;Persist Security Info=False;User ID=%v;Password=%v;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=False;Connection Timeout=30;", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| user, | ||
| string(DBSecret["password"]), | ||
| )) | ||
|
|
||
| case "adonet-urlonly": | ||
| formattedSecrets["adonet-urlonly"] = []byte(fmt.Sprintf( | ||
| "Server=tcp:%v,1433;Initial Catalog=%v;Persist Security Info=False; MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=False;Connection Timeout", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| )) | ||
|
|
||
| case "jdbc": | ||
| formattedSecrets["jdbc"] = []byte(fmt.Sprintf( | ||
| "jdbc:sqlserver://%v:1433;database=%v;user=%v@%v;password=%v;encrypt=true;trustServerCertificate=false;hostNameInCertificate=*."+config.Environment().SQLDatabaseDNSSuffix+";loginTimeout=30;", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| user, | ||
| instance.Spec.Server, | ||
| string(DBSecret["password"]), | ||
| )) | ||
| case "jdbc-urlonly": | ||
| formattedSecrets["jdbc-urlonly"] = []byte(fmt.Sprintf( | ||
| "jdbc:sqlserver://%v:1433;database=%v;encrypt=true;trustServerCertificate=false;hostNameInCertificate=*."+config.Environment().SQLDatabaseDNSSuffix+";loginTimeout=30;", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| )) | ||
|
|
||
| case "odbc": | ||
| formattedSecrets["odbc"] = []byte(fmt.Sprintf( | ||
| "Server=tcp:%v,1433;Initial Catalog=%v;Persist Security Info=False;User ID=%v;Password=%v;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=False;Connection Timeout=30;", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| user, | ||
| string(DBSecret["password"]), | ||
| )) | ||
| case "odbc-urlonly": | ||
| formattedSecrets["odbc-urlonly"] = []byte(fmt.Sprintf( | ||
| "Driver={ODBC Driver 13 for SQL Server};Server=tcp:%v,1433;Database=%v; Encrypt=yes;TrustServerCertificate=no;Connection Timeout=30;", | ||
| string(DBSecret["fullyQualifiedServerName"]), | ||
| instance.Spec.DbName, | ||
| )) | ||
| case "server": | ||
| formattedSecrets["server"] = DBSecret["fullyQualifiedServerName"] | ||
|
|
||
| case "database": | ||
| formattedSecrets["database"] = []byte(instance.Spec.DbName) | ||
|
|
||
| case "username": | ||
| formattedSecrets["username"] = []byte(user) | ||
|
|
||
| case "password": | ||
| formattedSecrets["password"] = DBSecret["password"] | ||
| } | ||
| } else { | ||
| err = sqlUserSecretClient.Delete( | ||
| ctx, | ||
| types.NamespacedName{Namespace: key.Namespace, Name: instance.Name + "-" + formatName}, | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd like to drop all this secret formatting stuff. There is a PR to refactor it in AzureSQL and I don't want to introduce this feature until its fixed and/or required by a user.
…eve username and full server name, move the common function to helpers, change to v2 version for postgresqlserver in controllers.
…azure-service-operator-1 into UserManagerPgreSQL
Remove conn string formatting and harden against sql injection
Closes #1056
What this PR does / why we need it:
Support the function to add users with specific roles to Postgresql database.
--Be able to add a user with a predefined roles and delete the user.
-- Able to delete a user under the postgresql database.
Special notes for your reviewer:
The Azure Database for PostgreSQL server is created with the 3 default roles defined.
--azure_pg_admin
--azure_superuser
--your server admin user
The server admin user account can be used to create additional users and grant those users into the azure_pg_admin role.
Also, the server admin account can be used to create less privileged users and roles that have access to individual databases and schemas but this function is not included in this PR.
To verify the PR:
--Use the config/samples/azure_v1alpha1_postgresql_everything.yaml to config pgresql server, database and firewallrule. Ensure at least your local client has the access to azure.
-- Apply the sample file config/samples/azure_v1alpha1_postgresqluser.yaml to add a user with a predefined role.
Use any tool to connect the postgresqlServer (e.g. pgadmin). The default server admin password could be derived from kubectl get secret -o yaml. By default the secret name is the pgresqlserver name.
-- Delete the newly added user by
kubect delete postgresqluser <metatdata name>. Note the username is the metadata name applied when adding the pgresql user but not the actual pgresql user name.From the pgadmin tool, the user has been deleted.
How does this PR make you feel:
If applicable: