Add spec validation for BackingStore and NamespaceStore #1745
Conversation
WalkthroughAdds exported pre-validation functions that ensure a BackingStore or NamespaceStore declared Type has a corresponding non-nil sub-spec and return a ValidationError if missing; these checks run before existing secret/target-bucket and type-specific validations (ValidateBSInValidSpec, ValidateNSInValidSpec). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
VershaAgrawal
force-pushed
the
add-spec-validation
branch
from
35797d0 to
17f597c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/validations/namespacestore_validations.go (1)
22-48: Early sub-spec presence checks look correct; be aware of stricter behaviorThe new switch that requires the corresponding
Spec.<Type>field to be non-nil for eachNamespaceStore.Spec.Typeis consistent with how the rest of this file already dereferences those fields (e.g., inValidateNSEmptySecretName,ValidateNSEmptyTargetBucket,ValidateNsStoreNSFS) and will prevent nil-pointer panics on malformed specs.One semantic change to note: for
NSStoreTypeNSFS, aNamespaceStorewithspec.type = "nsfs"but a missingspec.nsfsblock used to pass validation (sinceValidateNsStoreNSFSreturnsnilwhennsfs == nil); with this pre-check it will now be rejected. The same applies to other types where the sub-spec was previously omitted. That aligns with the PR goal (“reject invalid spec”), but it’s worth double-checking that:
- No existing clusters rely on such partially-specified
NamespaceStoreobjects, and- Admission is called consistently on both create and update so this stronger validation doesn’t surprise upgrades.
If possible, I’d also recommend adding unit tests that cover each type with a nil sub-spec, asserting that
ValidateNamespaceStorereturns the newValidationErrormessages.pkg/validations/backingstore_validations.go (1)
21-47: Good guard against nil sub-specs; verify impact on existing BackingStoresThe added pre-validation on
bs.Spec.Typeto ensure the matchingbs.Spec.<Type>is non-nil is aligned with how the rest ofValidateBackingStoreand helpers use these fields (e.g.,ValidateBSEmptySecretName,ValidateBSEmptyTargetBucket, PVPool volume validations). This should eliminate potential nil-pointer panics for malformed CRs and makes the admission behavior clearer.This does tighten validation semantics: any
BackingStorewithspec.typeset to one of these values but missing the corresponding sub-spec will now be rejected up front instead of reaching later logic. That’s probably what you want, but I’d suggest:
- Double-checking there are no legacy
BackingStoreobjects in the wild that rely on a missing sub-spec being tolerated.- Adding unit tests that, for each store type, construct a
BackingStorewith a nil sub-spec and assert thatValidateBackingStorereturns the newValidationError.Functionally the change looks solid and consistent with the namespace-store validations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/validations/backingstore_validations.go(1 hunks)pkg/validations/namespacestore_validations.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T15:28:44.080Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
Applied to files:
pkg/validations/backingstore_validations.gopkg/validations/namespacestore_validations.go
🧬 Code graph analysis (1)
pkg/validations/backingstore_validations.go (2)
pkg/apis/noobaa/v1alpha1/backingstore_types.go (6)
StoreTypeAWSS3(125-125)StoreTypeS3Compatible(128-128)StoreTypeIBMCos(131-131)StoreTypeAzureBlob(137-137)StoreTypeGoogleCloudStorage(134-134)StoreTypePVPool(140-140)pkg/util/util.go (1)
ValidationError(105-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-admission-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-azure-vault-test
- GitHub Check: golangci-lint
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-hac-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-tls-token-test
VershaAgrawal
changed the title
|
@VershaAgrawal,
|
VershaAgrawal
force-pushed
the
add-spec-validation
branch
from
17f597c to
410d1af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/validations/namespacestore_validations.go (1)
62-102: Consider making error messages more specific and reducing code duplication.The current implementation uses a generic error message for all types. Making it type-specific would improve the user experience when debugging invalid configurations. For example, "AWSS3 spec must be provided" is clearer than "Namespacestore spec must be provided".
Additionally, this function follows the same pattern as
ValidateBSInValidSpecin backingstore_validations.go. As per shirady's feedback in the PR comments, consider factoring shared logic into a common function to reduce duplication.Example of more specific error messages:
func ValidateNSInValidSpec(nsStore nbv1.NamespaceStore) error { - err := util.ValidationError{Msg: "Namespacestore spec must be provided"} switch nsStore.Spec.Type { case nbv1.NSStoreTypeNSFS: if nsStore.Spec.NSFS == nil { - return err + return util.ValidationError{Msg: "NSFS spec must be provided"} } case nbv1.NSStoreTypeAWSS3: if nsStore.Spec.AWSS3 == nil { - return err + return util.ValidationError{Msg: "AWSS3 spec must be provided"} }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/admission/test/unit/admission_unit_test.go(2 hunks)pkg/validations/backingstore_validations.go(2 hunks)pkg/validations/namespacestore_validations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/validations/backingstore_validations.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/admission/test/unit/admission_unit_test.go (4)
pkg/apis/noobaa/v1alpha1/backingstore_types.go (2)
StoreTypeAWSS3(125-125)AWSS3Spec(144-165)pkg/validations/backingstore_validations.go (1)
ValidateBSInValidSpec(62-97)pkg/apis/noobaa/v1alpha1/namespacestore_types.go (2)
NamespaceStoreSpec(58-90)NSStoreTypeAWSS3(156-156)pkg/validations/namespacestore_validations.go (1)
ValidateNSInValidSpec(62-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-operator-tests
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-sa-test
🔇 Additional comments (3)
pkg/validations/namespacestore_validations.go (1)
22-27: LGTM!The early validation correctly ensures that the declared type has a corresponding non-nil sub-spec before proceeding with downstream validations. This prevents potential nil pointer dereferences in subsequent validation functions.
pkg/admission/test/unit/admission_unit_test.go (2)
36-59: LGTM!The test properly validates both the deny scenario (missing AWSS3 sub-spec for declared type) and the allow scenario (valid AWSS3 sub-spec provided). The error message assertions ensure the validation function returns expected errors.
412-435: LGTM!The test correctly validates both scenarios: denying when the AWSS3 sub-spec is nil despite the declared type, and allowing when the proper sub-spec is provided. The test structure mirrors the BackingStore tests, maintaining consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/validations/namespacestore_validations.go (1)
61-102:ValidateNSInValidSpeclogic is correct; consider minor tidy‑upsThe function correctly covers all existing namespace store types, returns a clear
"Namespacestore spec must be provided"when the declared type’s sub‑spec is nil, and reuses the existing"Invalid Namespacestore type..."error for unknown types. This matches the admission behavior described in the PR and should prevent panics on malformed specs.Two minor, non‑blocking suggestions:
- To avoid copying potentially large CRDs, you could accept a pointer for consistency with
ValidateNamespaceStore:-func ValidateNSInValidSpec(nsStore nbv1.NamespaceStore) error { +func ValidateNSInValidSpec(nsStore *nbv1.NamespaceStore) error { @@ - switch nsStore.Spec.Type { + switch nsStore.Spec.Type { @@ - if nsStore.Spec.NSFS == nil { + if nsStore.Spec.NSFS == nil { return err }
- The
errvalue is reused identically in all nil‑subspec branches; optionally you could declare it as avarat package scope or inline the struct literals to keep intent obvious—current form is fine if you prefer the single shared instance.Overall, behavior and messages look good.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/validations/namespacestore_validations.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-hac-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (1)
pkg/validations/namespacestore_validations.go (1)
21-27: Pre‑validation hook inValidateNamespaceStorecorrectly guards downstream checksAdding
ValidateNSInValidSpec(*nsStore)beforeValidateNSEmptySecretName/ValidateNSEmptyTargetBucketis a good safety net: it guarantees the type‑specific sub‑spec is non‑nil for the declaredSpec.Type, which prevents potential nil dereferences in existing validators and aligns behavior with the BackingStore validation flow. No functional issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/validations/backingstore_validations.go (2)
61-97: Consider more specific error messages per type.The generic error "Backingstore spec must be provided" doesn't indicate which sub-spec is missing. More specific messages would help users debug faster.
func ValidateBSInValidSpec(bs nbv1.BackingStore) error { - - err := util.ValidationError{Msg: "Backingstore spec must be provided"} - switch bs.Spec.Type { case nbv1.StoreTypeAWSS3: if bs.Spec.AWSS3 == nil { - return err + return util.ValidationError{Msg: "AWSS3 spec must be provided for aws-s3 type BackingStore"} } case nbv1.StoreTypeS3Compatible: if bs.Spec.S3Compatible == nil { - return err + return util.ValidationError{Msg: "S3Compatible spec must be provided for s3-compatible type BackingStore"} } case nbv1.StoreTypeIBMCos: if bs.Spec.IBMCos == nil { - return err + return util.ValidationError{Msg: "IBMCos spec must be provided for ibm-cos type BackingStore"} } case nbv1.StoreTypeAzureBlob: if bs.Spec.AzureBlob == nil { - return err + return util.ValidationError{Msg: "AzureBlob spec must be provided for azure-blob type BackingStore"} } case nbv1.StoreTypeGoogleCloudStorage: if bs.Spec.GoogleCloudStorage == nil { - return err + return util.ValidationError{Msg: "GoogleCloudStorage spec must be provided for google-cloud-storage type BackingStore"} } case nbv1.StoreTypePVPool: if bs.Spec.PVPool == nil { - return err + return util.ValidationError{Msg: "PVPool spec must be provided for pv-pool type BackingStore"} } default: return util.ValidationError{ Msg: "Invalid Backingstore type, please provide a valid Backingstore type", } } return nil }This aligns with the PR reviewer's suggestion to provide type-specific messages.
53-57: Dead code: default case is now unreachable.Since
ValidateBSInValidSpecis called first and its default case handles invalid types, this default branch will never execute. Consider removing it or adding a comment explaining it's kept as a defensive measure.case nbv1.StoreTypeAWSS3, nbv1.StoreTypeAzureBlob, nbv1.StoreTypeGoogleCloudStorage: return nil - default: - return util.ValidationError{ - Msg: "Invalid Backingstore type, please provide a valid Backingstore type", - } } return nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/validations/backingstore_validations.go(2 hunks)pkg/validations/namespacestore_validations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/validations/namespacestore_validations.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T15:28:44.080Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
Applied to files:
pkg/validations/backingstore_validations.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-hac-test
🔇 Additional comments (1)
pkg/validations/backingstore_validations.go (1)
21-26: Good addition of pre-validation to prevent nil pointer dereferences.This early check ensures sub-specs are present before accessing their fields in
ValidateBSEmptySecretNameandValidateBSEmptyTargetBucket, preventing potential panics.
VershaAgrawal
force-pushed
the
add-spec-validation
branch
from
b5e0ac5 to
093aa8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/validations/backingstore_validations.go (1)
64-64: Consider type-specific error messages for better UX.The generic error message works but could be more helpful by indicating which sub-spec is missing. For example:
fmt.Sprintf("%s spec must be provided", bs.Spec.Type)would tell users exactly which field they forgot to populate.Apply this diff for more specific error messages:
- err := util.ValidationError{Msg: "Backingstore spec must be provided"} - switch bs.Spec.Type { case nbv1.StoreTypeAWSS3: if bs.Spec.AWSS3 == nil { - return err + return util.ValidationError{Msg: "AWSS3 spec must be provided for aws-s3 type backingstore"} } case nbv1.StoreTypeS3Compatible: if bs.Spec.S3Compatible == nil { - return err + return util.ValidationError{Msg: "S3Compatible spec must be provided for s3-compatible type backingstore"} } case nbv1.StoreTypeIBMCos: if bs.Spec.IBMCos == nil { - return err + return util.ValidationError{Msg: "IBMCos spec must be provided for ibm-cos type backingstore"} } case nbv1.StoreTypeAzureBlob: if bs.Spec.AzureBlob == nil { - return err + return util.ValidationError{Msg: "AzureBlob spec must be provided for azure-blob type backingstore"} } case nbv1.StoreTypeGoogleCloudStorage: if bs.Spec.GoogleCloudStorage == nil { - return err + return util.ValidationError{Msg: "GoogleCloudStorage spec must be provided for google-cloud-storage type backingstore"} } case nbv1.StoreTypePVPool: if bs.Spec.PVPool == nil { - return err + return util.ValidationError{Msg: "PVPool spec must be provided for pv-pool type backingstore"} }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/admission/test/unit/admission_unit_test.go(2 hunks)pkg/validations/backingstore_validations.go(2 hunks)pkg/validations/namespacestore_validations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/admission/test/unit/admission_unit_test.go
- pkg/validations/namespacestore_validations.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T15:28:44.080Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
Applied to files:
pkg/validations/backingstore_validations.go
🧬 Code graph analysis (1)
pkg/validations/backingstore_validations.go (2)
pkg/apis/noobaa/v1alpha1/backingstore_types.go (7)
BackingStore(24-39)StoreTypeAWSS3(125-125)StoreTypeS3Compatible(128-128)StoreTypeIBMCos(131-131)StoreTypeAzureBlob(137-137)StoreTypeGoogleCloudStorage(134-134)StoreTypePVPool(140-140)pkg/util/util.go (1)
ValidationError(105-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
🔇 Additional comments (2)
pkg/validations/backingstore_validations.go (2)
21-26: LGTM! Pre-validation prevents nil pointer panics.The early check for sub-spec presence is correctly placed before all downstream validations that access sub-spec fields (secret name, target bucket, signature version). This prevents potential nil pointer dereferences that would crash the admission controller.
61-97: Logic is correct and handles all store types.The function correctly validates that each declared type has its corresponding non-nil sub-spec, covering all six supported BackingStore types and properly handling unknown types in the default case.
VershaAgrawal
force-pushed
the
add-spec-validation
branch
from
093aa8f to
1b67782
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/validations/namespacestore_validations.go (2)
22-25: LGTM! Pre-validation ensures type-spec consistency.The pre-validation call correctly ensures the declared type has a matching non-nil sub-spec before other validations that dereference those fields.
Minor style note: Line 22 comment starts with lowercase "ensure" (Go convention is to capitalize the first word).
60-99: Function logic is correct; consider extracting shared validation pattern.The validation logic correctly ensures each declared type has its corresponding non-nil sub-spec. All six supported NamespaceStore types are properly covered.
Note: The past review comment about a typo in the function comment appears to have been resolved—the current comment correctly references "ValidateNSInValidSpec".
This function shares ~90% of its structure with
ValidateBSInValidSpecinbackingstore_validations.go. As shirady noted in PR comments, consider extracting the shared type-validation pattern into a common helper function to reduce duplication and improve maintainability. However, this can be deferred to a follow-up refactoring.pkg/validations/backingstore_validations.go (1)
21-24: LGTM! Pre-validation correctly guards against nil sub-specs.The pre-validation call ensures the declared BackingStore type has a matching non-nil sub-spec before subsequent validations attempt to access those fields.
Minor style note: Line 21 comment should have a space after
//(Go convention:// Ensureinstead of//Ensure).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/admission/test/unit/admission_unit_test.go(2 hunks)pkg/validations/backingstore_validations.go(2 hunks)pkg/validations/namespacestore_validations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/admission/test/unit/admission_unit_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T15:28:44.080Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
Applied to files:
pkg/validations/backingstore_validations.go
🧬 Code graph analysis (2)
pkg/validations/namespacestore_validations.go (2)
pkg/apis/noobaa/v1alpha1/namespacestore_types.go (7)
NamespaceStore(24-39)NSStoreTypeNSFS(171-171)NSStoreTypeAWSS3(156-156)NSStoreTypeS3Compatible(159-159)NSStoreTypeIBMCos(162-162)NSStoreTypeAzureBlob(165-165)NSStoreTypeGoogleCloudStorage(168-168)pkg/util/util.go (1)
ValidationError(105-107)
pkg/validations/backingstore_validations.go (2)
pkg/apis/noobaa/v1alpha1/backingstore_types.go (7)
BackingStore(24-39)StoreTypeAWSS3(125-125)StoreTypeS3Compatible(128-128)StoreTypeIBMCos(131-131)StoreTypeAzureBlob(137-137)StoreTypeGoogleCloudStorage(134-134)StoreTypePVPool(140-140)pkg/util/util.go (1)
ValidationError(105-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
🔇 Additional comments (1)
pkg/validations/backingstore_validations.go (1)
60-93: Function logic is correct and handles all BackingStore types.The validation logic correctly ensures each declared BackingStore type has its corresponding non-nil sub-spec. All six supported types (including PVPool) are properly covered with clear error messages.
As noted in the review of
namespacestore_validations.go, this function shares the same validation pattern and could benefit from shared helper extraction, but this refactoring is optional and can be addressed separately.
Signed-off-by: VershaAgrawal <[email protected]>
VershaAgrawal
force-pushed
the
add-spec-validation
branch
from
1b67782 to
0f14f36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/validations/backingstore_validations.go (1)
60-93:ValidateBSInValidSpeclogic is correct; consider a small refactor to reduce duplication.The type→sub-spec presence checks and per-type messages are correct and match the CRD expectations. The same “invalid type” error string is now repeated here and in other validators, and a very similar pattern exists in
ValidateNSInValidSpecfor NamespaceStore.If you want to keep things DRY and future-proof for new store types, consider:
- Centralizing the “invalid Backingstore type” error construction in a tiny helper (e.g.,
newInvalidBackingStoreTypeError()), and- Optionally extracting a small internal helper that, given a type and the corresponding sub-spec pointer, performs the nil check and formats a message based on the type string.
Not urgent, but would make further type additions a bit less error-prone.
pkg/validations/namespacestore_validations.go (1)
60-99:ValidateNSInValidSpecmatches the intended stricter spec validation; DRY opportunity with BackingStore helper.The switch correctly enforces that each NamespaceStore type (NSFS, AWSS3, S3Compatible, IBMCos, AzureBlob, GoogleCloudStorage) has its corresponding non-nil spec, and it returns a clear error for unsupported types. This aligns well with the BackingStore pre-validation.
As with
ValidateBSInValidSpec, there’s some duplication in:
- The repeated invalid-type error string, and
- The “if spec is nil, return type-specific message” pattern, which is now duplicated between BackingStore and NamespaceStore.
If you’d like, you could:
- Factor the invalid-type error into a tiny shared helper for NamespaceStore, and
- Optionally introduce a small internal helper (or table-driven approach) shared between BS/NS to encapsulate the “required sub-spec for declared type” pattern, keeping only the type enums and human-readable type strings different.
Not blocking, but would simplify future maintenance if more store types are added.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/admission/test/unit/admission_unit_test.go(2 hunks)pkg/validations/backingstore_validations.go(2 hunks)pkg/validations/namespacestore_validations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/admission/test/unit/admission_unit_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T15:28:44.080Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
Applied to files:
pkg/validations/backingstore_validations.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: golangci-lint
🔇 Additional comments (2)
pkg/validations/backingstore_validations.go (1)
21-25: Pre-spec validation ordering inValidateBackingStorelooks good and improves safety.Calling
ValidateBSInValidSpecup front ensures a non-nil per-type spec before touching fields inValidateBSEmptySecretName/ValidateBSEmptyTargetBucketand the PVPool validators, which removes potential nil deref risks and makes the admission error messages clearer for malformed specs. The control flow and returned errors for valid objects remain unchanged.pkg/validations/namespacestore_validations.go (1)
22-26: Pre-validation hook inValidateNamespaceStoreis consistent and safe.Running
ValidateNSInValidSpecfirst ensures the declared type has its matching sub-spec before callingValidateNSEmptySecretName,ValidateNSEmptyTargetBucket, and the type-specific validators. That both tightens admission behavior for malformed specs and removes potential nil deref paths for type-backed fields.
2 participants
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
In the above example:
aws-s3type must haveAWSS3Spec(awsS3in the yaml)3. Save the yaml file using
kubectl apply -f <file_name>.yaml.The requested is rejected for this invalid spec with error message :
"Backingstore spec must be provided".Same can be performed for namespacestore as well
Alternatively use:
ginkgo -v pkg/admission/test/unitSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.