Skip to content

Conversation

cbandy
Copy link
Member

@cbandy cbandy commented Sep 15, 2025

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

This tool helped me understand the cost of CEL validation rules as I was writing them. The second and third commits are optional; I followed some of the recommendations emitted by the tool.

The output for PGUpgrades looks like this:

go run github.com/openshift/crd-schema-checker/cmd/crd-schema-checker@latest check-manifests --disabled-validators 'NoBools,NoMaps' --new-crd-filename 'config/crd/bases/postgres-operator.crunchydata.com_pgupgrades.yaml' 2>&1 | awk -f hack/check-manifests.awk                                                                                                                          
ERROR: "ListsMustHaveSSATags": crd/pgupgrades.postgres-operator.crunchydata.com version/v1beta1 field/^.spec.imagePullSecrets must set x-kubernetes-list-type                                 
info: "CostBudget": ^.spec: Field has a maximum cardinality of 1.                                                                                                                             
info: "CostBudget": ^.spec: Rule 0 raw cost is 5. Estimated total cost of 5. Rule is 0.00% of allowed budget, 10M.                                                                            
info: "CostBudget": ^.spec: Field has a maximum cardinality of 1.                                                                                                                             
info: "CostBudget": ^.spec: Rule 1 raw cost is 19. Estimated total cost of 19. Rule is 0.00% of allowed budget, 10M.                                                                          
info: "CostBudget": ^.spec: Field has a maximum cardinality of 1.                                                                                                                             
info: "CostBudget": ^.spec: Rule 2 raw cost is 20. Estimated total cost of 20. Rule is 0.00% of allowed budget, 10M.
exit status 1

Makefile Outdated
Comment on lines 266 to 268
define NL


endef
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Defining a newline? Does the awk on l. 156 need a newline?

Copy link
Member Author

@cbandy cbandy Sep 15, 2025

Choose a reason for hiding this comment

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

What is this?
Defining a newline?

Yeah, this is a Make variable that contains one newline.

Does the awk on l. 156 need a newline?

Without the newline, Make prints all the go run commands at once, then four outputs from the tool come out. With this, each command and its output are separated like this:

go run github.com/openshift/crd-schema-checker/cmd/crd-schema-checker@latest check-manifests --new-crd-filename 'config/crd/bases/postgresoperator.crunchydata.com_crunchybridgeclusters.yaml' 2>&1 | awk -f hack/check-manifests.awk
##
## wall of log lines
##
go run github.com/openshift/crd-schema-checker/cmd/crd-schema-checker@latest check-manifests --new-crd-filename 'config/crd/bases/postgres-operator.crunchydata.com_pgadmins.yaml' 2>&1 | awk -f hack/check-manifests.awk
##
## wall of log lines
##
go run github.com/openshift/crd-schema-checker/cmd/crd-schema-checker@latest check-manifests --new-crd-filename 'config/crd/bases/postgres-operator.crunchydata.com_pgupgrades.yaml' 2>&1 | awk -f hack/check-manifests.awk
##
## wall of log lines
##
go run github.com/openshift/crd-schema-checker/cmd/crd-schema-checker@latest check-manifests --new-crd-filename 'config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml' 2>&1 | awk -f hack/check-manifests.awk
##
## wall of log lines
##

🤔 What's a better way to present / read this stuff?

@@ -1307,6 +1307,7 @@ spec:
type: object
type: object
type: array
x-kubernetes-list-type: atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are all these lists getting flagged for atomic? Is that the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are all these lists getting flagged for atomic?

Because the destination for these values is an atomic list in Kubernetes; https://pkg.go.dev/k8s.io/api/core/v1#PodSpec.Tolerations

	// If specified, the pod's tolerations.
	// +optional
	// +listType=atomic
	Tolerations [][Toleration](https://pkg.go.dev/k8s.io/api/core/v1#Toleration)

Is that the default?

Not the way I think you're asking, but also yes, that's my understanding.

https://docs.k8s.io/reference/using-api/server-side-apply#custom-resources-and-server-side-apply

By default, Server-Side Apply treats custom resources as unstructured data. All keys are treated the same as struct fields, and all lists are considered atomic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are all these lists getting flagged for atomic?

I've added the +listType=atomic marker to our Go structs in /pkg.

Copy link
Contributor

@benjaminjb benjaminjb left a comment

Choose a reason for hiding this comment

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

No blockers

This tool estimates the total and relative cost of CEL validation rules.

See: https://github.com/openshift/crd-schema-checker
The "ListsMustHaveSSATags" checker identifies this issue.
This change is backward compatible because unmarked lists are atomic.

See: https://docs.k8s.io/reference/using-api/server-side-apply#custom-resources-and-server-side-apply
The "ListsMustHaveSSATags" checker identifies this issue.
This change is backward compatible because unmarked lists are atomic.

See: https://docs.k8s.io/reference/using-api/server-side-apply#custom-resources-and-server-side-apply
@benjaminjb
Copy link
Contributor

Just thinking aloud here, but this make target isn't wired up to any other make target, so we might want to write / think about some guidance about when to use it. It's not just for checking CEL rule budget, yeah?

@cbandy
Copy link
Member Author

cbandy commented Sep 16, 2025

Just thinking aloud here, but this make target isn't wired up to any other make target, so we might want to write / think about some guidance about when to use it. It's not just for checking CEL rule budget, yeah?

I don't understand the tool enough to give any guidance, really. I used it to check CEL budgets, but I have no idea how accurate that was (or if accuracy even matters).

When we understand the tool better, we can configure it and wire it up as a blocking check.

  • I'm curious about the old → new concept. It sounds like it can detect not-good changes to a CRD.
  • This file has a list of checks and some ideas for others. Are these all enabled?
  • Our strategy for v1 relies on validation ratcheting. What are these calls to RatchetCompare?

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.

3 participants