Reviewal and Proposal for CRD Validation

[Joe-Bresee]

Audit of kustomize-controller CRD Validations

Related to: fluxcd/flux2#2993

This issue tracks the audit of kustomize-controller CRD validations. I plan on implementing the accepted changes once reviewed.

---

Initial Findings

I have grouped validations by struct. The first struct here is KustomizationSpec, and I will list annotations for each type in the struct. Then I review individual structs and reference types.

---

kustomization_types.go

KustomizationSpec

• CommonMetadata: Ok
• DependsOn: maxItems
• Decryption: Ok
• Interval: min 60s; Suggest XValidation: Interval >= 60s
• RetryInterval: min 60s; Suggest XValidation: RetryInterval >= 60s
• KubeConfig: Ok
• Path: min/max length, path structure pattern matching
• PostBuild: Ok
• Prune: Ok
• DeletionPolicy: Ok
• HealthChecks: maxItems. Backed by meta.NamespacedObjectKindReference, which currently has no DNS-1123 validation. Flagged for potential follow-up in the shared meta API rather than addressing locally.
• NamePrefix: maxLength, perhaps pattern matching later on to help with the full name matching DNS1123. Recommend removing +kubebuilder:validation:Optional because +optional is already included
• NameSuffix: maxLength. Same as above
• Patches: Ok. Maybe add maxItems
• Images: Ok. Maybe add maxItems
• ServiceAccountName: min/max string length, DNS-1123
• SourceRef: Ok
  Suspend: Optional boolean defaults are inconsistent across the spec. Add default: false to all optional boolean fields for consistency and improved clarity in kubectl explain. This would make the default behavior explicit and observable at the API level and should be treated as an API change.
• TargetNamespace: DNS-1123. Remove one of the +Optionals
• Timeout: pattern for min/max timeout length, e.g., maximum 24h, or XValidation: Timeout >= 1s
• Force: See Suspend note
• Wait: See Suspend note
• Components: maxItems
• IgnoreMissingComponents: See Suspend note
• HealthCheckExprs: maxItems

---

Individual Structs

CommonMetadata

• Annotations and Labels: add maxProperties, matchLength on values/keys

Decryption

• ServiceAccountName: add DNS-1123 and min/max length checks

PostBuild

• Substitute: maxProperty validation
• SubstituteFrom: maxItems

SubstituteReference

• Name: DNS-1123
• Optional: see Suspend note

---

reference_types.go

CrossNamespaceSourceReference

• APIVersion: min/max length, pattern validation
• Kind: Ok
• Name, Namespace: DNS-1123, min/max length

DependencyReference

• Name, Namespace: min/max length, DNS-1123
• ReadyExpr: CEL pattern validation, min/max string length

---

Backwards Compatibility

• No missing validations noticed from earlier api versions.

Below I'm going to organize these issues into low vs. high risk changes, i.e., low being most likely to implement without big consequences.

Low Risk

KustomizationSpec

• DependsOn: maxItems
• Interval: min 60s; Suggest XValidation: Interval >= 60s
• RetryInterval: min 60s; Suggest XValidation: RetryInterval >= 60s
• Path: min/max length only
• HealthChecks: maxItems. Backed by meta.NamespacedObjectKindReference, noted for potential shared-type follow-up.
• NamePrefix: maxLength. Remove +kubebuilder:validation:Optional (already optional)
• NameSuffix: maxLength. Remove +kubebuilder:validation:Optional
• Patches: maxItems
• Images: maxItems
• ServiceAccountName: min/max string length, DNS-1123
• TargetNamespace: DNS-1123. Remove one of the +Optionals
• Components: maxItems
• IgnoreMissingComponents: See Suspend note (deferred for now)
• HealthCheckExprs: maxItems

CommonMetadata

• Annotations and Labels: add maxProperties, maxLength on keys, maxLength on values

Decryption

• ServiceAccountName: add DNS-1123 and min/max length checks

PostBuild

• Substitute: maxProperties validation
• SubstituteFrom: maxItems

SubstituteReference

• Name: DNS-1123
• Optional: deferred (boolean default, high risk)

reference_types.go

CrossNamespaceSourceReference

• APIVersion: min/max length
• Kind: Ok
• Name, Namespace: DNS-1123, min/max length

DependencyReference

• Name, Namespace: min/max length, DNS-1123
• ReadyExpr: CEL pattern validation, min/max string length

---

High Risk

• Path: path structure pattern matching (regex)
• meta.NamespacedObjectKindReference: DNS-1123 validation (shared-type issue; out of scope here)
• Optional boolean defaults (Suspend, Force, Wait, IgnoreMissingComponents): would make defaults visible at API level and should be treated as an API change

[Joe-Bresee]

@kthurman59, linking you to this issue to look over my fields and markers

[kthurman59]

Overall this matches what 2993 is asking for. The struct by struct audit is the right start. Also, it looks super nice!

A few things to tighten before you implement changes. Keep in mind, this is just advice. I understand if you wanna go your own way or disagree.

Interval and RetryInterval:
Flux docs set the minimum to 60s. If we enforce a minimum, use 60s, not 1s. I would avoid adding a max like 24h unless we can point to a spec requirement or a real problem that needs it.

NamePrefix and NameSuffix:
Be careful with DNS 1123 validation here. These are fragments, not necessarily standalone object names, and strict DNS 1123 rules can reject common prefix or suffix conventions even when the final rendered name is valid. Safer first pass is maxLength only.

CommonMetadata labels and annotations:
Do not validate keys as DNS 1123. Kubernetes key rules are different and DNS 1123 would reject valid keys. Low risk improvements are maxProperties, maxLength on values, and maxLength on keys if we want a cap without enforcing format. Reviewing the contributing guidelines will help clear this up.

Reference types and HealthChecks:
If meta reference structs are missing validations, the clean fix is in the shared type, not repeated per field in kustomize controller. That avoids controllers drifting from each other over time.

Defaults for optional bools:
Adding default false can improve kubectl explain clarity, but do it consistently and treat it as behavior visible at the API level.

Backwards compatibility
Any new CRD validation is apiserver enforced and can break existing objects. We should assume this is a breaking change until proven otherwise. Tests need to cover both valid and invalid examples, and we should keep the initial set of validations to the ones we are confident will not block real world usage.

Implementation wise, I would start with the low risk caps first, then tighten patterns only where docs already define the rule. If it would be helpful, feel free to post a small diff early and I will review it.

I hope this helped you. If you have any more questions, just let me know. I am always glad to help in anyway.

Just let me know! Thx!

[Joe-Bresee]

Thanks a ton for your input- makes a big difference. I updated some areas in the issue to align with your suggestions - which make sense to me.
I also made a table of my opinion of low vs high risk changes. I propose implementing low-risk improvements first, and leave the higher risk issues up for discussion. If the scope and fixes seems appropriate, I can open a PR with these changes