Fix: label values not validated (#355) (#339)

Includes: * Fix: label values not validated (#355) * Support for CreatedByLabelValue; clean labels in api * Smoke test updates * Co-authored-by: Alexander Dejanovski <[email protected]>
k8ssandra · Jun 14, 2022 · 34648fd · 34648fd
1 parent 942bd73
commit 34648fd
Show file tree

Hide file tree

Showing 15 changed files with 188 additions and 76 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ Changelog for Cass Operator, new PRs should update the `main / unreleased` secti
 ```
 
 ## unreleased
+* [BUGFIX] [#355](https://github.com/k8ssandra/cass-operator/issues/335) Cleanse label values derived from cluster name, which can contain illegal chars. Include app.kubernetes.io/created-by label. 
 
 ## v1.11.0
 

diff --git a/apis/cassandra/v1beta1/cassandradatacenter_types.go b/apis/cassandra/v1beta1/cassandradatacenter_types.go
@@ -32,7 +32,6 @@ const (
 	// RackLabel is the operator's label for the rack name
 	RackLabel = "cassandra.datastax.com/rack"
 
-	// RackLabel is the operator's label for the rack name
 	CassOperatorProgressLabel = "cassandra.datastax.com/operator-progress"
 
 	// PromMetricsLabel is a service label that can be selected for prometheus metrics scraping
@@ -53,19 +52,16 @@ const (
 	// CassandraDatacenter is deleted.
 	DecommissionOnDeleteAnnotation = "cassandra.datastax.com/decommission-on-delete"
 
-	// CassNodeState
 	CassNodeState = "cassandra.datastax.com/node-state"
 
-	// Progress states for status
 	ProgressUpdating ProgressState = "Updating"
 	ProgressReady    ProgressState = "Ready"
 
-	// Default port numbers
 	DefaultNativePort    = 9042
 	DefaultInternodePort = 7000
 )
 
-// This type exists so there's no chance of pushing random strings to our progress status
+// ProgressState - this type exists so there's no chance of pushing random strings to our progress status
 type ProgressState string
 
 type CassandraUser struct {
@@ -238,7 +234,7 @@ type NodePortConfig struct {
 	InternodeSSL int `json:"internodeSSL,omitempty"`
 }
 
-// Is the NodePort service enabled?
+// IsNodePortEnabled is the NodePort service enabled?
 func (dc *CassandraDatacenter) IsNodePortEnabled() bool {
 	return dc.Spec.Networking != nil && dc.Spec.Networking.NodePort != nil
 }
@@ -254,7 +250,7 @@ type DseWorkloads struct {
 	SearchEnabled    bool `json:"searchEnabled,omitempty"`
 }
 
-// StorageConfig defines additional storage configurations
+// AdditionalVolumes StorageConfig defines additional storage configurations
 type AdditionalVolumes struct {
 	// Mount path into cassandra container
 	MountPath string `json:"mountPath"`
@@ -533,20 +529,31 @@ func (dc *CassandraDatacenter) SetCondition(condition DatacenterCondition) {
 // GetDatacenterLabels ...
 func (dc *CassandraDatacenter) GetDatacenterLabels() map[string]string {
 	labels := dc.GetClusterLabels()
-	labels[DatacenterLabel] = dc.Name
+	labels[DatacenterLabel] = CleanLabelValue(dc.Name)
 	return labels
 }
 
 // GetClusterLabels returns a new map with the cluster label key and cluster name value
 func (dc *CassandraDatacenter) GetClusterLabels() map[string]string {
 	return map[string]string{
-		ClusterLabel: dc.Spec.ClusterName,
+		ClusterLabel: CleanLabelValue(dc.Spec.ClusterName),
 	}
 }
 
 // apimachinery validation does not expose this, copied here
 const dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?"
 
+var whitelistRegex = regexp.MustCompile(`(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?`)
+
+// CleanLabelValue a valid label must be an empty string or consist of alphanumeric characters,
+// '-', '_' or '.', and must start and end with an alphanumeric.
+// Note: we apply a prefix of "cassandra-" to the cluster name value used as label name.
+// As such, empty string isn't a valid case.
+func CleanLabelValue(value string) string {
+	regexpResult := whitelistRegex.FindAllString(strings.Replace(value, " ", "", -1), -1)
+	return strings.Join(regexpResult, "")
+}
+
 func CleanupForKubernetes(input string) string {
 	if len(validation.IsDNS1035Label(input)) > 0 {
 		r := regexp.MustCompile(dns1035LabelFmt)
@@ -653,8 +660,7 @@ func (dc *CassandraDatacenter) GetConfigAsJSON(config []byte) (string, error) {
 	}
 
 	// Combine the model values with the user-specified values
-
-	modelParsed, err := gabs.ParseJSON([]byte(modelBytes))
+	modelParsed, err := gabs.ParseJSON(modelBytes)
 	if err != nil {
 		return "", errors.Wrap(err, "Model information for CassandraDatacenter resource was not properly configured")
 	}
@@ -673,6 +679,7 @@ func (dc *CassandraDatacenter) GetConfigAsJSON(config []byte) (string, error) {
 	return modelParsed.String(), nil
 }
 
+// GetNodePortNativePort
 // Gets the defined CQL port for NodePort.
 // 0 will be returned if NodePort is not configured.
 // The SSL port will be returned if it is defined,
@@ -691,6 +698,7 @@ func (dc *CassandraDatacenter) GetNodePortNativePort() int {
 	}
 }
 
+// GetNodePortInternodePort
 // Gets the defined internode/broadcast port for NodePort.
 // 0 will be returned if NodePort is not configured.
 // The SSL port will be returned if it is defined,

diff --git a/apis/cassandra/v1beta1/cassandradatacenter_webhook.go b/apis/cassandra/v1beta1/cassandradatacenter_webhook.go
@@ -33,9 +33,9 @@ import (
 
 var log = logf.Log.WithName("api")
 
-func (r *CassandraDatacenter) SetupWebhookWithManager(mgr ctrl.Manager) error {
+func (dc *CassandraDatacenter) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
-		For(r).
+		For(dc).
 		Complete()
 }
 
@@ -45,7 +45,7 @@ func (r *CassandraDatacenter) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Defaulter = &CassandraDatacenter{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type
-func (r *CassandraDatacenter) Default() {
+func (dc *CassandraDatacenter) Default() {
 	// No mutations at this point
 }
 

diff --git a/config/crd/bases/cassandra.datastax.com_cassandradatacenters.yaml b/config/crd/bases/cassandra.datastax.com_cassandradatacenters.yaml
@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   name: cassandradatacenters.cassandra.datastax.com
 spec:
@@ -188,12 +187,12 @@ spec:
                 description: "ConfigSecret is the name of a secret that contains configuration
                   for Cassandra. The secret is expected to have a property named config
                   whose value should be a JSON formatted string that should look like
-                  this: \n    config: |-      {        \"cassandra-yaml\": {          \"read_request_timeout_in_ms\":
-                  10000        },        \"jmv-options\": {          \"max_heap_size\":
-                  1024M        }      } \n ConfigSecret is mutually exclusive with
-                  Config. ConfigSecret takes precedence and will be used exclusively
-                  if both properties are set. The operator sets a watch such that
-                  an update to the secret will trigger an update of the StatefulSets."
+                  this: \n config: |- { \"cassandra-yaml\": { \"read_request_timeout_in_ms\":
+                  10000 }, \"jmv-options\": { \"max_heap_size\": 1024M } } \n ConfigSecret
+                  is mutually exclusive with Config. ConfigSecret takes precedence
+                  and will be used exclusively if both properties are set. The operator
+                  sets a watch such that an update to the secret will trigger an update
+                  of the StatefulSets."
                 type: string
               disableSystemLoggerSidecar:
                 description: Configuration for disabling the simple log tailing sidecar
@@ -5688,13 +5687,13 @@ spec:
                                 with a pod if it doesn''t satisfy the spread constraint.
                                 - DoNotSchedule (default) tells the scheduler not
                                 to schedule it. - ScheduleAnyway tells the scheduler
-                                to schedule the pod in any location,   but giving
-                                higher precedence to topologies that would help reduce
-                                the   skew. A constraint is considered "Unsatisfiable"
-                                for an incoming pod if and only if every possible
-                                node assignment for that pod would violate "MaxSkew"
-                                on some topology. For example, in a 3-zone cluster,
-                                MaxSkew is set to 1, and pods with the same labelSelector
+                                to schedule the pod in any location, but giving higher
+                                precedence to topologies that would help reduce the
+                                skew. A constraint is considered "Unsatisfiable" for
+                                an incoming pod if and only if every possible node
+                                assignment for that pod would violate "MaxSkew" on
+                                some topology. For example, in a 3-zone cluster, MaxSkew
+                                is set to 1, and pods with the same labelSelector
                                 spread as 3/1/1: | zone1 | zone2 | zone3 | | P P P
                                 |   P   |   P   | If WhenUnsatisfiable is set to DoNotSchedule,
                                 incoming pod can only be scheduled to zone2(zone3)
@@ -6131,13 +6130,12 @@ spec:
                                 when the pod is removed. \n Use this if: a) the volume
                                 is only needed while the pod runs, b) features of
                                 normal volumes like restoring from snapshot or capacity
-                                \   tracking are needed, c) the storage driver is
-                                specified through a storage class, and d) the storage
-                                driver supports dynamic volume provisioning through
-                                \   a PersistentVolumeClaim (see EphemeralVolumeSource
-                                for more    information on the connection between
-                                this volume type    and PersistentVolumeClaim). \n
-                                Use PersistentVolumeClaim or one of the vendor-specific
+                                tracking are needed, c) the storage driver is specified
+                                through a storage class, and d) the storage driver
+                                supports dynamic volume provisioning through a PersistentVolumeClaim
+                                (see EphemeralVolumeSource for more information on
+                                the connection between this volume type and PersistentVolumeClaim).
+                                \n Use PersistentVolumeClaim or one of the vendor-specific
                                 APIs for volumes that persist for longer than the
                                 lifecycle of an individual pod. \n Use CSI for light-weight
                                 local ephemeral volumes if the CSI driver is meant
@@ -6262,14 +6260,15 @@ spec:
                                             are two important differences between
                                             DataSource and DataSourceRef: * While
                                             DataSource only allows two specific types
-                                            of objects, DataSourceRef   allows any
-                                            non-core object, as well as PersistentVolumeClaim
+                                            of objects, DataSourceRef allows any non-core
+                                            object, as well as PersistentVolumeClaim
                                             objects. * While DataSource ignores disallowed
-                                            values (dropping them), DataSourceRef   preserves
-                                            all values, and generates an error if
-                                            a disallowed value is   specified. (Alpha)
-                                            Using this field requires the AnyVolumeDataSource
-                                            feature gate to be enabled.'
+                                            values (dropping them), DataSourceRef
+                                            preserves all values, and generates an
+                                            error if a disallowed value is specified.
+                                            (Alpha) Using this field requires the
+                                            AnyVolumeDataSource feature gate to be
+                                            enabled.'
                                           properties:
                                             apiGroup:
                                               description: APIGroup is the group for
@@ -7440,7 +7439,8 @@ spec:
                 properties:
                   additionalVolumes:
                     items:
-                      description: StorageConfig defines additional storage configurations
+                      description: AdditionalVolumes StorageConfig defines additional
+                        storage configurations
                       properties:
                         mountPath:
                           description: Mount path into cassandra container
@@ -7503,11 +7503,11 @@ spec:
                                 if one of them is empty and the other is non-empty.
                                 There are two important differences between DataSource
                                 and DataSourceRef: * While DataSource only allows
-                                two specific types of objects, DataSourceRef   allows
+                                two specific types of objects, DataSourceRef allows
                                 any non-core object, as well as PersistentVolumeClaim
                                 objects. * While DataSource ignores disallowed values
-                                (dropping them), DataSourceRef   preserves all values,
-                                and generates an error if a disallowed value is   specified.
+                                (dropping them), DataSourceRef preserves all values,
+                                and generates an error if a disallowed value is specified.
                                 (Alpha) Using this field requires the AnyVolumeDataSource
                                 feature gate to be enabled.'
                               properties:
@@ -7680,10 +7680,10 @@ spec:
                           if one of them is empty and the other is non-empty. There
                           are two important differences between DataSource and DataSourceRef:
                           * While DataSource only allows two specific types of objects,
-                          DataSourceRef   allows any non-core object, as well as PersistentVolumeClaim
+                          DataSourceRef allows any non-core object, as well as PersistentVolumeClaim
                           objects. * While DataSource ignores disallowed values (dropping
-                          them), DataSourceRef   preserves all values, and generates
-                          an error if a disallowed value is   specified. (Alpha) Using
+                          them), DataSourceRef preserves all values, and generates
+                          an error if a disallowed value is specified. (Alpha) Using
                           this field requires the AnyVolumeDataSource feature gate
                           to be enabled.'
                         properties:
@@ -7958,23 +7958,23 @@ spec:
                   description: 'ObjectReference contains enough information to let
                     you inspect or modify the referred object. --- New uses of this
                     type are discouraged because of difficulty describing its usage
-                    when embedded in APIs.  1. Ignored fields.  It includes many fields
+                    when embedded in APIs. 1. Ignored fields.  It includes many fields
                     which are not generally honored.  For instance, ResourceVersion
-                    and FieldPath are both very rarely valid in actual usage.  2.
-                    Invalid usage help.  It is impossible to add specific help for
-                    individual usage.  In most embedded usages, there are particular     restrictions
+                    and FieldPath are both very rarely valid in actual usage. 2. Invalid
+                    usage help.  It is impossible to add specific help for individual
+                    usage.  In most embedded usages, there are particular restrictions
                     like, "must refer only to types A and B" or "UID not honored"
-                    or "name must be restricted".     Those cannot be well described
-                    when embedded.  3. Inconsistent validation.  Because the usages
-                    are different, the validation rules are different by usage, which
-                    makes it hard for users to predict what will happen.  4. The fields
+                    or "name must be restricted". Those cannot be well described when
+                    embedded. 3. Inconsistent validation.  Because the usages are
+                    different, the validation rules are different by usage, which
+                    makes it hard for users to predict what will happen. 4. The fields
                     are both imprecise and overly precise.  Kind is not a precise
-                    mapping to a URL. This can produce ambiguity     during interpretation
+                    mapping to a URL. This can produce ambiguity during interpretation
                     and require a REST mapping.  In most cases, the dependency is
-                    on the group,resource tuple     and the version of the actual
-                    struct is irrelevant.  5. We cannot easily change it.  Because
-                    this type is embedded in many locations, updates to this type     will
-                    affect numerous schemas.  Don''t make new APIs embed an underspecified
+                    on the group,resource tuple and the version of the actual struct
+                    is irrelevant. 5. We cannot easily change it.  Because this type
+                    is embedded in many locations, updates to this type will affect
+                    numerous schemas.  Don''t make new APIs embed an underspecified
                     API type they do not control. Instead of using this type, create
                     a locally provided and used type that is well-focused on your
                     reference. For example, ServiceReferences for admission registration:

diff --git a/config/crd/bases/control.k8ssandra.io_cassandratasks.yaml b/config/crd/bases/control.k8ssandra.io_cassandratasks.yaml
@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   name: cassandratasks.control.k8ssandra.io
 spec:

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -1,4 +1,3 @@
-
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -15,7 +14,6 @@ rules:
   - get
   - list
   - watch
-
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role

diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
@@ -1,4 +1,3 @@
-
 ---
 apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration

diff --git a/pkg/oplabels/labels.go b/pkg/oplabels/labels.go
@@ -5,7 +5,6 @@ package oplabels
 
 import (
 	"fmt"
-
 	api "github.com/k8ssandra/cass-operator/apis/cassandra/v1beta1"
 )
 
@@ -18,15 +17,15 @@ const (
 	InstanceLabel              = "app.kubernetes.io/instance"
 	VersionLabel               = "app.kubernetes.io/version"
 	CreatedByLabel             = "app.kubernetes.io/created-by"
+	CreatedByLabelValue        = "cassandradatacenter_controller"
 )
 
 func AddOperatorLabels(m map[string]string, dc *api.CassandraDatacenter) {
 	m[ManagedByLabel] = ManagedByLabelValue
 	m[NameLabel] = NameLabelValue
 	m[VersionLabel] = dc.Spec.ServerVersion
-
-	instanceName := fmt.Sprintf("cassandra-%s", dc.Spec.ClusterName)
-	m[InstanceLabel] = instanceName
+	m[InstanceLabel] = fmt.Sprintf("cassandra-%s", api.CleanLabelValue(dc.Spec.ClusterName))
+	m[CreatedByLabel] = CreatedByLabelValue
 
 	if len(dc.Spec.AdditionalLabels) != 0 {
 		for key, value := range dc.Spec.AdditionalLabels {
-Original file line number
+Diff line change
@@ Expand Up @@
     ```
     ## unreleased
+    * [BUGFIX] [#355](https://github.com/k8ssandra/cass-operator/issues/335) Cleanse label values derived from cluster name, which can contain illegal chars. Include app.kubernetes.io/created-by label.
     ## v1.11.0
@@ Expand Down @@