From dcfc9925f6b3ef5dea51f81a99f0c95a0963b945 Mon Sep 17 00:00:00 2001 From: Sergey Dudoladov Date: Tue, 20 Feb 2018 14:43:02 +0100 Subject: [PATCH] Respond to code review --- manifests/configmap.yaml | 4 +- manifests/postgres-operator.yaml | 3 +- pkg/apiserver/apiserver.go | 6 +-- pkg/controller/controller.go | 69 ++++++++++++-------------------- pkg/controller/status.go | 12 ------ pkg/util/config/config.go | 2 +- pkg/util/util.go | 7 ++++ 7 files changed, 40 insertions(+), 63 deletions(-) diff --git a/manifests/configmap.yaml b/manifests/configmap.yaml index 8401a335d..cc0af9fe0 100644 --- a/manifests/configmap.yaml +++ b/manifests/configmap.yaml @@ -4,8 +4,8 @@ metadata: name: postgres-operator data: # the env var with the same name in the operator pod may overwrite this value - # if neither is set, listen to the 'default' namespace - # if set to the empty string "", listen to all namespaces + # if neither is set or evaluates to the empty string, listen to the operator's own namespace + # if set to the "*", listen to all namespaces # watched_namespace: development service_account_name: operator cluster_labels: application:spilo diff --git a/manifests/postgres-operator.yaml b/manifests/postgres-operator.yaml index 1b4c80c34..8e4425637 100644 --- a/manifests/postgres-operator.yaml +++ b/manifests/postgres-operator.yaml @@ -16,7 +16,8 @@ spec: imagePullPolicy: IfNotPresent env: # uncomment to overwrite a similar setting from operator configmap - # if set to the empty string, watch the 'default' namespace + # if set to the empty string, watch the operator's own namespace + # if set to the "*", listen to all namespaces # - name: WATCHED_NAMESPACE # valueFrom: # fieldRef: diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 76195f57d..8c01edeac 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -48,9 +48,9 @@ type Server struct { } var ( - clusterStatusURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/?$`) - clusterLogsURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/logs/?$`) - clusterHistoryURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/(?P[a-zA-Z][a-zA-Z0-9-]*)/history/?$`) + clusterStatusURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?)/(?P[a-zA-Z][a-zA-Z0-9-]*)/?$`) + clusterLogsURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?)/(?P[a-zA-Z][a-zA-Z0-9-]*)/logs/?$`) + clusterHistoryURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?)/(?P[a-zA-Z][a-zA-Z0-9-]*)/history/?$`) teamURL = regexp.MustCompile(`^/clusters/(?P[a-zA-Z][a-zA-Z0-9]*)/?$`) workerLogsURL = regexp.MustCompile(`^/workers/(?P\d+)/logs/?$`) workerEventsQueueURL = regexp.MustCompile(`^/workers/(?P\d+)/queue/?$`) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 62046ab7f..b0738bba8 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -14,6 +14,7 @@ import ( "github.com/zalando-incubator/postgres-operator/pkg/apiserver" "github.com/zalando-incubator/postgres-operator/pkg/cluster" "github.com/zalando-incubator/postgres-operator/pkg/spec" + "github.com/zalando-incubator/postgres-operator/pkg/util" "github.com/zalando-incubator/postgres-operator/pkg/util/config" "github.com/zalando-incubator/postgres-operator/pkg/util/constants" "github.com/zalando-incubator/postgres-operator/pkg/util/k8sutil" @@ -97,41 +98,7 @@ func (c *Controller) initOperatorConfig() { c.logger.Infoln("no ConfigMap specified. Loading default values") } - watchedNsConfigMapVar, isPresentInOperatorConfigMap := configMapData["watched_namespace"] - watchedNsEnvVar, isPresentInOperatorEnv := os.LookupEnv("WATCHED_NAMESPACE") - - if (!isPresentInOperatorConfigMap) && (!isPresentInOperatorEnv) { - - c.logger.Infof("No namespace to watch specified. By convention, the operator falls back to watching the namespace it is deployed to: '%v' \n", spec.GetOperatorNamespace()) - configMapData["watched_namespace"] = spec.GetOperatorNamespace() - - } - - if (isPresentInOperatorConfigMap) && (!isPresentInOperatorEnv) { - - // explicitly specify the policy for handling all namespaces - // note that '*' is not a valid namespace name - if watchedNsConfigMapVar == "*" { - c.logger.Infof("The watched namespace field in the operator config map evaluates to '*', meaning watching all namespaces.\n") - configMapData["watched_namespace"] = v1.NamespaceAll - } - } - - if isPresentInOperatorEnv { - - if isPresentInOperatorConfigMap { - c.logger.Infof("Both WATCHED_NAMESPACE=%q env var and wacthed_namespace=%q field in operator config map are defined. The env variable takes priority over the configMap param\n", watchedNsEnvVar, watchedNsConfigMapVar) - } - - // handle all namespaces consistently - if watchedNsEnvVar == "*" { - c.logger.Infof("The watched namespace field in the operator config map evaluates to '*', meaning watching all namespaces.\n") - configMapData["watched_namespace"] = v1.NamespaceAll - } else { - c.logger.Infof("Watch the %q namespace specified in the env variable WATCHED_NAMESPACE\n", watchedNsEnvVar) - configMapData["watched_namespace"] = watchedNsEnvVar - } - } + configMapData["watched_namespace"] = c.getEffectiveNamespace(os.Getenv("WATCHED_NAMESPACE"), configMapData["watched_namespace"]) if c.config.NoDatabaseAccess { configMapData["enable_database_access"] = "false" @@ -152,15 +119,6 @@ func (c *Controller) initController() { c.initClients() c.initOperatorConfig() - // earliest point where we can check if the namespace to watch actually exists - if c.opConfig.WatchedNamespace != v1.NamespaceAll { - _, err := c.KubeClient.Namespaces().Get(c.opConfig.WatchedNamespace, metav1.GetOptions{}) - if err != nil { - c.logger.Fatalf("Operator was told to watch the %q namespace but was unable to find it via Kubernetes API.", c.opConfig.WatchedNamespace) - } - - } - c.initSharedInformers() c.logger.Infof("config: %s", c.opConfig.MustMarshal()) @@ -290,3 +248,26 @@ func (c *Controller) kubeNodesInformer(stopCh <-chan struct{}, wg *sync.WaitGrou c.nodesInformer.Run(stopCh) } + +func (c *Controller) getEffectiveNamespace(namespaceFromEnvironment, namespaceFromConfigMap string) string { + + namespace := util.Coalesce(namespaceFromEnvironment, util.Coalesce(namespaceFromConfigMap, spec.GetOperatorNamespace())) + + if namespace == "*" { + + namespace = v1.NamespaceAll + c.logger.Infof("Listening to all namespaces") + + } else { + + if _, err := c.KubeClient.Namespaces().Get(namespace, metav1.GetOptions{}); err != nil { + // the namespace may be created manually at runtime + c.logger.Warnf("Could not find the watched namespace %q", namespace) + } else { + c.logger.Infof("Listenting to the specific namespace %q", namespace) + } + + } + + return namespace +} diff --git a/pkg/controller/status.go b/pkg/controller/status.go index 932b77500..73f25f697 100644 --- a/pkg/controller/status.go +++ b/pkg/controller/status.go @@ -16,10 +16,6 @@ import ( // ClusterStatus provides status of the cluster func (c *Controller) ClusterStatus(team, namespace, cluster string) (*spec.ClusterStatus, error) { - if namespace == "" { - namespace = c.opConfig.WatchedNamespace - } - clusterName := spec.NamespacedName{ Namespace: namespace, Name: team + "-" + cluster, @@ -97,10 +93,6 @@ func (c *Controller) GetStatus() *spec.ControllerStatus { // ClusterLogs dumps cluster ring logs func (c *Controller) ClusterLogs(team, namespace, name string) ([]*spec.LogEntry, error) { - if namespace == "" { - namespace = c.opConfig.WatchedNamespace - } - clusterName := spec.NamespacedName{ Namespace: namespace, Name: team + "-" + name, @@ -224,10 +216,6 @@ func (c *Controller) WorkerStatus(workerID uint32) (*spec.WorkerStatus, error) { // ClusterHistory dumps history of cluster changes func (c *Controller) ClusterHistory(team, namespace, name string) ([]*spec.Diff, error) { - if namespace == "" { - namespace = c.opConfig.WatchedNamespace - } - clusterName := spec.NamespacedName{ Namespace: namespace, Name: team + "-" + name, diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go index 0bd2b1edf..edee72de4 100644 --- a/pkg/util/config/config.go +++ b/pkg/util/config/config.go @@ -67,7 +67,7 @@ type Config struct { Resources Auth Scalyr - WatchedNamespace string `name:"watched_namespace"` // may be v1.NamespaceAll, meaning watch all namespaces + WatchedNamespace string `name:"watched_namespace"` // may be "*", meaning watch all namespaces EtcdHost string `name:"etcd_host" default:"etcd-client.default.svc.cluster.local:2379"` DockerImage string `name:"docker_image" default:"registry.opensource.zalan.do/acid/spiloprivate-9.6:1.2-p4"` ServiceAccountName string `name:"service_account_name" default:"operator"` diff --git a/pkg/util/util.go b/pkg/util/util.go index c003ef477..b1b3d91b3 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -119,3 +119,10 @@ func MapContains(haystack, needle map[string]string) bool { return true } + +func Coalesce(val, defaultVal string) string { + if val == "" { + return defaultVal + } + return val +}