Skip to content

Flpath-4803 remove k8s refs - #39

Open
LinskId wants to merge 3 commits into
dcm-project:mainfrom
LinskId:FLPATH-4803_remove_k8s_refs
Open

Flpath-4803 remove k8s refs#39
LinskId wants to merge 3 commits into
dcm-project:mainfrom
LinskId:FLPATH-4803_remove_k8s_refs

Conversation

@LinskId

@LinskId LinskId commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Decouple the embedded storage public API from Kubernetes-specific types. Kubernetes hint parsing moves into internal/openshift/storage/kubernetes/. Runtime behavior is unchanged — embedded storage still creates/deletes PVCs on the cluster the same way. I was asked to do so in #31.

Added comments in README that list/get goes through control-plane SPRM, not agent /volumes

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Decouple storage API from Kubernetes-specific provider hints

✨ Enhancement 📝 Documentation 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Generalize storage API models and terminology by removing Kubernetes-specific contract details.
• Decode Kubernetes provider hints within the Kubernetes storage implementation.
• Document embedded provider routing and control-plane instance retrieval.
Diagram

graph TD
  Client["Storage Client"] --> API["Generic Storage API"] --> Models["Provider Hints Map"] --> Decoder["Kubernetes Decoder"] --> Converter["PVC Converter"] --> K8s["Kubernetes API"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Discriminated provider schemas
  • ➕ Provides OpenAPI validation and documentation for each provider's hints.
  • ➕ Avoids runtime map-to-struct conversion.
  • ➖ Couples the portable storage API to every supported provider.
  • ➖ Requires shared API regeneration whenever a provider adds configuration.
2. Raw JSON hint payloads
  • ➕ Preserves provider data without interface normalization.
  • ➕ Can defer all decoding to provider implementations.
  • ➖ Produces less ergonomic generated client models.
  • ➖ Requires custom schema or generation handling and explicit JSON decoding.

Recommendation: Keep the provider-neutral map in the shared API and decode Kubernetes hints inside the Kubernetes package. This best enforces the intended ownership boundary while retaining generated-client compatibility; provider-specific schemas would recreate the coupling this PR removes.

Files changed (9) +171 / -287

Enhancement (1) +49 / -0
hints.goAdd Kubernetes-owned provider hint translation +49/-0

Add Kubernetes-owned provider hint translation

• Introduces an internal Kubernetes hint structure plus helpers for decoding generic API hints and encoding PVC-derived values. Invalid hint payloads are translated into storage invalid-argument errors.

internal/openshift/storage/kubernetes/hints.go

Refactor (3) +52 / -220
openapi.yamlGeneralize the storage API contract +16/-51

Generalize the storage API contract

• Removes Kubernetes and PVC terminology, Kubernetes-specific hint schemas, and Kubernetes-prefixed resource identifiers. Provider hints remain an extensible object so individual implementations can own their configuration.

api/storage/v1alpha1/openapi.yaml

types.gen.goRegenerate provider-neutral storage models +9/-147

Regenerate provider-neutral storage models

• Replaces typed Kubernetes provider hints with a generic map and removes Kubernetes-specific enums and custom JSON handling. Generated comments now use provider-neutral storage terminology and resource identifiers.

api/storage/v1alpha1/types.gen.go

convert.goConsume provider-neutral hints during PVC conversion +27/-22

Consume provider-neutral hints during PVC conversion

• Routes storage class, access mode, and volume mode handling through the Kubernetes-local hint decoder. PVC-derived hints are now emitted through the generic API map while retaining validation and defaults.

internal/openshift/storage/kubernetes/convert.go

Tests (1) +17 / -21
store_crud_test.goAdapt storage CRUD tests to generic provider hints +17/-21

Adapt storage CRUD tests to generic provider hints

• Adds a helper for constructing map-based Kubernetes hints and updates storage class and volume mode scenarios to use the provider-neutral API representation.

internal/openshift/storage/kubernetes/store_crud_test.go

Documentation (3) +13 / -2
README.mdDocument embedded provider routing and instance retrieval +11/-0

Document embedded provider routing and instance retrieval

• Clarifies that embedded service providers do not expose service-type REST endpoints through the agent. Documents create/delete forwarding, control-plane list/get routes, and CloudEvent-driven status updates.

README.md

health.goGeneralize storage health terminology +1/-1

Generalize storage health terminology

• Updates the health checker comment to describe backing platform connectivity rather than Kubernetes connectivity.

internal/embedded/storage/health.go

setup.goGeneralize the embedded storage package description +1/-1

Generalize the embedded storage package description

• Removes the Kubernetes qualifier from the embedded storage provider package comment.

internal/embedded/storage/setup.go

Other (1) +40 / -44
spec.gen.goRegenerate the embedded storage OpenAPI specification +40/-44

Regenerate the embedded storage OpenAPI specification

• Updates the generated compressed specification to match the provider-neutral storage OpenAPI contract.

api/storage/v1alpha1/spec.gen.go

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📜 Skill insights (0)

Grey Divider


Action required

1. Misspelled storage hints are ignored 🐞 Bug ≡ Correctness
Description
k8sHintsFromSpec decodes the generic kubernetes object with json.Unmarshal, which accepts
fields outside the three declared fields and treats an object containing only unknown names as
empty. When a client misspells storage_class, volume_mode, or access_mode, volume creation
proceeds with defaults instead of rejecting the request, potentially creating a volume with
unintended storage semantics.
Code

internal/openshift/storage/kubernetes/hints.go[R33-35]

+	var hints k8sProviderHints
+	if err := json.Unmarshal(data, &hints); err != nil {
+		return nil, &store.InvalidArgumentError{Message: fmt.Sprintf("invalid kubernetes provider hints: %v", err)}
Evidence
The generic API schema permits arbitrary provider-hint properties, while the local Kubernetes struct
declares only three supported fields. The decoder uses ordinary json.Unmarshal and then returns
nil when none of those supported fields were populated, so an object containing only a misspelled
field reaches the default-setting paths without an error.

api/storage/v1alpha1/openapi.yaml[291-315]
internal/openshift/storage/kubernetes/hints.go[11-18]
internal/openshift/storage/kubernetes/hints.go[21-40]
internal/openshift/storage/kubernetes/convert.go[163-186]
internal/openshift/storage/kubernetes/convert.go[189-194]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new generic provider-hints decoder silently ignores unknown fields inside the `kubernetes` object. Reject unknown fields so misspelled storage settings cannot result in a volume being created with defaults.

## Issue Context
The public schema now permits generic provider hints, so Kubernetes-specific validation must happen in the Kubernetes implementation. Preserve support for other top-level provider namespaces while strictly validating fields within the `kubernetes` namespace, and add coverage for misspelled keys.

## Fix Focus Areas
- internal/openshift/storage/kubernetes/hints.go[21-40]
- internal/openshift/storage/kubernetes/store_crud_test.go[160-179]
- api/storage/v1alpha1/openapi.yaml[291-315]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Provider hint failures lack context 📘 Rule violation ≡ Correctness
Description
buildPVC and resolveAccessMode return errors from k8sHintsFromSpec and resolveVolumeMode
directly instead of wrapping them with operation-specific context. When hint decoding or mode
validation fails, the error reaches storage creation without identifying which provider-hint
processing stage was underway.
Code

internal/openshift/storage/kubernetes/convert.go[R139-140]

+	if err != nil {
+		return nil, err
Evidence
Compliance rule 2788501 requires returned errors to be wrapped with contextual `fmt.Errorf(... %w
...). The changed branches return err` unchanged after provider-hint decoding and volume-mode
resolution, and resolveAccessMode introduces another unchanged return.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/openshift/storage/kubernetes/convert.go[138-145]
internal/openshift/storage/kubernetes/convert.go[163-166]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New provider-hint processing paths return bare errors rather than wrapping them with `fmt.Errorf` and `%w`.

## Issue Context
Preserve each underlying error while adding context that distinguishes hint decoding, access-mode resolution, and volume-mode resolution.

## Fix Focus Areas
- internal/openshift/storage/kubernetes/convert.go[138-145]
- internal/openshift/storage/kubernetes/convert.go[163-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Hint decoding errors lose their cause 📘 Rule violation ≡ Correctness
Description
k8sHintsFromSpec interpolates the json.Unmarshal error into InvalidArgumentError.Message with
%v rather than retaining it through %w. When malformed Kubernetes hints reach this branch,
callers can only inspect flattened text and cannot use errors.Is or errors.As on the underlying
decoding error.
Code

internal/openshift/storage/kubernetes/hints.go[R34-35]

+	if err := json.Unmarshal(data, &hints); err != nil {
+		return nil, &store.InvalidArgumentError{Message: fmt.Sprintf("invalid kubernetes provider hints: %v", err)}
Evidence
Compliance rule 2788501 explicitly prohibits using %v when returning an underlying error and
requires %w wrapping. The new malformed-hints branch formats the decoder error with %v into a
string-only field.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/openshift/storage/kubernetes/hints.go[29-35]
internal/openshift/storage/store/errors.go[23-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The JSON decoding error is flattened into a message with `%v`, which discards its error chain.

## Issue Context
Retain the existing invalid-argument classification while preserving the decoding cause through `%w` or typed-error `Unwrap` support.

## Fix Focus Areas
- internal/openshift/storage/kubernetes/hints.go[34-35]
- internal/openshift/storage/store/errors.go[23-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Malformed hints report the wrong failure 🐞 Bug ≡ Correctness
Description
resolveStorageClass suppresses errors from k8sHintsFromSpec and returns the configured default
storage class instead. If a malformed hint is submitted while that default class does not exist,
Create returns a missing-class precondition failure before the same hint can be reported as an
invalid argument during volume construction.
Code

internal/openshift/storage/kubernetes/convert.go[R190-192]

+	hints, err := k8sHintsFromSpec(spec)
+	if err != nil || hints == nil || hints.StorageClass == nil {
+		return defaultClass
Evidence
The parser explicitly wraps decoding failures as InvalidArgumentError, but resolveStorageClass
converts every parser error into use of the default class. Create validates that fallback before
calling buildPVC, and missing-class validation produces FailedPreconditionError, proving that
this earlier branch can mask the malformed request.

internal/openshift/storage/kubernetes/hints.go[29-40]
internal/openshift/storage/kubernetes/convert.go[189-194]
internal/openshift/storage/kubernetes/store_create.go[22-31]
internal/openshift/storage/kubernetes/store_create.go[45-53]
internal/openshift/storage/kubernetes/convert.go[106-115]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Storage-class resolution discards malformed provider-hint errors, allowing validation of a fallback class to mask the actual invalid request. Parse the Kubernetes hints once or return the parsing error from storage-class resolution before validating any class.

## Issue Context
`Create` resolves and validates the storage class before `buildPVC`, while the latter reparses the same hints. Ensure malformed hints consistently produce `InvalidArgumentError`, and add a test combining malformed hints with a missing configured default class.

## Fix Focus Areas
- internal/openshift/storage/kubernetes/convert.go[189-194]
- internal/openshift/storage/kubernetes/store_create.go[22-31]
- internal/openshift/storage/kubernetes/hints.go[21-40]
- internal/openshift/storage/kubernetes/store_crud_test.go[241-272]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 17 rules
Review mode: 🧠 Deep: This removes Kubernetes-specific API types while introducing new provider-hint parsing and conversion logic across schemas, generated models, and CRUD paths, creating many independent compatibility and behavioral risks.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/openshift/storage/kubernetes/convert.go Outdated
Comment thread internal/openshift/storage/kubernetes/hints.go Outdated
Comment thread internal/openshift/storage/kubernetes/hints.go Outdated
Comment thread internal/openshift/storage/kubernetes/convert.go Outdated
@gabriel-farache

Copy link
Copy Markdown
Contributor

Not sure I understand what was done exactly: there are changes in name/comment but also refactor to extract method into a new file and some new lines in the README
Could you detail a bit more what is done in the PR's description?

Assisted-by: Cursor
Signed-off-by: igavra <igavra@redhat.com>
@LinskId

LinskId commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@jenniferubah @gabriel-farache better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants