deprecate ClusterName field of Postgresql type and remove team from REST endpoints (zalando#2015)

FxKu · web-flow · commit 4c07494ac731 · 2022-08-29T15:00:25.000+02:00
* deprecate ClusterName field of Postgresql type
* remove for teamId from operator API endpints /status /logs /history
* update dns_format_string and yaml template in UI
diff --git a/pkg/apis/acid.zalan.do/v1/postgresql_type.go b/pkg/apis/acid.zalan.do/v1/postgresql_type.go
@@ -36,6 +36,9 @@ type PostgresSpec struct {
 	TeamID      string `json:"teamId"`
 	DockerImage string `json:"dockerImage,omitempty"`
 
+	// deprecated field storing cluster name without teamId prefix
+	ClusterName string `json:"-"`
+
 	SpiloRunAsUser  *int64 `json:"spiloRunAsUser,omitempty"`
 	SpiloRunAsGroup *int64 `json:"spiloRunAsGroup,omitempty"`
 	SpiloFSGroup    *int64 `json:"spiloFSGroup,omitempty"`
@@ -62,7 +65,6 @@ type PostgresSpec struct {
 	NumberOfInstances     int32                       `json:"numberOfInstances"`
 	MaintenanceWindows    []MaintenanceWindow         `json:"maintenanceWindows,omitempty"`
 	Clone                 *CloneDescription           `json:"clone,omitempty"`
-	ClusterName           string                      `json:"-"`
 	Databases             map[string]string           `json:"databases,omitempty"`
 	PreparedDatabases     map[string]PreparedDatabase `json:"preparedDatabases,omitempty"`
 	SchedulerName         *string                     `json:"schedulerName,omitempty"`
diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go
@@ -31,9 +31,9 @@ type controllerInformer interface {
 	GetOperatorConfig() *config.Config
 	GetStatus() *spec.ControllerStatus
 	TeamClusterList() map[string][]spec.NamespacedName
-	ClusterStatus(team, namespace, cluster string) (*cluster.ClusterStatus, error)
-	ClusterLogs(team, namespace, cluster string) ([]*spec.LogEntry, error)
-	ClusterHistory(team, namespace, cluster string) ([]*spec.Diff, error)
+	ClusterStatus(namespace, cluster string) (*cluster.ClusterStatus, error)
+	ClusterLogs(namespace, cluster string) ([]*spec.LogEntry, error)
+	ClusterHistory(namespace, cluster string) ([]*spec.Diff, error)
 	ClusterDatabasesMap() map[string][]string
 	WorkerLogs(workerID uint32) ([]*spec.LogEntry, error)
 	ListQueue(workerID uint32) (*spec.QueueDump, error)
@@ -55,9 +55,9 @@ const (
 )
 
 var (
-	clusterStatusRe  = fmt.Sprintf(`^/clusters/%s/%s/%s/?$`, teamRe, namespaceRe, clusterRe)
-	clusterLogsRe    = fmt.Sprintf(`^/clusters/%s/%s/%s/logs/?$`, teamRe, namespaceRe, clusterRe)
-	clusterHistoryRe = fmt.Sprintf(`^/clusters/%s/%s/%s/history/?$`, teamRe, namespaceRe, clusterRe)
+	clusterStatusRe  = fmt.Sprintf(`^/clusters/%s/%s/?$`, namespaceRe, clusterRe)
+	clusterLogsRe    = fmt.Sprintf(`^/clusters/%s/%s/logs/?$`, namespaceRe, clusterRe)
+	clusterHistoryRe = fmt.Sprintf(`^/clusters/%s/%s/history/?$`, namespaceRe, clusterRe)
 	teamURLRe        = fmt.Sprintf(`^/clusters/%s/?$`, teamRe)
 
 	clusterStatusURL     = regexp.MustCompile(clusterStatusRe)
@@ -170,7 +170,7 @@ func (s *Server) clusters(w http.ResponseWriter, req *http.Request) {
 
 	if matches := util.FindNamedStringSubmatch(clusterStatusURL, req.URL.Path); matches != nil {
 		namespace := matches["namespace"]
-		resp, err = s.controller.ClusterStatus(matches["team"], namespace, matches["cluster"])
+		resp, err = s.controller.ClusterStatus(namespace, matches["cluster"])
 	} else if matches := util.FindNamedStringSubmatch(teamURL, req.URL.Path); matches != nil {
 		teamClusters := s.controller.TeamClusterList()
 		clusters, found := teamClusters[matches["team"]]
@@ -181,21 +181,21 @@ func (s *Server) clusters(w http.ResponseWriter, req *http.Request) {
 
 		clusterNames := make([]string, 0)
 		for _, cluster := range clusters {
-			clusterNames = append(clusterNames, cluster.Name[len(matches["team"])+1:])
+			clusterNames = append(clusterNames, cluster.Name)
 		}
 
 		resp, err = clusterNames, nil
 	} else if matches := util.FindNamedStringSubmatch(clusterLogsURL, req.URL.Path); matches != nil {
 		namespace := matches["namespace"]
-		resp, err = s.controller.ClusterLogs(matches["team"], namespace, matches["cluster"])
+		resp, err = s.controller.ClusterLogs(namespace, matches["cluster"])
 	} else if matches := util.FindNamedStringSubmatch(clusterHistoryURL, req.URL.Path); matches != nil {
 		namespace := matches["namespace"]
-		resp, err = s.controller.ClusterHistory(matches["team"], namespace, matches["cluster"])
+		resp, err = s.controller.ClusterHistory(namespace, matches["cluster"])
 	} else if req.URL.Path == clustersURL {
 		clusterNamesPerTeam := make(map[string][]string)
 		for team, clusters := range s.controller.TeamClusterList() {
 			for _, cluster := range clusters {
-				clusterNamesPerTeam[team] = append(clusterNamesPerTeam[team], cluster.Name[len(team)+1:])
+				clusterNamesPerTeam[team] = append(clusterNamesPerTeam[team], cluster.Name)
 			}
 		}
 		resp, err = clusterNamesPerTeam, nil
diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go
@@ -5,9 +5,9 @@ import (
 )
 
 const (
-	clusterStatusTest        = "/clusters/test-id/test_namespace/testcluster/"
-	clusterStatusNumericTest = "/clusters/test-id-1/test_namespace/testcluster/"
-	clusterLogsTest          = "/clusters/test-id/test_namespace/testcluster/logs/"
+	clusterStatusTest        = "/clusters/test-namespace/testcluster/"
+	clusterStatusNumericTest = "/clusters/test-namespace-1/testcluster/"
+	clusterLogsTest          = "/clusters/test-namespace/testcluster/logs/"
 	teamTest                 = "/clusters/test-id/"
 )
 
diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
@@ -1478,7 +1478,8 @@ func (c *Cluster) GetCurrentProcess() Process {
 // GetStatus provides status of the cluster
 func (c *Cluster) GetStatus() *ClusterStatus {
 	status := &ClusterStatus{
-		Cluster:             c.Spec.ClusterName,
+		Cluster:             c.Name,
+		Namespace:           c.Namespace,
 		Team:                c.Spec.TeamID,
 		Status:              c.Status,
 		Spec:                c.Spec,
diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go
@@ -587,7 +587,7 @@ func TestServiceAnnotations(t *testing.T) {
 			clusterAnnotations:         make(map[string]string),
 			operatorAnnotations:        make(map[string]string),
 			expect: map[string]string{
-				"external-dns.alpha.kubernetes.io/hostname":                            "test.test.db.example.com",
+				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
 			},
 		},
@@ -723,7 +723,7 @@ func TestServiceAnnotations(t *testing.T) {
 			clusterAnnotations:          make(map[string]string),
 			operatorAnnotations:         make(map[string]string),
 			expect: map[string]string{
-				"external-dns.alpha.kubernetes.io/hostname":                            "test-repl.test.db.example.com",
+				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
 			},
 		},
@@ -751,11 +751,6 @@ func TestServiceAnnotations(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.about, func(t *testing.T) {
 			cl.OpConfig.EnableTeamIdClusternamePrefix = tt.enableTeamIdClusterPrefix
-			if tt.enableTeamIdClusterPrefix {
-				cl.Postgresql.Spec.ClusterName = "test"
-			} else {
-				cl.Postgresql.Spec.ClusterName = "acid-test"
-			}
 
 			cl.OpConfig.CustomServiceAnnotations = tt.operatorAnnotations
 			cl.OpConfig.EnableMasterLoadBalancer = tt.enableMasterLoadBalancerOC
@@ -764,6 +759,7 @@ func TestServiceAnnotations(t *testing.T) {
 			cl.OpConfig.ReplicaDNSNameFormat = "{cluster}-repl.{namespace}.{hostedzone}"
 			cl.OpConfig.DbHostedZone = "db.example.com"
 
+			cl.Postgresql.Spec.ClusterName = ""
 			cl.Postgresql.Spec.TeamID = "acid"
 			cl.Postgresql.Spec.ServiceAnnotations = tt.clusterAnnotations
 			cl.Postgresql.Spec.EnableMasterLoadBalancer = tt.enableMasterLoadBalancerSpec
diff --git a/pkg/cluster/types.go b/pkg/cluster/types.go
@@ -59,6 +59,7 @@ type WorkerStatus struct {
 type ClusterStatus struct {
 	Team                string
 	Cluster             string
+	Namespace           string
 	MasterService       *v1.Service
 	ReplicaService      *v1.Service
 	MasterEndpoint      *v1.Endpoints
diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go
@@ -507,38 +507,38 @@ func (c *Cluster) roleLabelsSet(shouldAddExtraLabels bool, role PostgresRole) la
 
 func (c *Cluster) dnsName(role PostgresRole) string {
 	var dnsString string
+
 	if role == Master {
 		dnsString = c.masterDNSName()
 	} else {
 		dnsString = c.replicaDNSName()
 	}
 
-	// when cluster name starts with teamId prefix create an extra DNS entry
-	// to support the old format when prefix contraint was enabled (but is disabled now)
-	if !c.OpConfig.EnableTeamIdClusternamePrefix {
-		clusterNameWithoutTeamPrefix, _ := acidv1.ExtractClusterName(c.Name, c.Spec.TeamID)
-		if clusterNameWithoutTeamPrefix != "" {
-			if role == Replica {
-				clusterNameWithoutTeamPrefix = fmt.Sprintf("%s-repl", clusterNameWithoutTeamPrefix)
-			}
-			dnsString = fmt.Sprintf("%s,%s", dnsString, c.oldDNSFormat(clusterNameWithoutTeamPrefix))
+	// if cluster name starts with teamID we might need to provide backwards compatibility
+	clusterNameWithoutTeamPrefix, _ := acidv1.ExtractClusterName(c.Name, c.Spec.TeamID)
+	if clusterNameWithoutTeamPrefix != "" {
+		if role == Replica {
+			clusterNameWithoutTeamPrefix = fmt.Sprintf("%s-repl", clusterNameWithoutTeamPrefix)
 		}
+		dnsString = fmt.Sprintf("%s,%s", dnsString, c.oldDNSFormat(clusterNameWithoutTeamPrefix))
 	}
 
 	return dnsString
 }
 
 func (c *Cluster) masterDNSName() string {
 	return strings.ToLower(c.OpConfig.MasterDNSNameFormat.Format(
-		"cluster", c.Spec.ClusterName,
+		"cluster", c.Name,
 		"namespace", c.Namespace,
+		"team", c.teamName(),
 		"hostedzone", c.OpConfig.DbHostedZone))
 }
 
 func (c *Cluster) replicaDNSName() string {
 	return strings.ToLower(c.OpConfig.ReplicaDNSNameFormat.Format(
-		"cluster", c.Spec.ClusterName,
+		"cluster", c.Name,
 		"namespace", c.Namespace,
+		"team", c.teamName(),
 		"hostedzone", c.OpConfig.DbHostedZone))
 }
 
diff --git a/pkg/controller/logs_and_api.go b/pkg/controller/logs_and_api.go
@@ -15,11 +15,11 @@ import (
 )
 
 // ClusterStatus provides status of the cluster
-func (c *Controller) ClusterStatus(team, namespace, cluster string) (*cluster.ClusterStatus, error) {
+func (c *Controller) ClusterStatus(namespace, cluster string) (*cluster.ClusterStatus, error) {
 
 	clusterName := spec.NamespacedName{
 		Namespace: namespace,
-		Name:      team + "-" + cluster,
+		Name:      cluster,
 	}
 
 	c.clustersMu.RLock()
@@ -92,11 +92,11 @@ func (c *Controller) GetStatus() *spec.ControllerStatus {
 }
 
 // ClusterLogs dumps cluster ring logs
-func (c *Controller) ClusterLogs(team, namespace, name string) ([]*spec.LogEntry, error) {
+func (c *Controller) ClusterLogs(namespace, name string) ([]*spec.LogEntry, error) {
 
 	clusterName := spec.NamespacedName{
 		Namespace: namespace,
-		Name:      team + "-" + name,
+		Name:      name,
 	}
 
 	c.clustersMu.RLock()
@@ -215,11 +215,11 @@ func (c *Controller) WorkerStatus(workerID uint32) (*cluster.WorkerStatus, error
 }
 
 // ClusterHistory dumps history of cluster changes
-func (c *Controller) ClusterHistory(team, namespace, name string) ([]*spec.Diff, error) {
+func (c *Controller) ClusterHistory(namespace, name string) ([]*spec.Diff, error) {
 
 	clusterName := spec.NamespacedName{
 		Namespace: namespace,
-		Name:      team + "-" + name,
+		Name:      name,
 	}
 
 	c.clustersMu.RLock()
diff --git a/pkg/controller/postgresql.go b/pkg/controller/postgresql.go
@@ -159,24 +159,16 @@ func (c *Controller) acquireInitialListOfClusters() error {
 }
 
 func (c *Controller) addCluster(lg *logrus.Entry, clusterName spec.NamespacedName, pgSpec *acidv1.Postgresql) (*cluster.Cluster, error) {
-	var (
-		extractedClusterName string
-		err                  error
-	)
-
 	if c.opConfig.EnableTeamIdClusternamePrefix {
-		if extractedClusterName, err = acidv1.ExtractClusterName(clusterName.Name, pgSpec.Spec.TeamID); err != nil {
+		if _, err := acidv1.ExtractClusterName(clusterName.Name, pgSpec.Spec.TeamID); err != nil {
 			c.KubeClient.SetPostgresCRDStatus(clusterName, acidv1.ClusterStatusInvalid)
 			return nil, err
 		}
-	} else {
-		extractedClusterName = clusterName.Name
 	}
 
 	cl := cluster.New(c.makeClusterConfig(), c.KubeClient, *pgSpec, lg, c.eventRecorder)
 	cl.Run(c.stopCh)
 	teamName := strings.ToLower(cl.Spec.TeamID)
-	cl.ClusterName = extractedClusterName
 
 	defer c.clustersMu.Unlock()
 	c.clustersMu.Lock()
diff --git a/ui/app/src/new.tag.pug b/ui/app/src/new.tag.pug
@@ -64,7 +64,7 @@ new
 
               a.btn.btn-small.btn-warning(
                 if='{ clusterExists }'
-                href='/#/status/{ namespace.state }/{ team }-{ name }'
+                href='/#/status/{ namespace.state }/{ name }'
               )
                 | Cluster exists (show status)
 
@@ -137,10 +137,10 @@ new
                 input.form-control(
                   ref='name'
                   type='text'
-                  placeholder='new-cluster (can be { 53 - team.length - 1 } characters long)'
+                  placeholder='new-cluster (can be 53 characters long)'
                   title='Database cluster name, must be a valid hostname component'
                   pattern='[a-z0-9]+[a-z0-9\-]+[a-z0-9]+'
-                  maxlength='{ 53 - team.length - 1 }'
+                  maxlength=53
                   required
                   value='{ name }'
                   onchange='{ nameChange }'
@@ -520,7 +520,7 @@ new
       apiVersion: "acid.zalan.do/v1"
 
       metadata:
-        name: "{{ team }}-{{ name }}"
+        name: "{{ name }}"
         namespace: "{{ namespace.state }}"
         labels:
           team: {{ team }}
@@ -670,13 +670,12 @@ new
     this.updateDNSName = () => {
       this.dnsName = this.config.dns_format_string.format(
         this.name,
-        this.team,
         this.namespace.state,
       )
     }
 
     this.updateClusterName = () => {
-      this.clusterName = (this.team + '-' + this.name).toLowerCase()
+      this.clusterName = (this.name).toLowerCase()
       this.checkClusterExists()
       this.updateDNSName()
     }
@@ -950,7 +949,7 @@ new
         this.team = ''
       }
 
-      this.clusterName = (this.name + '-' + this.team).toLowerCase()
+      this.clusterName = (this.name + '-').toLowerCase()
       this.volumeSize = 10
       this.instanceCount = 1
       this.ranges = {}
diff --git a/ui/manifests/deployment.yaml b/ui/manifests/deployment.yaml
@@ -55,7 +55,7 @@ spec:
               value: |-
                 {
                   "docs_link":"https://postgres-operator.readthedocs.io/en/latest/",
-                  "dns_format_string": "{1}-{0}.{2}",
+                  "dns_format_string": "{0}.{1}",
                   "databases_visible": true,
                   "master_load_balancer_visible": true,
                   "nat_gateways_visible": false,
diff --git a/ui/operator_ui/main.py b/ui/operator_ui/main.py
@@ -321,7 +321,7 @@ def index():
     'databases_visible': True,
     'resources_visible': RESOURCES_VISIBLE,
     'postgresql_versions': ['11','12','13','14'],
-    'dns_format_string': '{0}.{1}.{2}',
+    'dns_format_string': '{0}.{1}',
     'pgui_link': '',
     'static_network_whitelist': {},
     'read_only_mode': READ_ONLY_MODE,
@@ -981,15 +981,7 @@ def get_operator_get_logs(worker: int):
 @app.route('/operator/clusters/<namespace>/<cluster>/logs')
 @authorize
 def get_operator_get_logs_per_cluster(namespace: str, cluster: str):
-    team, cluster_name = cluster.split('-', 1)
-    # team id might contain hyphens, try to find correct team name
-    user_teams = get_teams_for_user(session.get('user_name', ''))
-    for user_team in user_teams:
-        if cluster.find(user_team + '-') == 0:
-            team = cluster[:len(user_team)]
-            cluster_name = cluster[len(user_team + '-'):]
-            break
-    return proxy_operator(f'/clusters/{team}/{namespace}/{cluster_name}/logs/')
+    return proxy_operator(f'/clusters/{namespace}/{cluster}/logs/')
 
 
 @app.route('/login')
diff --git a/ui/run_local.sh b/ui/run_local.sh
@@ -14,7 +14,7 @@ export TARGET_NAMESPACE="${TARGET_NAMESPACE-*}"
 
 default_operator_ui_config='{
   "docs_link":"https://postgres-operator.readthedocs.io/en/latest/",
-  "dns_format_string": "{1}-{0}.{2}",
+  "dns_format_string": "{0}.{1}",
   "databases_visible": true,
   "nat_gateways_visible": false,
   "resources_visible": true,

Original file line number	Diff line number	Diff line change
`@@ -5,9 +5,9 @@ import (`
`5`	`5`	`)`
`6`	`6`
`7`	`7`	`const (`
`8`		`- clusterStatusTest = "/clusters/test-id/test_namespace/testcluster/"`
`9`		`- clusterStatusNumericTest = "/clusters/test-id-1/test_namespace/testcluster/"`
`10`		`- clusterLogsTest = "/clusters/test-id/test_namespace/testcluster/logs/"`
	`8`	`+ clusterStatusTest = "/clusters/test-namespace/testcluster/"`
	`9`	`+ clusterStatusNumericTest = "/clusters/test-namespace-1/testcluster/"`
	`10`	`+ clusterLogsTest = "/clusters/test-namespace/testcluster/logs/"`
`11`	`11`	`teamTest = "/clusters/test-id/"`
`12`	`12`	`)`
`13`	`13`
Original file line number	Diff line number	Diff line change
`@@ -15,11 +15,11 @@ import (`
`15`	`15`	`)`
`16`	`16`
`17`	`17`	`// ClusterStatus provides status of the cluster`
`18`		`-func (c Controller) ClusterStatus(team, namespace, cluster string) (cluster.ClusterStatus, error) {`
	`18`	`+func (c Controller) ClusterStatus(namespace, cluster string) (cluster.ClusterStatus, error) {`
`19`	`19`
`20`	`20`	`clusterName := spec.NamespacedName{`
`21`	`21`	`Namespace: namespace,`
`22`		`- Name: team + "-" + cluster,`
	`22`	`+ Name: cluster,`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`c.clustersMu.RLock()`
`@@ -92,11 +92,11 @@ func (c Controller) GetStatus() spec.ControllerStatus {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`// ClusterLogs dumps cluster ring logs`
`95`		`-func (c Controller) ClusterLogs(team, namespace, name string) ([]spec.LogEntry, error) {`
	`95`	`+func (c Controller) ClusterLogs(namespace, name string) ([]spec.LogEntry, error) {`
`96`	`96`
`97`	`97`	`clusterName := spec.NamespacedName{`
`98`	`98`	`Namespace: namespace,`
`99`		`- Name: team + "-" + name,`
	`99`	`+ Name: name,`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`c.clustersMu.RLock()`
`@@ -215,11 +215,11 @@ func (c Controller) WorkerStatus(workerID uint32) (cluster.WorkerStatus, error`
`215`	`215`	`}`
`216`	`216`
`217`	`217`	`// ClusterHistory dumps history of cluster changes`
`218`		`-func (c Controller) ClusterHistory(team, namespace, name string) ([]spec.Diff, error) {`
	`218`	`+func (c Controller) ClusterHistory(namespace, name string) ([]spec.Diff, error) {`
`219`	`219`
`220`	`220`	`clusterName := spec.NamespacedName{`
`221`	`221`	`Namespace: namespace,`
`222`		`- Name: team + "-" + name,`
	`222`	`+ Name: name,`
`223`	`223`	`}`
`224`	`224`
`225`	`225`	`c.clustersMu.RLock()`