Adds the 'environment' Field to the PostgresCluster Spec

[andrewlecuyer]

An environment field is now available within the PostgresCluster spec, which allows the user to specify the infrastructure and/or stage in development the PostgresCluster is associated with. Accepted values for this field are production and development. production is the default.

Additionally, a CEL validation rule has been added to the CRD that requires backups to be configured for production PostgresCluster's. This change is fully backward compatible and non-breaking with existing PostgresCluster specs.

Issue: PGO-1645

[benjaminjb]

Worth adjusting the backups optional test to add a test for this?

[cbandy]

🤔 I forget how optional + default behaves. My memory is that the API fills in the default before it saves. If so, will we "always" have a value?

👍🏻 Pointer for optional seems right. If we don't like dereferencing it, we can change the struct later.

[andrewlecuyer]

If so, will we "always" have a value?

These semantics always trip me up too. But yea, like you mentioned, the field should/will always have a value, since the API will will in the default if nothing is provided by the user.

[cbandy]

⛏️ I wanna drop "PostgresCluster" here, too. Not sure why. I don't like the singular "environment" in my proposed message, so double not-sure.

🔧 I think we should use has() to check for the presence of a field. Is pgbackrest required within the backups section? If so, we can trust/defer to its own validation rules.

Suggested change

	// +kubebuilder:validation:XValidation:rule="(self.environment == 'production') ? self.backups.pgbackrest != null : true", message="Backups must be enabled for production PostgresCluster's."
	// +kubebuilder:validation:XValidation:rule="(self.environment == 'production') ? has(self.backups) : true", message="Backups must be enabled for production environment."

🌱 With controller-gen >= 0.16.0, we can use fieldPath to indicate more clearly that spec.backups is what's missing.

📝 If you have any doubts about the logic of the rule, you can add some tests like the ones in internal/testing/validation/postgrescluster_test.go.

[andrewlecuyer]

I went with the following message:

Backups must be enabled in a production environment.

I also switch the null check to has(). However, I did go one layer deeper with has(self.backups.pgbackrest) to protect against an empty backups section (backups: {}). I don't love the error messages displayed when this does fail, since the validation rule itself essentially fails due to the missing field. But for now this does appear to protect against everything we need it to, and it does seem like there is a fine line with keeping these errors pretty, and keeping the rules from getting too complex.

[andrewlecuyer]

I did attempt a bump to controller-gen 0.16.3, but saw something a bit strange with our RBAC (I'll capture in a story if no thoughts in the Slack thread I started). This did give me fieldPath and reason, though I found it was really only helpful if the user actually included the section (and the reason, while returned from the API, is displayed by kubectl). But this could again be tied to further expanding the rule a bit (e.g. per my last comment) to get some better validation error messages/behavior.

[andrewlecuyer]

I went ahead and made the bump to 0.16.3. fieldRef and reason have now been added to the CEL validation marker as a result.

Additionally, I made one final tweak to the validation rule:

(self.environment == 'production') ? (has(self.backups) && has(self.backups.pgbackrest)) : true

Now I get a clean/consistent error whether backups is empty or missing:

Missing backups:

$ kubectl apply -n postgres-operator -f - <<EOF
apiVersion: postgres-operator.crunchydata.com/v1beta1
kind: PostgresCluster
metadata:
  name: hippo
spec:
  environment: production                                                            
  image: registry.developers.crunchydata.com/crunchydata/crunchy-postgres:ubi8-16.3-1
  postgresVersion: 16
  instances:
    - name: instance1
      dataVolumeClaimSpec:
        accessModes:
        - "ReadWriteOnce"
        resources:
          requests:
            storage: 1Gi
EOF          
The PostgresCluster "hippo" is invalid: spec.backups: Required value: Backups must be enabled in a production environment.

Empty backups:

$ kubectl apply -n postgres-operator -f - <<EOF
apiVersion: postgres-operator.crunchydata.com/v1beta1
kind: PostgresCluster
metadata:
  name: hippo
spec:
  environment: production                                                            
  image: registry.developers.crunchydata.com/crunchydata/crunchy-postgres:ubi8-16.3-1
  postgresVersion: 16
  instances:
    - name: instance1
      dataVolumeClaimSpec:
        accessModes:
        - "ReadWriteOnce"
        resources:
          requests:
            storage: 1Gi
  backups: {}
EOF
The PostgresCluster "hippo" is invalid: spec.backups: Required value: Backups must be enabled in a production environment.