K8SPXC-377: Add VS support and bug fix (percona#1882)

* Send UserManagementEnabled to VS. * Set default VS_BRANCH env var value. * Fix updated user check. Add validation for empty username. * Check properly user secret pass hash. * Return nil if user is not found in the DB. * Fix checking if rows is empty and update e2e test. * Fix checking for empty rows. * Fix e2e test. * Fix comparing hosts and dbs. --------- Co-authored-by: Viacheslav Sarzhan <[email protected]>
dcaputo-harmoni · Dec 6, 2024 · cc52062 · cc52062
1 parent ad71ccb
commit cc52062
Show file tree

Hide file tree

Showing 33 changed files with 3,257 additions and 197 deletions.
diff --git a/Makefile b/Makefile
@@ -101,6 +101,10 @@ ENVTEST = $(shell pwd)/bin/setup-envtest
 envtest: ## Download envtest-setup locally if necessary.
 	$(call go-get-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@latest)
 
+SWAGGER = $(shell pwd)/bin/swagger
+swagger: ## Download swagger locally if necessary.
+	$(call go-get-tool,$(SWAGGER),github.com/go-swagger/go-swagger/cmd/swagger@latest)
+
 # Prepare release
 CERT_MANAGER_VER := $(shell grep -Eo "cert-manager v.*" go.mod|grep -Eo "[0-9]+\.[0-9]+\.[0-9]+")
 release: manifests
@@ -141,3 +145,16 @@ after-release: manifests
 		-e "/^  backup:/,/^    image:/{s#image: .*#image: perconalab/percona-xtradb-cluster-operator:main-pxc8.0-backup#}" \
 		-e "/initContainer:/,/image:/{s#image: .*#image: perconalab/percona-xtradb-cluster-operator:main#}" \
 		-e "/^  pmm:/,/^    image:/{s#image: .*#image: perconalab/pmm-client:dev-latest#}" deploy/cr.yaml
+
+VS_BRANCH = main
+version-service-client: swagger
+	curl https://raw.githubusercontent.com/Percona-Lab/percona-version-service/$(VS_BRANCH)/api/version.swagger.yaml \
+		--output ./version.swagger.yaml
+	rm -rf ./version/client
+	mkdir -p ./version/client/models
+	mkdir -p ./version/client/version_service
+	./bin/swagger generate client \
+		-f ./version.swagger.yaml \
+		-c ./version/client \
+		-m ./version/client/models
+	rm ./version.swagger.yaml
diff --git a/e2e-tests/custom-users/run b/e2e-tests/custom-users/run
@@ -37,6 +37,15 @@ compare_mysql_user "-h $cluster-haproxy -uuser-one -pnew-password"
 compare_mysql_cmd "user-one-1" "SELECT User, Host from mysql.user WHERE User = 'user-one';" "-h $cluster-haproxy -uroot -proot_password"
 compare_mysql_cmd "user-one-2" "SHOW GRANTS FOR 'user-one'@'127.0.0.1';" "-h $cluster-haproxy -uroot -proot_password"
 
+desc 'check removing secretPasswordRef from user generates user password stored in generated custom-user-secret'
+kubectl_bin patch pxc some-name \
+  --type=json \
+  -p='[{"op": "replace", "path": "/spec/users/1/passwordSecretRef", "value": null}]'
+wait_cluster_consistency "$cluster" 3 3
+
+userTwoPass=$(kubectl_bin get secret $generatedUserSecret -o jsonpath="{.data.user-two}" | base64 -d)
+compare_mysql_user "-h $cluster-haproxy -uuser-two -p'$userTwoPass'"
+
 desc 'delete initial users from CR and create a new one'
 kubectl_bin patch pxc some-name --type=merge -p='{
 		"spec": {"users":[
@@ -57,7 +66,7 @@ compare_mysql_cmd "user-four-1" "SELECT User, Host from mysql.user WHERE User =
 
 # user-one, user-two and three should not be deleted
 compare_mysql_user "-h $cluster-haproxy -uuser-one -pnew-password"
-compare_mysql_user "-h $cluster-haproxy -uuser-two -ptestpass3"
+compare_mysql_user "-h $cluster-haproxy -uuser-two -p'$userTwoPass'"
 
 desc 'check user DBs updated'
 kubectl_bin patch pxc some-name --type=merge -p='{

diff --git a/pkg/controller/pxc/users_custom.go b/pkg/controller/pxc/users_custom.go
@@ -51,6 +51,11 @@ func (r *ReconcilePerconaXtraDBCluster) reconcileCustomUsers(ctx context.Context
 	sysUserNames := sysUserNames()
 
 	for _, user := range cr.Spec.Users {
+		if user.Name == "" {
+			log.Error(nil, "user name is not set", "user", user)
+			continue
+		}
+
 		if _, ok := sysUserNames[user.Name]; ok {
 			log.Error(nil, "creating user with reserved user name is forbidden", "user", user.Name)
 			continue
@@ -91,7 +96,13 @@ func (r *ReconcilePerconaXtraDBCluster) reconcileCustomUsers(ctx context.Context
 
 		annotationKey := fmt.Sprintf("percona.com/%s-%s-hash", cr.Name, user.Name)
 
-		if userPasswordChanged(userSecret, annotationKey, userSecretPassKey) {
+		u, err := um.GetUser(ctx, user.Name)
+		if err != nil {
+			log.Error(err, "failed to get user", "user", user)
+			continue
+		}
+
+		if userPasswordChanged(userSecret, u, annotationKey, userSecretPassKey) {
 			log.Info("User password changed", "user", user.Name)
 
 			err := um.UpsertUser(ctx, alterUserQuery(&user), string(userSecret.Data[userSecretPassKey]))
@@ -109,12 +120,6 @@ func (r *ReconcilePerconaXtraDBCluster) reconcileCustomUsers(ctx context.Context
 			log.Info("User password updated", "user", user.Name)
 		}
 
-		u, err := um.GetUser(ctx, user.Name)
-		if err != nil {
-			log.Error(err, "failed to get user", "user", user)
-			continue
-		}
-
 		if userChanged(u, &user, log) {
 			log.Info("Creating/updating user", "user", user.Name)
 
@@ -165,18 +170,19 @@ func generateUserPass(
 	return nil
 }
 
-func userPasswordChanged(secret *corev1.Secret, key, passKey string) bool {
+func userPasswordChanged(secret *corev1.Secret, dbUser *users.User, key, passKey string) bool {
 	if secret.Annotations == nil {
 		return false
 	}
 
 	hash, ok := secret.Annotations[key]
 	if !ok {
-		return false
+		// If annotation is not present in the secret and the user is created (not nil),
+		// we assume that password has changed.
+		return dbUser != nil
 	}
 
 	newHash := sha256Hash(secret.Data[passKey])
-
 	return hash != newHash
 }
 
@@ -188,16 +194,6 @@ func userChanged(current *users.User, desired *api.User, log logr.Logger) bool {
 		return true
 	}
 
-	if len(current.Hosts) != len(desired.Hosts) {
-		log.Info("Hosts changed", "current", current.Hosts, "desired", desired.Hosts, "user", userName)
-		return true
-	}
-
-	if len(current.DBs) != len(desired.DBs) {
-		log.Info("DBs changed", "current", current.DBs, "desired", desired.DBs)
-		return true
-	}
-
 	for _, u := range desired.Hosts {
 		if !current.Hosts.Has(u) {
 			log.Info("Hosts changed", "current", current.Hosts, "desired", desired.Hosts, "user", userName)

diff --git a/pkg/controller/pxc/version.go b/pkg/controller/pxc/version.go
@@ -237,17 +237,18 @@ func (r *ReconcilePerconaXtraDBCluster) getNewVersions(ctx context.Context, cr *
 	}
 
 	vm := versionMeta{
-		Apply:               cr.Spec.UpgradeOptions.Apply,
-		Platform:            string(cr.Spec.Platform),
-		KubeVersion:         r.serverVersion.Info.GitVersion,
-		PXCVersion:          cr.Status.PXC.Version,
-		PMMVersion:          cr.Status.PMM.Version,
-		HAProxyVersion:      cr.Status.HAProxy.Version,
-		ProxySQLVersion:     cr.Status.ProxySQL.Version,
-		BackupVersion:       cr.Status.Backup.Version,
-		LogCollectorVersion: cr.Status.LogCollector.Version,
-		CRUID:               string(cr.GetUID()),
-		ClusterWideEnabled:  watchNs == "",
+		Apply:                 cr.Spec.UpgradeOptions.Apply,
+		Platform:              string(cr.Spec.Platform),
+		KubeVersion:           r.serverVersion.Info.GitVersion,
+		PXCVersion:            cr.Status.PXC.Version,
+		PMMVersion:            cr.Status.PMM.Version,
+		HAProxyVersion:        cr.Status.HAProxy.Version,
+		ProxySQLVersion:       cr.Status.ProxySQL.Version,
+		BackupVersion:         cr.Status.Backup.Version,
+		LogCollectorVersion:   cr.Status.LogCollector.Version,
+		CRUID:                 string(cr.GetUID()),
+		ClusterWideEnabled:    watchNs == "",
+		UserManagementEnabled: len(cr.Spec.Users) > 0,
 	}
 
 	endpoint := apiv1.GetDefaultVersionServiceEndpoint()

diff --git a/pkg/controller/pxc/vs.go b/pkg/controller/pxc/vs.go
@@ -31,22 +31,23 @@ func (vs VersionServiceClient) GetExactVersion(cr *api.PerconaXtraDBCluster, end
 	})
 
 	applyParams := &version_service.VersionServiceApplyParams{
-		Apply:               vm.Apply,
-		BackupVersion:       &vm.BackupVersion,
-		CustomResourceUID:   &vm.CRUID,
-		DatabaseVersion:     &vm.PXCVersion,
-		HaproxyVersion:      &vm.HAProxyVersion,
-		KubeVersion:         &vm.KubeVersion,
-		LogCollectorVersion: &vm.LogCollectorVersion,
-		NamespaceUID:        new(string),
-		OperatorVersion:     cr.Spec.CRVersion,
-		Platform:            &vm.Platform,
-		PmmVersion:          &vm.PMMVersion,
-		Product:             productName,
-		ProxysqlVersion:     &vm.ProxySQLVersion,
-		Context:             nil,
-		ClusterWideEnabled:  &vm.ClusterWideEnabled,
-		HTTPClient:          &http.Client{Timeout: 10 * time.Second},
+		Apply:                 vm.Apply,
+		BackupVersion:         &vm.BackupVersion,
+		CustomResourceUID:     &vm.CRUID,
+		DatabaseVersion:       &vm.PXCVersion,
+		HaproxyVersion:        &vm.HAProxyVersion,
+		KubeVersion:           &vm.KubeVersion,
+		LogCollectorVersion:   &vm.LogCollectorVersion,
+		NamespaceUID:          new(string),
+		OperatorVersion:       cr.Spec.CRVersion,
+		Platform:              &vm.Platform,
+		PmmVersion:            &vm.PMMVersion,
+		Product:               productName,
+		ProxysqlVersion:       &vm.ProxySQLVersion,
+		Context:               nil,
+		ClusterWideEnabled:    &vm.ClusterWideEnabled,
+		HTTPClient:            &http.Client{Timeout: 10 * time.Second},
+		UserManagementEnabled: &vm.UserManagementEnabled,
 	}
 	applyParams = applyParams.WithTimeout(10 * time.Second)
 
@@ -151,15 +152,16 @@ type VersionServiceClient struct {
 }
 
 type versionMeta struct {
-	Apply               string
-	PXCVersion          string
-	KubeVersion         string
-	Platform            string
-	PMMVersion          string
-	BackupVersion       string
-	ProxySQLVersion     string
-	HAProxyVersion      string
-	LogCollectorVersion string
-	CRUID               string
-	ClusterWideEnabled  bool
+	Apply                 string
+	PXCVersion            string
+	KubeVersion           string
+	Platform              string
+	PMMVersion            string
+	BackupVersion         string
+	ProxySQLVersion       string
+	HAProxyVersion        string
+	LogCollectorVersion   string
+	CRUID                 string
+	ClusterWideEnabled    bool
+	UserManagementEnabled bool
 }
diff --git a/pkg/pxc/users/users.go b/pkg/pxc/users/users.go
@@ -335,6 +335,8 @@ func (p *Manager) GetUser(ctx context.Context, user string) (*User, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer rows.Close()
+
 	for rows.Next() {
 		var host string
 		var db sql.NullString
@@ -349,11 +351,16 @@ func (p *Manager) GetUser(ctx context.Context, user string) (*User, error) {
 		u.Hosts.Insert(host)
 	}
 
+	if len(u.Hosts) == 0 {
+		return nil, nil
+	}
+
 	for host := range u.Hosts {
 		rows, err := p.db.QueryContext(ctx, "SHOW GRANTS FOR ?@?", user, host)
 		if err != nil {
 			return nil, err
 		}
+		// Plus 1 is for the default grant every user has, which is USAGE.
 		grants := make([]string, 0, len(u.DBs)+1)
 		for rows.Next() {
 			var grant string

diff --git a/version/client/models/googlerpc_status.go b/version/client/models/googlerpc_status.go