Skip to content

Fix Issue 2023: Validate VolumeClaimTemplate overrides contain spec and provide helpful error messages when they don't #2024

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.

Sign up for GitHub

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

Open
jmealo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Issue 2023: Validate VolumeClaimTemplate overrides contain spec and provide helpful error messages when they don't #2024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions api/v1beta1/rabbitmqcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,10 @@ type PersistentVolumeClaim struct {

// Spec defines the desired characteristics of a volume requested by a pod author.
// More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#persistentvolumeclaims
// +optional
Spec corev1.PersistentVolumeClaimSpec `json:"spec,omitempty" protobuf:"bytes,2,opt,name=spec"`
// When using override.statefulSet.spec.volumeClaimTemplates, you must provide the complete
// template including spec.resources.requests.storage. Overrides replace the entire template.
// +kubebuilder:validation:Required
Spec corev1.PersistentVolumeClaimSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`
}

// TLSSpec allows for the configuration of TLS certificates to be used by RabbitMQ. Also allows for non-TLS traffic to be disabled.
Expand Down
2 changes: 2 additions & 0 deletions config/crd/bases/rabbitmq.com_rabbitmqclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5059,6 +5059,8 @@ spec:
volumeName:
type: string
type: object
required:
- spec
type: object
type: array
type: object
Expand Down
25 changes: 20 additions & 5 deletions controllers/reconcile_persistence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ import (

func (r *RabbitmqClusterReconciler) reconcilePVC(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster, desiredSts *appsv1.StatefulSet) error {
logger := ctrl.LoggerFrom(ctx)
desiredCapacity := persistenceStorageCapacity(desiredSts.Spec.VolumeClaimTemplates)
err := scaling.NewPersistenceScaler(r.Clientset).Scale(ctx, *rmq, desiredCapacity)
desiredCapacity, err := persistenceStorageCapacity(desiredSts.Spec.VolumeClaimTemplates)
if err != nil {
msg := fmt.Sprintf("Failed to determine PVC capacity: %s", err.Error())
logger.Error(err, msg)
Comment on lines +19 to +20
Copy link
Member

@Zerpet Zerpet Dec 15, 2025

Choose a reason for hiding this comment

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

You are already providing a lot of information below in the stack, in the return error of persistenceStorageCapacity(). In addition to that, the function logger.Error(err, any...) already prints the error, there's no need to template the error inside the message msg.

Suggested change
msg := fmt.Sprintf("Failed to determine PVC capacity: %s", err.Error())
logger.Error(err, msg)
logger.Error(err, "failed to determine PVC capacity")

r.Recorder.Event(rmq, corev1.EventTypeWarning, "FailedReconcilePersistence", msg)
Copy link
Member

@Zerpet Zerpet Dec 15, 2025

Choose a reason for hiding this comment

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

Setting the status condition is done higher up in the stack, it shouldn't be done here, inside a helper function.

cluster-operator/controllers/rabbitmqcluster_controller.go

Lines 228 to 231 in a25511e

if err = r.reconcilePVC(ctx, rabbitmqCluster, sts); err != nil {
r.setReconcileSuccess(ctx, rabbitmqCluster, corev1.ConditionFalse, "FailedReconcilePVC", err.Error())
return ctrl.Result{}, err
}

return err
}
err = scaling.NewPersistenceScaler(r.Clientset).Scale(ctx, *rmq, desiredCapacity)
if err != nil {
msg := fmt.Sprintf("Failed to scale PVCs: %s", err.Error())
logger.Error(fmt.Errorf("hit an error while scaling PVC capacity: %w", err), msg)
Expand All @@ -24,11 +30,20 @@ func (r *RabbitmqClusterReconciler) reconcilePVC(ctx context.Context, rmq *rabbi
return err
}

func persistenceStorageCapacity(templates []corev1.PersistentVolumeClaim) k8sresource.Quantity {
func persistenceStorageCapacity(templates []corev1.PersistentVolumeClaim) (k8sresource.Quantity, error) {
for _, t := range templates {
if t.Name == "persistence" {
return t.Spec.Resources.Requests[corev1.ResourceStorage]
storage := t.Spec.Resources.Requests[corev1.ResourceStorage]
if storage.IsZero() {
return storage, fmt.Errorf(
"PVC template 'persistence' has spec.resources.requests.storage=0 (or missing). " +
"If using override.statefulSet.spec.volumeClaimTemplates, you must provide " +
"the COMPLETE template including spec.resources.requests.storage. " +
"Overrides replace the entire volumeClaimTemplate, not merge with it")
Comment on lines +39 to +42
Copy link
Member

@Zerpet Zerpet Dec 15, 2025

Choose a reason for hiding this comment

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

This error message is too specific and leaks implementation details. Gemini suggests the following:

Suggested change
"PVC template 'persistence' has spec.resources.requests.storage=0 (or missing). " +
"If using override.statefulSet.spec.volumeClaimTemplates, you must provide " +
"the COMPLETE template including spec.resources.requests.storage. " +
"Overrides replace the entire volumeClaimTemplate, not merge with it")
"PVC template 'persistence' is missing the storage request. " +
"If you are overriding the persistence template, please provide the full template definition including storage request and metadata")

}
return storage, nil
}
}
return k8sresource.MustParse("0")
// No persistence template found - this is valid for ephemeral storage (storage: "0Gi")
return k8sresource.MustParse("0"), nil
}
2 changes: 2 additions & 0 deletions docs/api/rabbitmq.com.ref.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-

| *`spec`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#persistentvolumeclaimspec-v1-core[$$PersistentVolumeClaimSpec$$]__ | Spec defines the desired characteristics of a volume requested by a pod author.
More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#persistentvolumeclaims
When using override.statefulSet.spec.volumeClaimTemplates, you must provide the complete
template including spec.resources.requests.storage. Overrides replace the entire template.
|===


Expand Down
3 changes: 2 additions & 1 deletion internal/scaling/scaling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ func (p PersistenceScaler) Scale(ctx context.Context, rmq rabbitmqv1beta1.Rabbit

// desired storage capacity is smaller than the current capacity; we can't proceed lest we lose data
if err == nil && existingCapacity.Cmp(desiredCapacity) == 1 {
msg := "shrinking persistent volumes is not supported"
msg := fmt.Sprintf("shrinking persistent volumes is not supported (existing: %s, desired: %s)",
existingCapacity.String(), desiredCapacity.String())
logger.Error(errors.New("unsupported operation"), msg)
return errors.New(msg)
}
Expand Down
Loading