MG-86: Proposal for disableUpload in must-gather-operator spec #1838
Conversation
|
@rausingh-rh: This pull request references MG-86 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@rausingh-rh: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| ### User Stories | ||
|
|
||
| * As a cluster admin, I want to collect must-gather data without automatically uploading it to Red Hat's case management system so I can review the data locally first. |
There was a problem hiding this comment.
The disconnected case could also be a relevant use case where you don't have the ability to upload outside the airgapped env
|
|
||
| * **Prerequisites**: Requires Kubernetes 1.25+ for CEL validation support (enabled by default) | ||
| * **API**: Add `DisableUpload` to `MustGatherSpec` with kubebuilder default marker and CEL validation rules. | ||
| * **Validation**: Use kubebuilder CEL validation (`+kubebuilder:validation:XValidation`) to enforce that `caseID` and `caseManagementAccountSecretRef` are mandatory when `disableUpload` is false or unset. |
There was a problem hiding this comment.
ah..ok..that makes sense..should have read this first :)
| 2. **Validation Fallback**: For clusters < Kubernetes 1.25 (without CEL support), should we implement controller-level validation as fallback? | ||
| - **Impact**: Ensures consistent behavior across all supported Kubernetes versions | ||
| - **Complexity**: Requires dual validation paths | ||
| - **Decision Needed**: Is this necessary given OpenShift's Kubernetes version requirements? |
|
/lgtm |
| // A flag to control whether the must-gather bundle should be uploaded to SFTP server. | ||
| // If set to true, the bundle will be collected but not uploaded. | ||
| // +kubebuilder:default:=false | ||
| DisableUpload bool `json:"disableUpload,omitempty"` |
There was a problem hiding this comment.
@iamkirkbater suggested on the other EP, to use an API field like spec.uploadTarget. It sounds reasonable to add UnionDiscriminator type field that uploads only when set appropriately.
ref: https://github.com/openshift/api/blob/1517fca97fe327e076bcff90accb86d1f4e804ed/example/v1/types_stable.go#L69 of UnionDiscriminator
eg.
type MustGatherSpec struct {
// ...
// uploadTarget sets the target config for uploading the collected must-gather tar,
// uploading is disabled if this field is unset
//
// +optional
UploadTarget *UploadTarget `json:"uploadTarget,omitempty"`
// ...
}
// UploadType is a specific method for uploading to a target
// +kubebuilder:validation:Enum=SFTP
type UploadType string
// UploadTarget defines the configuration for uploading the must-gather tar ...
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'SFTP' ? has(self.sftp) : !has(self.sftp)",message="sftp upload target config is required when upload type is SFTP, and forbidden otherwise"
// +union
type UploadTarget struct {
// type defines the method used for uploading to a specific target
// Available types are SFTP only.
//
// +unionDiscriminator
// +required
Type UploadType `json:"type"`
// sftp defines the target details for uploading to a valid
// SFTP server.
//
// +unionMember
// +optional
SFTP *SFTPUploadTargetConfig `json:"sftp,omitempty"`
}
// SFTPUploadTargetConfig defines the ...
type SFTPUploadTargetConfig struct {
// caseID specifies the ...
//
// +kubebuilder:validation:MaxLength=128
// +kubebuilder:validation:MinLength=1
// +required
CaseID string `json:"caseId"`
// uploadTokenSecretRef ...
// +required
UploadTokenSecretRef corev1.LocalObjectReference `json:".."`
// sftpHost specifies ...
// +kubebuilder:default:="sftp.access.redhat.com"
// +optional
SFTPHost string `json:"sftpHost"`
}
also 👀 @shivprakashmuley as this also might interfere with #1831.
There was a problem hiding this comment.
are we updating the spec here or is it rolled into #1830 ?
There was a problem hiding this comment.
I will be closing this EP. Changes will be rolled into #1830
4 participants
This PR includes proposal to introduce a new API field in must-gather-operator spec for disabling upload of must-gather bundle.
Changes include:
disableUpload: Field is added to MustGather CRD allowing users to collect must-gather data without uploading to Red Hat's SFTP server.caseIDandcaseManagementAccountSecretRefrequired only when upload enabled.