-
Notifications
You must be signed in to change notification settings - Fork 567
CNTRLPLANE-3173: persist lastSuccessfulEtcdBackupURL in HostedCluster status #8179
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.
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
Changes from all commits
2f173f9
dac4638
3a68b71
647381e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1953,7 +1953,8 @@ type ManagedEtcdStorageSpec struct { | |
| // +kubebuilder:validation:MaxItems=1 | ||
| // +kubebuilder:validation:items:MaxLength=1024 | ||
| // +kubebuilder:validation:XValidation:rule="self.size() <= 1", message="RestoreSnapshotURL shouldn't contain more than 1 entry" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="restoreSnapshotURL is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="restoreSnapshotURL is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self.size() == 0 || self[0].matches('^(https|s3)://.*')", message="restoreSnapshotURL must be a valid URL with scheme https or s3" | ||
| RestoreSnapshotURL []string `json:"restoreSnapshotURL,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -2180,6 +2181,16 @@ type HostedClusterStatus struct { | |
| // configuration contains the cluster configuration status of the HostedCluster | ||
| // +optional | ||
| Configuration *ConfigurationStatus `json:"configuration,omitempty"` | ||
|
|
||
| // lastSuccessfulEtcdBackupURL is the cloud storage URL of the most recent | ||
| // successful etcd backup snapshot. Persisted here because HCPEtcdBackup CRs | ||
| // are ephemeral and may be deleted by retention policies. | ||
|
Comment on lines
+2185
to
+2187
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General questions for context:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, instead of adding a new field to the status have you considered having a different semantic for retention enforcement such that retention enforcement doesn't ever remove the most recently successful backup? When it comes to status fields, I try to evaluate whether or not this information is inherently useful to the consumer of the field - which on the |
||
| // +openshift:enable:FeatureGate=HCPEtcdBackup | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^(https|s3)://.*')",message="lastSuccessfulEtcdBackupURL must be a valid URL with scheme https or s3" | ||
| LastSuccessfulEtcdBackupURL string `json:"lastSuccessfulEtcdBackupURL,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URLs often have a formatting constraint to ensure that only valid URLs are accepted as input. I double checked that the earliest supported version for running HyperShift is OCP 4.14 which runs k8s 1.27 so we should be OK to use the URL CEL library (https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-url-library) to validate that the input parses as a valid URL. Are there any additional constraints on the URL being provided as an input? For example, can it contain query parameters, fragments, userinfo? Does the scheme have to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the CEL URL library availability. The URL here is a cloud storage object URL like I'll look into adding CEL validation with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this please have the same validation added to HCPEtcdBackupStatus for the same field
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and we can include this as well in the envtest etcd test suite
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — added the same CEL URL scheme validation to CRDs regenerated.
Tracked in CNTRLPLANE-3214 as a follow-up.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this a follow up, can we please include here as we are introducing a new field
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the new field introduced by this PR LastSuccessfulEtcdBackupURL still doesn't seem to have coverage in envtest |
||
| } | ||
|
|
||
| // AutoNodeStatus contains the observed state of the AutoNode provisioner. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please include a test / suite for that covers all cases for this field immutability https://github.com/openshift/hypershift/tree/main/cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not in this PR, but can please follow up to do the same for the etcdbackup API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in #8186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created follow-up ticket: CNTRLPLANE-3214 — "Add CEL immutability validation and CRD tests for HCPEtcdBackup API"