Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default labels to use Kubernetes resource name #698

Merged
merged 9 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions apis/cassandra/v1beta1/cassandradatacenter_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,7 @@ type CassandraDatacenterSpec struct {
// CDC allows configuration of the change data capture agent which can run within the Management API container. Use it to send data to Pulsar.
CDC *CDCConfiguration `json:"cdc,omitempty"`

// DatacenterName allows to override the name of the Cassandra datacenter. Kubernetes objects will be named after a sanitized version of it if set, and if not metadata.name. In Cassandra the DC name will be overridden by this value.
// It may generate some confusion as objects created for the DC will have a different name than the CasandraDatacenter object itself.
// DatacenterName allows to override the name of the Cassandra datacenter. In Cassandra the DC name will be overridden by this value.
// This setting can create conflicts if multiple DCs coexist in the same namespace if metadata.name for a DC with no override is set to the same value as the override name of another DC.
// Use cautiously.
// +optional
Expand Down Expand Up @@ -488,6 +487,9 @@ type CassandraDatacenterStatus struct {
// This field is used to perform validation checks preventing a user from changing the override
// +optional
DatacenterName *string `json:"datacenterName,omitempty"`

// +optional
MetadataVersion int64 `json:"metadataVersion,omitempty"`
}

// CassandraDatacenter is the Schema for the cassandradatacenters API
Expand Down Expand Up @@ -599,7 +601,7 @@ func (dc *CassandraDatacenter) SetCondition(condition DatacenterCondition) {
// GetDatacenterLabels ...
func (dc *CassandraDatacenter) GetDatacenterLabels() map[string]string {
labels := dc.GetClusterLabels()
labels[DatacenterLabel] = CleanLabelValue(dc.DatacenterName())
labels[DatacenterLabel] = CleanLabelValue(dc.Name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is CleanLabelValue still necessary now that we're using the Kubernetes meta.name which will be DNS compliant anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not really, but they do have a bit different rules:

	dns1123SubdomainFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*"
	dns1123LabelFmt     string = "(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?"

So better safe than sorry I guess, especially if Kubernetes would one day update these..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think the dns1123LabelFmt is more liberal than the dns1123SubdomainFmt, I also don't think k8s will ever change these (since they're linked to the DNS spec).

I'd remove the extra function call just so it is clear to readers that the label is always going to be the exact meta.Name, and not something else that we've interfered with.

This is just a suggestion though, so it isn't blocking.

return labels
}

Expand Down Expand Up @@ -664,19 +666,19 @@ func (dc *CassandraDatacenter) GetSeedServiceName() string {
}

func (dc *CassandraDatacenter) GetAdditionalSeedsServiceName() string {
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-additional-seed-service"
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-additional-seed-service"
}

func (dc *CassandraDatacenter) GetAllPodsServiceName() string {
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-all-pods-service"
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-all-pods-service"
}

func (dc *CassandraDatacenter) GetDatacenterServiceName() string {
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-service"
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-service"
}

func (dc *CassandraDatacenter) GetNodePortServiceName() string {
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-node-port-service"
return CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-node-port-service"
}

func (dc *CassandraDatacenter) ShouldGenerateSuperuserSecret() bool {
Expand Down Expand Up @@ -973,9 +975,17 @@ func SplitRacks(nodeCount, rackCount int) []int {
return topology
}

// SanitizedName returns a sanitized version of the name returned by DatacenterName()
func (dc *CassandraDatacenter) SanitizedName() string {
return CleanupForKubernetes(dc.DatacenterName())
func (dc *CassandraDatacenter) DatacenterNameStatus() bool {
return dc.Status.DatacenterName != nil
}

// LabelResourceName returns a sanitized version of the name returned by DatacenterName()
func (dc *CassandraDatacenter) LabelResourceName() string {
// If existing cluster, return dc.DatacenterName() else return dc.Name
if dc.DatacenterNameStatus() {
return CleanupForKubernetes(*dc.Status.DatacenterName)
}
return CleanupForKubernetes(dc.Name)
}

// DatacenterName returns the Cassandra DC name override if it exists,
Expand Down
48 changes: 48 additions & 0 deletions apis/cassandra/v1beta1/cassandradatacenter_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"testing"

"github.com/Jeffail/gabs/v2"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
Expand Down Expand Up @@ -190,3 +191,50 @@ func TestUseClientImageEnforce(t *testing.T) {
assert.True(dc.UseClientImage())
}
}

func TestDatacenterNoOverrideConfig(t *testing.T) {
assert := assert.New(t)
dc := CassandraDatacenter{
ObjectMeta: metav1.ObjectMeta{
Name: "dc1",
},
Spec: CassandraDatacenterSpec{
ClusterName: "cluster1",
},
}

config, err := dc.GetConfigAsJSON(dc.Spec.Config)
assert.NoError(err)

container, err := gabs.ParseJSON([]byte(config))
assert.NoError(err)

dataCenterInfo := container.ChildrenMap()["datacenter-info"]
assert.NotEmpty(dataCenterInfo)
assert.Equal(dc.Name, dataCenterInfo.ChildrenMap()["name"].Data().(string))
assert.Equal(dc.DatacenterName(), dc.Name)
}

func TestDatacenterOverrideInConfig(t *testing.T) {
assert := assert.New(t)
dc := CassandraDatacenter{
ObjectMeta: metav1.ObjectMeta{
Name: "dc1",
},
Spec: CassandraDatacenterSpec{
ClusterName: "cluster1",
DatacenterName: "Home_Dc",
},
}

config, err := dc.GetConfigAsJSON(dc.Spec.Config)
assert.NoError(err)

container, err := gabs.ParseJSON([]byte(config))
assert.NoError(err)

dataCenterInfo := container.ChildrenMap()["datacenter-info"]
assert.NotEmpty(dataCenterInfo)
assert.Equal(dc.Spec.DatacenterName, dataCenterInfo.ChildrenMap()["name"].Data().(string))
assert.Equal(dc.DatacenterName(), dc.Spec.DatacenterName)
}
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,7 @@ spec:
type: string
datacenterName:
description: |-
DatacenterName allows to override the name of the Cassandra datacenter. Kubernetes objects will be named after a sanitized version of it if set, and if not metadata.name. In Cassandra the DC name will be overridden by this value.
It may generate some confusion as objects created for the DC will have a different name than the CasandraDatacenter object itself.
DatacenterName allows to override the name of the Cassandra datacenter. In Cassandra the DC name will be overridden by this value.
This setting can create conflicts if multiple DCs coexist in the same namespace if metadata.name for a DC with no override is set to the same value as the override name of another DC.
Use cautiously.
type: string
Expand Down Expand Up @@ -11221,6 +11220,9 @@ spec:
with the management API
format: date-time
type: string
metadataVersion:
format: int64
type: integer
nodeReplacements:
items:
type: string
Expand Down
2 changes: 1 addition & 1 deletion config/manager/image_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ defaults:
# Note, postfix is ignored if repository is not set
cassandra:
repository: "k8ssandra/cass-management-api"
suffix: "-ubi8"
suffix: "-ubi"
dse:
repository: "datastax/dse-mgmtapi-6_8"
suffix: "-ubi8"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,9 @@ var _ = Describe("CassandraDatacenter tests", func() {
refreshDatacenter(ctx, &dc)

By("Updating the size to 3")
patch := client.MergeFrom(dc.DeepCopy())
dc.Spec.Size = 3
Expect(k8sClient.Update(ctx, &dc)).To(Succeed())
Expect(k8sClient.Patch(ctx, &dc, patch)).To(Succeed())

waitForDatacenterCondition(ctx, dcName, cassdcapi.DatacenterScalingUp, corev1.ConditionTrue).Should(Succeed())
waitForDatacenterProgress(ctx, dcName, cassdcapi.ProgressUpdating).Should(Succeed())
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/control/cassandratask_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (r *CassandraTaskReconciler) Reconcile(ctx context.Context, req ctrl.Reques
return ctrl.Result{}, errors.Wrapf(err, "unable to fetch target CassandraDatacenter: %s", cassTask.Spec.Datacenter)
}

logger = log.FromContext(ctx, "datacenterName", dc.SanitizedName(), "clusterName", dc.Spec.ClusterName)
logger = log.FromContext(ctx, "datacenterName", dc.LabelResourceName(), "clusterName", dc.Spec.ClusterName)
log.IntoContext(ctx, logger)

// If we're active, we can proceed - otherwise verify if we're allowed to run
Expand Down
2 changes: 1 addition & 1 deletion pkg/images/images_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestDefaultImageConfigParsing(t *testing.T) {

path, err = GetCassandraImage("cassandra", "4.1.4")
assert.NoError(err)
assert.Equal("k8ssandra/cass-management-api:4.1.4-ubi8", path)
assert.Equal("k8ssandra/cass-management-api:4.1.4-ubi", path)
}

func TestImageConfigParsing(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciliation/construct_podtemplatespec.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func addVolumes(dc *api.CassandraDatacenter, baseTemplate *corev1.PodTemplateSpe
Name: "encryption-cred-storage",
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: fmt.Sprintf("%s-keystore", dc.SanitizedName()),
SecretName: fmt.Sprintf("%s-keystore", dc.LabelResourceName()),
},
},
}
Expand Down Expand Up @@ -614,7 +614,7 @@ func getConfigDataEnVars(dc *api.CassandraDatacenter) ([]corev1.EnvVar, error) {
return envVars, nil
}

return nil, fmt.Errorf("datacenter %s is missing %s annotation", dc.SanitizedName(), api.ConfigHashAnnotation)
return nil, fmt.Errorf("datacenter %s is missing %s annotation", dc.LabelResourceName(), api.ConfigHashAnnotation)
}

configData, err := dc.GetConfigAsJSON(dc.Spec.Config)
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciliation/construct_podtemplatespec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1548,7 +1548,7 @@ func Test_makeImage(t *testing.T) {
serverType: "cassandra",
serverVersion: "3.11.10",
},
want: "localhost:5000/k8ssandra/cass-management-api:3.11.10-ubi8",
want: "localhost:5000/k8ssandra/cass-management-api:3.11.10-ubi",
errString: "",
},
{
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciliation/construct_statefulset.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func newNamespacedNameForStatefulSet(
dc *api.CassandraDatacenter,
rackName string) types.NamespacedName {

name := api.CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-" + api.CleanupSubdomain(rackName) + "-sts"
name := api.CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-" + api.CleanupSubdomain(rackName) + "-sts"
ns := dc.Namespace

return types.NamespacedName{
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciliation/construct_statefulset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ func Test_newStatefulSetForCassandraDatacenter_dcNameOverride(t *testing.T) {
oplabels.NameLabel: oplabels.NameLabelValue,
oplabels.CreatedByLabel: oplabels.CreatedByLabelValue,
oplabels.VersionLabel: "4.0.1",
api.DatacenterLabel: "MySuperDC",
api.DatacenterLabel: "dc1",
api.ClusterLabel: "piclem",
api.RackLabel: dc.Spec.Racks[0].Name,
}
Expand All @@ -652,7 +652,7 @@ func Test_newStatefulSetForCassandraDatacenter_dcNameOverride(t *testing.T) {
oplabels.NameLabel: oplabels.NameLabelValue,
oplabels.CreatedByLabel: oplabels.CreatedByLabelValue,
oplabels.VersionLabel: "4.0.1",
api.DatacenterLabel: "MySuperDC",
api.DatacenterLabel: "dc1",
api.ClusterLabel: "piclem",
api.RackLabel: dc.Spec.Racks[0].Name,
api.CassNodeState: stateReadyToStart,
Expand Down
29 changes: 16 additions & 13 deletions pkg/reconciliation/constructor.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func newPodDisruptionBudgetForDatacenter(dc *api.CassandraDatacenter) *policyv1.

pdb := &policyv1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: dc.SanitizedName() + "-pdb",
Name: dc.LabelResourceName() + "-pdb",
Namespace: dc.Namespace,
Labels: labels,
Annotations: anns,
Expand Down Expand Up @@ -62,8 +62,11 @@ func setOperatorProgressStatus(rc *ReconciliationContext, newState api.ProgressS
rc.Datacenter.Status.CassandraOperatorProgress = newState

if newState == api.ProgressReady {
if rc.Datacenter.Status.MetadataVersion < 1 {
rc.Datacenter.Status.MetadataVersion = 1
}
if rc.Datacenter.Status.DatacenterName == nil {
rc.Datacenter.Status.DatacenterName = &rc.Datacenter.Spec.DatacenterName
rc.Datacenter.Status.DatacenterName = &rc.Datacenter.Name
}
}
if err := rc.Client.Status().Patch(rc.Ctx, rc.Datacenter, patch); err != nil {
Expand All @@ -73,17 +76,6 @@ func setOperatorProgressStatus(rc *ReconciliationContext, newState api.ProgressS

monitoring.UpdateOperatorDatacenterProgressStatusMetric(rc.Datacenter, newState)

// The allow-upgrade=once annotation is temporary and should be removed after first successful reconcile
if metav1.HasAnnotation(rc.Datacenter.ObjectMeta, api.UpdateAllowedAnnotation) && rc.Datacenter.Annotations[api.UpdateAllowedAnnotation] == string(api.AllowUpdateOnce) {
// remove the annotation
patch = client.MergeFrom(rc.Datacenter.DeepCopy())
delete(rc.Datacenter.ObjectMeta.Annotations, api.UpdateAllowedAnnotation)
if err := rc.Client.Patch(rc.Ctx, rc.Datacenter, patch); err != nil {
rc.ReqLogger.Error(err, "error removing the allow-upgrade=once annotation")
return err
}
}

return nil
}

Expand All @@ -101,5 +93,16 @@ func setDatacenterStatus(rc *ReconciliationContext) error {
return err
}

// The allow-upgrade=once annotation is temporary and should be removed after first successful reconcile
if metav1.HasAnnotation(rc.Datacenter.ObjectMeta, api.UpdateAllowedAnnotation) && rc.Datacenter.Annotations[api.UpdateAllowedAnnotation] == string(api.AllowUpdateOnce) {
// remove the annotation
patch := client.MergeFrom(rc.Datacenter.DeepCopy())
delete(rc.Datacenter.ObjectMeta.Annotations, api.UpdateAllowedAnnotation)
if err := rc.Client.Patch(rc.Ctx, rc.Datacenter, patch); err != nil {
rc.ReqLogger.Error(err, "error removing the allow-upgrade=once annotation")
return err
}
}

return nil
}
8 changes: 4 additions & 4 deletions pkg/reconciliation/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func CreateReconciliationContext(
}

rc.ReqLogger = rc.ReqLogger.
WithValues("datacenterName", dc.SanitizedName()).
WithValues("datacenterName", dc.LabelResourceName()).
WithValues("clusterName", dc.Spec.ClusterName)

log.IntoContext(ctx, rc.ReqLogger)
Expand Down Expand Up @@ -146,8 +146,8 @@ func (rc *ReconciliationContext) validateDatacenterNameConflicts() []error {
errs = append(errs, fmt.Errorf("failed to list CassandraDatacenters in namespace %s: %w", dc.Namespace, err))
} else {
for _, existingDc := range cassandraDatacenters.Items {
if existingDc.SanitizedName() == dc.SanitizedName() && existingDc.Name != dc.Name {
errs = append(errs, fmt.Errorf("datacenter name/override %s/%s is already in use by CassandraDatacenter %s/%s", dc.Name, dc.SanitizedName(), existingDc.Name, existingDc.SanitizedName()))
if existingDc.LabelResourceName() == dc.LabelResourceName() && existingDc.Name != dc.Name {
errs = append(errs, fmt.Errorf("datacenter name/override %s/%s is already in use by CassandraDatacenter %s/%s", dc.Name, dc.LabelResourceName(), existingDc.Name, existingDc.LabelResourceName()))
}
}
}
Expand All @@ -164,7 +164,7 @@ func (rc *ReconciliationContext) validateDatacenterNameOverride() []error {
return errs
} else {
if *dc.Status.DatacenterName != dc.Spec.DatacenterName {
errs = append(errs, fmt.Errorf("datacenter %s name override '%s' cannot be changed after creation to '%s'.", dc.Name, dc.Spec.DatacenterName, *dc.Status.DatacenterName))
errs = append(errs, fmt.Errorf("datacenter %s name override '%s' cannot be changed after creation to '%s'", dc.Name, dc.Spec.DatacenterName, *dc.Status.DatacenterName))
}
}

Expand Down
8 changes: 5 additions & 3 deletions pkg/reconciliation/handler_reconcile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

func TestReconcile(t *testing.T) {
var (
name = "cluster-example-cluster"
name = "dc1-example"
namespace = "default"
size int32 = 2
)
Expand Down Expand Up @@ -74,6 +75,7 @@ func TestReconcile(t *testing.T) {
Client: fakeClient,
Scheme: s,
Recorder: record.NewFakeRecorder(100),
Log: ctrl.Log.WithName("controllers").WithName("CassandraDatacenter"),
}

request := reconcile.Request{
Expand All @@ -88,8 +90,8 @@ func TestReconcile(t *testing.T) {
t.Fatalf("Reconciliation Failure: (%v)", err)
}

if result != (reconcile.Result{Requeue: true, RequeueAfter: 2 * time.Second}) {
t.Error("Reconcile did not return a correct result.")
if result != (reconcile.Result{Requeue: true, RequeueAfter: 10 * time.Second}) {
t.Errorf("Reconcile did not return a correct result. (%v)", result)
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/reconciliation/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,11 @@ func TestConflictingDcNameOverride(t *testing.T) {
Spec: api.CassandraDatacenterSpec{
ClusterName: "cluster1",
DatacenterName: "CassandraDatacenter_example",
}}}
},
Status: api.CassandraDatacenterStatus{
DatacenterName: ptr.To[string]("CassandraDatacenter_example"),
},
}}
})

errs := rc.validateDatacenterNameConflicts()
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciliation/reconcile_configsecret.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func getConfigFromConfigSecret(dc *api.CassandraDatacenter, secret *corev1.Secre

// getDatacenterConfigSecretName The format is clusterName-dcName-config
func getDatacenterConfigSecretName(dc *api.CassandraDatacenter) string {
return api.CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.SanitizedName() + "-config"
return api.CleanupForKubernetes(dc.Spec.ClusterName) + "-" + dc.LabelResourceName() + "-config"
}

// getDatacenterConfigSecret Fetches the secret from the api server or creates a new secret
Expand Down
Loading
Loading