From eb24059be2c7096e82b703a78bf83640d6ce35c3 Mon Sep 17 00:00:00 2001
From: hobu <hobu@Hongs-MacBook-Pro.local>
Date: Fri, 5 Jun 2020 19:35:33 +0800
Subject: [PATCH] optimize the user delete of AzureSQLUser

---
 .../azuresql/azuresqluser/azuresqluser.go     | 31 +++++-----------
 .../azuresqluser/azuresqluser_reconcile.go    | 36 ++++++++++++-------
 2 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go b/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go
index 14d71ff6890..cbd2dbe5828 100644
--- a/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go
+++ b/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go
@@ -110,10 +110,10 @@ func (s *AzureSqlUserManager) CreateUser(ctx context.Context, secret map[string]
 	newPassword := string(secret[SecretPasswordKey])
 
 	// make an effort to prevent sql injection
-	if err := findBadChars(newUser); err != nil {
+	if err := helpers.FindBadChars(newUser); err != nil {
 		return "", fmt.Errorf("Problem found with username: %v", err)
 	}
-	if err := findBadChars(newPassword); err != nil {
+	if err := helpers.FindBadChars(newPassword); err != nil {
 		return "", fmt.Errorf("Problem found with password: %v", err)
 	}
 
@@ -136,10 +136,10 @@ func (s *AzureSqlUserManager) UpdateUser(ctx context.Context, secret map[string]
 	newPassword := helpers.NewPassword()
 
 	// make an effort to prevent sql injection
-	if err := findBadChars(user); err != nil {
+	if err := helpers.FindBadChars(user); err != nil {
 		return fmt.Errorf("Problem found with username: %v", err)
 	}
-	if err := findBadChars(newPassword); err != nil {
+	if err := helpers.FindBadChars(newPassword); err != nil {
 		return fmt.Errorf("Problem found with password: %v", err)
 	}
 
@@ -165,8 +165,10 @@ func (s *AzureSqlUserManager) UserExists(ctx context.Context, db *sql.DB, userna
 
 // DropUser drops a user from db
 func (s *AzureSqlUserManager) DropUser(ctx context.Context, db *sql.DB, user string) error {
-	tsql := "DROP USER @user"
-	_, err := db.ExecContext(ctx, tsql, sql.Named("user", user))
+	//tsql := "DROP USER @user"
+	//_, err := db.ExecContext(ctx, tsql, sql.Named("user", user))
+	tsql := fmt.Sprintf("DROP USER %q", user)
+	_, err := db.ExecContext(ctx, tsql)
 	return err
 }
 
@@ -260,20 +262,3 @@ func GetNamespacedName(instance *v1alpha1.AzureSQLUser, secretClient secrets.Sec
 
 	return namespacedName
 }
-
-func findBadChars(stack string) error {
-	badChars := []string{
-		"'",
-		"\"",
-		";",
-		"--",
-		"/*",
-	}
-
-	for _, s := range badChars {
-		if idx := strings.Index(stack, s); idx > -1 {
-			return fmt.Errorf("potentially dangerous character seqience found: '%s' at pos: %d", s, idx)
-		}
-	}
-	return nil
-}
diff --git a/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go b/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go
index 729f0e80aab..cc0e5fff424 100644
--- a/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go
+++ b/pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go
@@ -310,13 +310,6 @@ func (s *AzureSqlUserManager) Delete(ctx context.Context, obj runtime.Object, op
 	}
 	key := types.NamespacedName{Name: adminsecretName, Namespace: instance.Namespace}
 
-	var sqlUserSecretClient secrets.SecretClient
-	if options.SecretClient != nil {
-		sqlUserSecretClient = options.SecretClient
-	} else {
-		sqlUserSecretClient = s.SecretClient
-	}
-
 	// if the admin secret keyvault is not specified, fall back to global secretclient
 	if len(instance.Spec.AdminSecretKeyVault) != 0 {
 		adminSecretClient = keyvaultSecrets.New(instance.Spec.AdminSecretKeyVault)
@@ -348,20 +341,36 @@ func (s *AzureSqlUserManager) Delete(ctx context.Context, obj runtime.Object, op
 		return false, err
 	}
 
-	var user = string(adminSecret[SecretUsernameKey])
-	var password = string(adminSecret[SecretPasswordKey])
+	var adminuser = string(adminSecret[SecretUsernameKey])
+	var adminpassword = string(adminSecret[SecretPasswordKey])
 
-	db, err := s.ConnectToSqlDb(ctx, DriverName, instance.Spec.Server, instance.Spec.DbName, SqlServerPort, user, password)
+	db, err := s.ConnectToSqlDb(ctx, DriverName, instance.Spec.Server, instance.Spec.DbName, SqlServerPort, adminuser, adminpassword)
 	if err != nil {
 		instance.Status.Message = errhelp.StripErrorIDs(err)
 		if strings.Contains(err.Error(), "create a firewall rule for this IP address") {
 
-			// there is nothing much we can do here - cycle forever
-			return true, err
+			// Stop the reconcile
+			return false, nil
 		}
 		return false, err
 	}
 
+	var sqlUserSecretClient secrets.SecretClient
+	if options.SecretClient != nil {
+		sqlUserSecretClient = options.SecretClient
+	} else {
+		sqlUserSecretClient = s.SecretClient
+	}
+
+	userkey := GetNamespacedName(instance, sqlUserSecretClient)
+	userSecret, err := sqlUserSecretClient.Get(ctx, userkey)
+	if err != nil {
+		// assuming if the admin secret is gone the sql server is too
+		return false, nil
+	}
+
+	user := string(userSecret[SecretUsernameKey])
+
 	exists, err := s.UserExists(ctx, db, user)
 	if err != nil {
 		return true, err
@@ -382,7 +391,8 @@ func (s *AzureSqlUserManager) Delete(ctx context.Context, obj runtime.Object, op
 
 	instance.Status.Message = fmt.Sprintf("Delete AzureSqlUser succeeded")
 
-	return true, nil
+	//successfully delete
+	return false, nil
 }
 
 // GetParents gets the parents of the user