Skip to content

chore: Remove validation from reconcile loop #7

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

Merged
TeddyAndrieux merged 2 commits into from
Nov 12, 2025
Merged

chore: Remove validation from reconcile loop #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f80908a
chore: Remove pointer in struct when not needed
TeddyAndrieux Nov 7, 2025
6775a3b
chore: Remove validate from reconcile loop
TeddyAndrieux Nov 7, 2025
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
20 changes: 10 additions & 10 deletions api/v1alpha1/managedcrl_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ type CRLExposeSpec struct {

// Image specifies the container image to use for exposing the CRL.
// +optional
Image *ImageSpec `json:"image"`
Image ImageSpec `json:"image,omitempty"`
// Node Selector to deploy the CRL server
// +optional
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
Expand Down Expand Up @@ -128,8 +128,7 @@ type RevocationSpec struct {
RevocationTime *metav1.Time `json:"revocationTime,omitempty"`

// Reason is the reason for revocation (refer to RFC 5280 Section 5.3.1.).
// +optional
ReasonCode *int `json:"reasonCode,omitempty"`
ReasonCode int `json:"reasonCode,omitempty"`
}

// ManagedCRLSpec defines the desired state of ManagedCRL.
Expand Down Expand Up @@ -300,15 +299,9 @@ func (rs *RevocationSpec) withDefaults() {
if rs.RevocationTime == nil {
rs.RevocationTime = &metav1.Time{Time: metav1.Now().Time}
}
if rs.ReasonCode == nil {
rs.ReasonCode = ptr.To(0) // Unspecified
}
}

func (ces *CRLExposeSpec) withDefaults() {
if ces.Image == nil {
ces.Image = &ImageSpec{}
}
ces.Image.withDefaults()
Copy link

@anthony-treuillier-scality anthony-treuillier-scality Nov 12, 2025

Choose a reason for hiding this comment

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

Normally no default needed as the field is required.

Copy link
Contributor Author

@TeddyAndrieux TeddyAndrieux Nov 12, 2025

Choose a reason for hiding this comment

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

This is no longer mandatory

(I'm not really agree with this statement, it's not because a field is mandatory that you do not have "sub fields" with defaults that should be set)

Copy link

@anthony-treuillier-scality anthony-treuillier-scality Nov 12, 2025

Choose a reason for hiding this comment

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

"mouais", in that case, you validate instead of defaulting
But no way, the field is no more mandatory


if ces.Ingress != nil {
Expand Down Expand Up @@ -340,6 +333,7 @@ func (is *IngressSpec) withDefaults() {

// Validate validates the ManagedCRL resource.
func (mcrl *ManagedCRL) Validate() error {
mcrl.WithDefaults()
err := mcrl.Spec.validate()
if err != nil {
return fmt.Errorf("spec validation failed: %w", err)
Expand Down Expand Up @@ -439,12 +433,14 @@ func (rs RevocationSpec) ToRevocationListEntry() (x509.RevocationListEntry, erro
return x509.RevocationListEntry{
SerialNumber: serial,
RevocationTime: rs.RevocationTime.Time,
ReasonCode: *rs.ReasonCode,
ReasonCode: rs.ReasonCode,
}, nil
}

// GetRevokedListEntries converts the Revocations in ManagedCRLSpec to a slice of x509.RevocationListEntry.
func (mcrls *ManagedCRLSpec) GetRevokedListEntries() ([]x509.RevocationListEntry, error) {
mcrls.withDefaults()

if mcrls.Revocations == nil {
return []x509.RevocationListEntry{}, nil
}
Expand All @@ -462,6 +458,8 @@ func (mcrls *ManagedCRLSpec) GetRevokedListEntries() ([]x509.RevocationListEntry

// GetImage returns the full image string in the format "repository/name:tag".
func (is *ImageSpec) GetImage() string {
is.withDefaults()

image := fmt.Sprintf("%s:%s", *is.Name, *is.Tag)
if is.Repository != nil {
image = fmt.Sprintf("%s/%s", *is.Repository, image)
Expand All @@ -471,6 +469,8 @@ func (is *ImageSpec) GetImage() string {

// GetCRLDistributionPoint returns the CRL distribution point URL based on the Ingress configuration.
func (mcrl *ManagedCRL) GetCRLDistributionPoint() []string {
mcrl.WithDefaults()

var urls []string

// Add Ingress URLs if enabled
Expand Down
11 changes: 1 addition & 10 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions internal/controller/managedcrl_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,6 @@ func (r *ManagedCRLReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
// Apply defaults
instance.WithDefaults()
if err := instance.Validate(); err != nil {
return ctrl.Result{}, fmt.Errorf("validation failed: %w", err)
}

needRenewal := false
original := instance.DeepCopy()
Expand Down
1 change: 0 additions & 1 deletion internal/webhook/v1alpha1/managedcrl_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ func (v *ManagedCRLCustomValidator) ValidateDelete(ctx context.Context, obj runt

// validationManagedCRL validates the ManagedCRL fields.
func validationManagedCRL(logger logr.Logger, ctx context.Context, c client.Client, managedcrl *crloperatorv1alpha1.ManagedCRL) error {
managedcrl.WithDefaults()
if err := managedcrl.Validate(); err != nil {
Copy link

@anthony-treuillier-scality anthony-treuillier-scality Nov 12, 2025

Choose a reason for hiding this comment

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

Why ? validation does not expected some default fields

Copy link
Contributor Author

@TeddyAndrieux TeddyAndrieux Nov 12, 2025

Choose a reason for hiding this comment

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

The withDefaults as been moved to the Validate function directly

logger.Error(err, "Validation failed")
return err
Expand Down
11 changes: 3 additions & 8 deletions test/integration/managedcrl_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ var (
spec: crloperatorv1alpha1.ManagedCRLSpec{
Expose: &crloperatorv1alpha1.CRLExposeSpec{
Enabled: true,
Image: &crloperatorv1alpha1.ImageSpec{Repository: ptr.To("custom/repo"), Tag: ptr.To("v1.2.3")},
Image: crloperatorv1alpha1.ImageSpec{Repository: ptr.To("custom/repo"), Tag: ptr.To("v1.2.3")},
Internal: ptr.To(false),
},
},
Expand Down Expand Up @@ -401,7 +401,7 @@ var _ = Describe("ManagedCRL Controller", func() {
retrieved.Spec.Revocations = []crloperatorv1alpha1.RevocationSpec{
{
SerialNumber: "123456789",
ReasonCode: ptr.To(2),
ReasonCode: 2,
},
}
Expect(k8sClient.Update(ctx, retrieved)).To(Succeed())
Expand All @@ -412,7 +412,7 @@ var _ = Describe("ManagedCRL Controller", func() {
retrieved.Spec.Revocations = []crloperatorv1alpha1.RevocationSpec{
{
SerialNumber: "123456789",
ReasonCode: ptr.To(1),
ReasonCode: 1,
},
}
Expect(k8sClient.Update(ctx, retrieved)).To(Succeed())
Expand Down Expand Up @@ -510,7 +510,6 @@ func checkSecret(mcrlRef types.NamespacedName) {
}
return false
}, 10*time.Second, time.Second).Should(BeTrue())
retrieved.WithDefaults()

Expect(retrieved.ObjectMeta.Finalizers).To(ContainElement("crl-operator.scality.com/finalizer"))

Expand Down Expand Up @@ -576,7 +575,6 @@ func checkExposePod(mcrlRef types.NamespacedName, shouldRestart bool) {
}
return false
}, 10*time.Second, time.Second).Should(BeTrue())
retrieved.WithDefaults()
Expect(retrieved.Status.PodExposed).To(PointTo(BeFalse()))

// Check the deployment
Expand Down Expand Up @@ -615,7 +613,6 @@ func checkExposePod(mcrlRef types.NamespacedName, shouldRestart bool) {
}
return false
}, 10*time.Second, time.Second).Should(BeTrue())
retrieved.WithDefaults()

Expect(retrieved.Status.PodExposed).To(PointTo(BeTrue()))

Expand Down Expand Up @@ -663,7 +660,6 @@ func checkIngress(mcrlRef types.NamespacedName) {
}
return false
}, 10*time.Second, time.Second).Should(BeTrue())
retrieved.WithDefaults()

Expect(retrieved.Status.IngressExposed).To(PointTo(BeTrue()))

Expand Down Expand Up @@ -707,7 +703,6 @@ func checkIssuerConfigured(mcrlRef types.NamespacedName) {
}
return false
}, 10*time.Second, time.Second).Should(BeTrue())
retrieved.WithDefaults()

Expect(retrieved.Status.IssuerConfigured).To(PointTo(BeTrue()))

Expand Down