Skip to content

[FLINK-31849] Adds basic CRD validations#1144

Open
vsantwana wants to merge 1 commit into
apache:mainfrom
vsantwana:FLINK-31849-crd-validation
Open

[FLINK-31849] Adds basic CRD validations#1144
vsantwana wants to merge 1 commit into
apache:mainfrom
vsantwana:FLINK-31849-crd-validation

Conversation

@vsantwana

@vsantwana vsantwana commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

What is the purpose of the change

This pull request moves simple, single-field spec validation from the operator's imperative DefaultValidator into declarative CRD-generator annotations, so these constraints are enforced by the Kubernetes API server at admission time rather than only during reconciliation. This gives users immediate, schema-level feedback on kubectl apply and simplifies the validation logic in the operator.

Brief change log

  • Annotated four spec fields with CRD-generator validation annotations: @min(1) on JobManagerSpec.replicas and TaskManagerSpec.replicas, and @required on FlinkDeploymentSpec.flinkVersion and IngressSpec.template
  • Regenerated the flinkdeployments and flinkbluegreendeployments CRDs, which now carry minimum: 1 on the replicas fields and flinkVersion/template in their required lists
  • Removed the now-redundant imperative checks from DefaultValidator (including the validateTmReplicas method), keeping only the validation that genuinely cannot be expressed declaratively (e.g. the conditional parallelism fallback, the JM replicas > 1 requires HA cross-field rule, and the ingress URL parsing)

Verifying this change

  • Added FlinkConfigurationYamlSupportTest#crdRejectsInvalidSpecValues, which applies the regenerated CRD to an in-process API server (kubeapitest) and verifies that resources violating each new constraintare rejected at admission, while a fully specified deployment is accepted
  • Updated DefaultValidatorTest to drop the now-removed imperative checks; the remaining 41 validator tests pass, confirming no regression in the validation that stays in the operator

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: yes.
  • Core observer or reconciler logic that is regularly executed: no (validation logic only; no observer/reconciler changes)

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@vsantwana

Copy link
Copy Markdown
Contributor Author

@gyfora I think we can expand it.

I just added single-field constraints that map cleanly to the CRD schema with zero behavior change for already-valid resources. The same approach can be extended incrementally:
More single-field constraints: additional @Min/@Max/@Pattern/@SiZe annotations, like

  • FlinkDeploymentSpec.imagePullPolicy → @pattern("Always|Never|IfNotPresent") (currently unvalidated free-form string; net-new constraint).
  • FlinkSessionJobSpec.deploymentName → @pattern("a-z?") — the regex already exists in DefaultValidator.validateDeploymentName; this would move
    the session-job call to the schema (the metadata.name call stays, since metadata can't be annotated).

@required on always-mandatory fields: e.g. serviceAccount and deploymentName. These are already mandatory in DefaultValidator, so the only change is moving rejection to admission time but I am not sure about this because it is a user-visible UX change

Cross-field rules via CEL:
@ValidationRule (x-kubernetes-validations, K8s 1.25+) can express constraints like the snapshot "exactly one of checkpoint or savepoint" XOR, or immutability rules using transition rules (oldSelf). But this is very stretch goal.

@vsantwana
vsantwana force-pushed the FLINK-31849-crd-validation branch from 5a74f8b to 7a6d3b1 Compare June 22, 2026 16:48
@vsantwana

Copy link
Copy Markdown
Contributor Author

We cannot ship this as long as have support for < 1.30.

Screenshot 2026-06-23 at 1 57 01 PM

@Dennis-Mircea Dennis-Mircea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd say this PR touches a validation area that needs a more careful look, deeper investigation, and proper testing before it moves forward.

As it stands it feels a bit superficial, since it also misses a number of other fields that could be validated the same way, for example:

  • FlinkDeploymentSpec.serviceAccount -> @Required (currently in validateServiceAccount, same pattern as flinkVersion).
  • FlinkSessionJobSpec.deploymentName -> @Required + @Pattern (the DEPLOYMENT_NAME_PATTERN regex already exists).

Before marking this PR as ready for review, please make sure every single-field constraint in DefaultValidator that can be expressed declaratively is actually covered by an annotation. The checks that genuinely cannot move (cross-field or config-derived rules, e.g. the replicas > 1 requires-HA rule, the parallelism fallback, ingress URL parsing) should be the only ones left imperative, and ideally called out as such.

return Optional.of("Flink Version must be defined.");
// Presence is enforced by the CRD schema (@Required on
// FlinkDeploymentSpec.flinkVersion); nothing further to validate here.
return Optional.empty();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new validation is admission-only and does not cover the reconcile cycle. The new added constraints are enforced only at admission, and only if the updated CRD is installed, which is a manual step (Helm does not auto-upgrade crds/). DefaultValidator, however, runs on every reconcile as well, so removing these checks drops the runtime validation checks: after an operator bump where the CRD has not been replaced yet, an invalid spec reaching reconcile is no longer caught and hits deploy logic with a bad value.

* Maximum number of retries before the snapshot is considered as failed. Set to -1 for
* unlimited or 0 for no retries.
*/
@Min(-1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't look as a necessary validation constraint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants