-
Notifications
You must be signed in to change notification settings - Fork 3
feat(module): validate dvcr section in ModuleConfig #1628
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
Conversation
Signed-off-by: Valeriy Khorunzhin <[email protected]>
Reviewer's GuideThis PR adds validation for the dvcr section within ModuleConfig by introducing a new dvcrValidator that is integrated into the existing webhook pipeline to enforce immutability of storageClassName and prevent size reduction when the associated PVC already exists. Sequence diagram for dvcrValidator validation on ModuleConfig updatesequenceDiagram
participant "ModuleConfig Webhook"
participant "dvcrValidator"
participant "Kubernetes API (PVC)"
"ModuleConfig Webhook"->>"dvcrValidator": ValidateUpdate(ctx, oldMC, newMC)
"dvcrValidator"->>"Kubernetes API (PVC)": checkPVCExists(ctx)
"Kubernetes API (PVC)"-->>"dvcrValidator": PVC exists?
alt PVC exists
"dvcrValidator"->>"dvcrValidator": Compare storageClassName and size
alt storageClassName changed
"dvcrValidator"-->>"ModuleConfig Webhook": error: changing storageClassName forbidden
else size reduced
"dvcrValidator"-->>"ModuleConfig Webhook": error: reducing size forbidden
else valid
"dvcrValidator"-->>"ModuleConfig Webhook": success
end
else PVC does not exist
"dvcrValidator"-->>"ModuleConfig Webhook": success
end
Class diagram for dvcrValidator and related typesclassDiagram
class dvcrValidator {
+client: Client
+ValidateUpdate(ctx, oldMC, newMC): (Warnings, error)
+checkPVCExists(ctx): (bool, error)
}
class dvcrSettings {
+StorageType: string
+StorageClassName: string
+Size: string
}
class ModuleConfig {
+Spec: Spec
}
class Spec {
+Settings: SettingsValues
}
class SettingsValues
dvcrValidator --> dvcrSettings
dvcrValidator --> ModuleConfig
ModuleConfig --> Spec
Spec --> SettingsValues
dvcrValidator ..> Client : uses
dvcrValidator ..> PersistentVolumeClaim : checks existence
dvcrSettings <.. parseDvcrSettings : returned by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Workflow has started. The target step completed with status: failure. |
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.
Hey there - I've reviewed your changes - here's some feedback:
- dvcrValidator only runs on updates, so invalid dvcr settings in the initial ModuleConfig won’t be caught; consider adding a ValidateCreate implementation to enforce dvcr rules on creation as well.
- parseDvcrSettings returns an error whenever the dvcr section or nested fields are absent, which may block configs that simply omit dvcr; skip validation instead of erroring when dvcr isn’t present.
- The PVC name and namespace are hardcoded in dvcrValidator; consider making these configurable or reusing shared constants to improve flexibility and avoid future copy-paste updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- dvcrValidator only runs on updates, so invalid dvcr settings in the initial ModuleConfig won’t be caught; consider adding a ValidateCreate implementation to enforce dvcr rules on creation as well.
- parseDvcrSettings returns an error whenever the dvcr section or nested fields are absent, which may block configs that simply omit dvcr; skip validation instead of erroring when dvcr isn’t present.
- The PVC name and namespace are hardcoded in dvcrValidator; consider making these configurable or reusing shared constants to improve flexibility and avoid future copy-paste updates.
## Individual Comments
### Comment 1
<location> `images/virtualization-artifact/pkg/controller/moduleconfig/dvcr_validator.go:114-124` </location>
<code_context>
+ return nil, nil
+}
+
+func (v dvcrValidator) checkPVCExists(ctx context.Context) (bool, error) {
+ pvc, err := object.FetchObject(ctx, types.NamespacedName{
+ Name: dvcrPVCName,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error handling for PVC fetch may block updates unnecessarily.
Consider implementing a retry or fallback for transient errors, and handle specific cases like NotFound separately to avoid blocking updates unnecessarily.
```suggestion
import (
"time"
"k8s.io/apimachinery/pkg/api/errors"
)
func (v dvcrValidator) checkPVCExists(ctx context.Context) (bool, error) {
const maxRetries = 3
const retryDelay = 200 * time.Millisecond
var pvc interface{}
var err error
for attempt := 0; attempt < maxRetries; attempt++ {
pvc, err = object.FetchObject(ctx, types.NamespacedName{
Name: dvcrPVCName,
Namespace: dvcrNamespace,
}, v.client, &corev1.PersistentVolumeClaim{})
if err == nil {
return pvc != nil, nil
}
if errors.IsNotFound(err) {
return false, nil
}
// For transient errors, retry
if errors.IsTimeout(err) || errors.IsServerTimeout(err) || errors.IsTooManyRequests(err) {
time.Sleep(retryDelay)
continue
}
// For other errors, return immediately
return false, err
}
// If we exhausted retries, return last error
return false, err
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
images/virtualization-artifact/pkg/controller/moduleconfig/dvcr_validator.go
Show resolved
Hide resolved
images/virtualization-artifact/pkg/controller/moduleconfig/dvcr_validator.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Valeriy Khorunzhin <[email protected]>
Description
Validate dvcr section in ModuleConfig.
Checklist
Changelog entries