Skip to content

Adds the 'environment' Field to the PostgresCluster Spec #3993

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

Closed
wants to merge 5 commits into from
Closed

Adds the 'environment' Field to the PostgresCluster Spec #3993

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a22d726
Adds the 'environment' Field to the PostgresCluster Spec
andrewlecuyer Sep 13, 2024
a995cdd
Description & CEL Validation Updates for Environment
andrewlecuyer Sep 26, 2024
ccdcaa4
Update Controller Gen to v0.16.3
andrewlecuyer Sep 26, 2024
e0fb9bd
Cleanup environment Validation Rule and Add fieldPath & reason
andrewlecuyer Sep 26, 2024
049007c
Update Validation for 'environment' and 'backups'
andrewlecuyer Sep 27, 2024
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
17 changes: 17 additions & 0 deletions config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7599,6 +7599,19 @@ spec:
scheduling constraints will be used in addition to any custom constraints
provided.
type: boolean
environment:
default: production
description: |-
Environment allows PGO to adapt its behavior according to the specific infrastructure
and/or stage of development the PostgresCluster is associated with. This includes
requiring and/or loosening the requirements for certain components and settings, while
also providing deeper insights, events and status that more closely align with
PostgresCluster's intended use.
Defaults to “production”. Acceptable values are "development" and "production".
enum:
- production
- development
type: string
image:
description: |-
The image name to use for PostgreSQL containers. When omitted, the value
Expand Down Expand Up @@ -16874,6 +16887,10 @@ spec:
- instances
- postgresVersion
type: object
x-kubernetes-validations:
- message: Backups must be enabled for production PostgresCluster's.
rule: '(self.environment == ''production'') ? self.backups.pgbackrest
!= null : true'
status:
description: PostgresClusterStatus defines the observed state of PostgresCluster
properties:
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ spec:
pgbackrest:
repos: null
config: {}
environment: production
instances: null
patroni:
leaderLeaseDurationSeconds: 30
Expand Down Expand Up @@ -76,6 +77,7 @@ spec:
pgbackrest:
repos: null
config: {}
environment: production
instances:
- dataVolumeClaimSpec:
resources: {}
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
)

// PostgresClusterSpec defines the desired state of PostgresCluster
// +kubebuilder:validation:XValidation:rule="(self.environment == 'production') ? self.backups.pgbackrest != null : true", message="Backups must be enabled for production PostgresCluster's."
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

type PostgresClusterSpec struct {
// +optional
Metadata *Metadata `json:"metadata,omitempty"`
Expand Down Expand Up @@ -58,6 +59,17 @@ type PostgresClusterSpec struct {
// +optional
DisableDefaultPodScheduling *bool `json:"disableDefaultPodScheduling,omitempty"`

// Environment allows PGO to adapt its behavior according to the specific infrastructure
// and/or stage of development the PostgresCluster is associated with. This includes
// requiring and/or loosening the requirements for certain components and settings, while
// also providing deeper insights, events and status that more closely align with
// PostgresCluster's intended use.
// Defaults to “production”. Acceptable values are "development" and "production".
// +kubebuilder:validation:Enum={production,development}
// +kubebuilder:default=production
// +optional
Environment *string `json:"environment,omitempty"`
Comment on lines +67 to +69
Copy link
Member

Choose a reason for hiding this comment

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

🤔 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


// The image name to use for PostgreSQL containers. When omitted, the value
// comes from an operator environment variable. For standard PostgreSQL images,
// the format is RELATED_IMAGE_POSTGRES_{postgresVersion},
Expand Down Expand Up @@ -305,6 +317,11 @@ func (s *PostgresClusterSpec) Default() {
if s.UserInterface != nil {
s.UserInterface.Default()
}

if s.Environment == nil {
s.Environment = new(string)
*s.Environment = "production"
}
}

// Backups defines a PostgreSQL archive configuration
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/postgres-operator.crunchydata.com/v1beta1/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.

Loading