Extract .spec from cluster record format for snapshot input #3043

dheerajodha · 2025-11-25T15:35:00Z

User description

Modify readSnapshotSource to automatically extract the .spec field when the input is a Kubernetes cluster record, matching the behavior for policy.yaml files. This allows direct use of kubectl output without manual jq extraction.
The change maintains backward compatibility with direct SnapshotSpec input and works with both JSON and YAML formats.

resolves: EC-1534

PR Type

Enhancement

Description

Auto-extract .spec field from Kubernetes cluster records
Enables direct use of kubectl output without manual jq extraction
Maintains backward compatibility with direct SnapshotSpec input
Supports both JSON and YAML formats seamlessly

Diagram Walkthrough

flowchart LR
  A["Cluster Record<br/>with .spec wrapper"] -->|readSnapshotSource| B["Extract .spec field"]
  C["Direct SnapshotSpec<br/>no wrapper"] -->|readSnapshotSource| B
  B -->|Unmarshal| D["SnapshotSpec Object"]

File Walkthrough

Relevant files

Enhancement

input.go `Add .spec extraction logic to readSnapshotSource` internal/applicationsnapshot/input.go Added logic to detect and extract `.spec` field from cluster record format Parses input as generic map first to check for `.spec` key presence Re-marshals extracted spec back to bytes for SnapshotSpec unmarshaling Maintains backward compatibility by processing input directly if no `.spec` wrapper exists	+22/-0

Tests

input_test.go `Add comprehensive tests for .spec extraction` internal/applicationsnapshot/input_test.go Added test case for cluster record format with `.spec` wrapper in JSON Added test case for cluster record format with `.spec` wrapper in YAML Added test case verifying backward compatibility with direct SnapshotSpec input All tests validate correct parsing and SnapshotSpec object equality	+92/-0

codecov · 2025-11-25T15:42:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
acceptance	`55.53% <75.00%> (-0.17%)`	⬇️
generative	`19.00% <0.00%> (-0.06%)`	⬇️
integration	`27.88% <0.00%> (-0.09%)`	⬇️
unit	`67.58% <100.00%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/applicationsnapshot/input.go	`92.98% <100.00%> (+0.16%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

qodo-code-review · 2025-11-26T14:15:17Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Weak error context: Errors like "spec is not a valid map structure" lack input context or path details, and Debugf logs embed raw input bytes which is not robust and may hinder actionable debugging. Referred Code if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) } // Extract the "spec" key from YAML, if present, to use as the snapshot. if spec, ok := v["spec"]; ok { v, ok = spec.(map[string]interface{}) if !ok { return app.SnapshotSpec{}, fmt.Errorf("spec is not a valid map structure") } // Marshal the spec back to bytes for unmarshaling into SnapshotSpec specBytes, err := yaml.Marshal(v) if err != nil { return app.SnapshotSpec{}, fmt.Errorf("unable to marshal spec: %w", err) } input = specBytes Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Input leak in logs: Debug logging includes the full `input` byte content in error paths, potentially exposing sensitive data from cluster records. Referred Code if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Sensitive data logging: The debug logs output raw `input` content, which may contain sensitive metadata or images from cluster records, violating secure logging practices. Referred Code if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new parsing path that extracts the `.spec` field performs critical input handling without emitting any audit-relevant logs of the action or outcome. Referred Code var v map[string]interface{} // Since JSON is a subset of YAML, yaml.Unmarshal can be used directly. if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) } // Extract the "spec" key from YAML, if present, to use as the snapshot. if spec, ok := v["spec"]; ok { v, ok = spec.(map[string]interface{}) if !ok { return app.SnapshotSpec{}, fmt.Errorf("spec is not a valid map structure") } // Marshal the spec back to bytes for unmarshaling into SnapshotSpec specBytes, err := yaml.Marshal(v) if err != nil { return app.SnapshotSpec{}, fmt.Errorf("unable to marshal spec: %w", err) } input = specBytes } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Generic variable name: The variable `v` is used as a generic map container which reduces readability compared to a more descriptive name like `root` or `docMap`. Referred Code var v map[string]interface{} // Since JSON is a subset of YAML, yaml.Unmarshal can be used directly. if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) } // Extract the "spec" key from YAML, if present, to use as the snapshot. if spec, ok := v["spec"]; ok { v, ok = spec.(map[string]interface{}) if !ok { return app.SnapshotSpec{}, fmt.Errorf("spec is not a valid map structure") } // Marshal the spec back to bytes for unmarshaling into SnapshotSpec specBytes, err := yaml.Marshal(v) if err != nil { return app.SnapshotSpec{}, fmt.Errorf("unable to marshal spec: %w", err) } input = specBytes } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Insufficient validation: Parsing trusts that `.spec` is a map without validating allowed fields or size limits, and does not sanitize or constrain untrusted input prior to unmarshaling. Referred Code // Extract the "spec" key from YAML, if present, to use as the snapshot. if spec, ok := v["spec"]; ok { v, ok = spec.(map[string]interface{}) if !ok { return app.SnapshotSpec{}, fmt.Errorf("spec is not a valid map structure") } // Marshal the spec back to bytes for unmarshaling into SnapshotSpec specBytes, err := yaml.Marshal(v) if err != nil { return app.SnapshotSpec{}, fmt.Errorf("unable to marshal spec: %w", err) } input = specBytes } var file app.SnapshotSpec err := yaml.Unmarshal(input, &file) if err != nil { Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-11-26T14:16:15Z

PR Code Suggestions ✨

Latest suggestions up to 44cbce8

Category	Suggestion	Impact
Possible issue	Fix mock error index access In the `mockTestRunner.Run` function, change `args.Error(2)` to `args.Error(1)` to prevent a runtime panic caused by accessing an out-of-bounds return value index. internal/applicationsnapshot/input_test.go [63-66] func (m *mockTestRunner) Run(ctx context.Context, inputs []string) ([]Outcome, error) { args := m.Called(ctx, inputs) - return args.Get(0).([]Outcome), args.Error(2) + return args.Get(0).([]Outcome), args.Error(1) } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical bug in the mock implementation where `args.Error(2)` would cause a panic because the function only has two return values (indexed 0 and 1). Fixing this to `args.Error(1)` is essential for the tests to run correctly.	High
Incremental ^[*]	~~Remove unsafe input logging~~ Remove the logging of raw `input` bytes from debug messages and error formatting to avoid leaking potentially sensitive data and to improve log readability. internal/applicationsnapshot/input.go [186-206] // Define a temporary struct to capture the wrapped spec var wrapper struct { Spec app.SnapshotSpec `yaml:"spec"` } // Attempt to unmarshal into the wrapper to check for cluster record format if err := yaml.Unmarshal(input, &wrapper); err == nil && wrapper.Spec != nil { // If successful and spec exists, return it directly - log.Debugf("Read application snapshot from cluster record format") + log.Debug("Read application snapshot from cluster record format") return wrapper.Spec, nil } // Fallback: unmarshal directly into SnapshotSpec for backward compatibility var spec app.SnapshotSpec if err := yaml.Unmarshal(input, &spec); err != nil { - log.Debugf("Problem parsing application snapshot from file %s", input) - return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) + log.Debug("Problem parsing application snapshot from provided bytes") + return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification: %w", err) } -log.Debugf("Read application snapshot from file %s", input) +log.Debug("Read application snapshot from provided bytes") return spec, nil `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that logging the raw `input` byte slice can expose sensitive information and clutter logs, which is a valid security and operational concern.	Low
General	Add json tag for wrapper Add a `json:"spec"` tag to the `wrapper` struct's `Spec` field to ensure correct unmarshaling from both YAML and JSON inputs. internal/applicationsnapshot/input.go [186-196] // Define a temporary struct to capture the wrapped spec var wrapper struct { - Spec app.SnapshotSpec `yaml:"spec"` + Spec app.SnapshotSpec `yaml:"spec" json:"spec"` } // Attempt to unmarshal into the wrapper to check for cluster record format if err := yaml.Unmarshal(input, &wrapper); err == nil && wrapper.Spec != nil { // If successful and spec exists, return it directly log.Debugf("Read application snapshot from cluster record format") return wrapper.Spec, nil } Apply / Chat* Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that adding a `json:"spec"` tag improves robustness and clarity for handling both JSON and YAML inputs. While `sigs.k8s.io/yaml` often handles this, being explicit is better practice and prevents potential edge cases.	Low
More

Previous suggestions

✅ Suggestions up to commit ae837ea

Category	Suggestion	Impact
Possible issue	✅ ~~Correct YAML indentation in test~~ Suggestion Impact: The YAML within the clusterRecord string was re-indented to valid YAML as suggested. code diff: clusterRecord := `apiVersion: appstudio.redhat.com/v1alpha1 - kind: Snapshot - metadata: - name: vsa-demo-app-x9xln - namespace: user-ns2 - spec: - components: - - name: Named - containerImage: registry.io/repository/image:tag - ` +kind: Snapshot +metadata: + name: vsa-demo-app-x9xln + namespace: user-ns2 +spec: + components: + - name: Named + containerImage: registry.io/repository/image:tag` Correct the indentation in the YAML string literal within the test case `TestReadSnapshotFile` to ensure it is valid YAML. internal/applicationsnapshot/input_test.go [287-296] clusterRecord := `apiVersion: appstudio.redhat.com/v1alpha1 - kind: Snapshot - metadata: - name: vsa-demo-app-x9xln - namespace: user-ns2 - spec: - components: - - name: Named - containerImage: registry.io/repository/image:tag - ` +kind: Snapshot +metadata: + name: vsa-demo-app-x9xln + namespace: user-ns2 +spec: + components: + - name: Named + containerImage: registry.io/repository/image:tag +` `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the YAML in the test case is malformed due to incorrect indentation, and fixing it makes the test more robust and reliable.	Medium
General	Prevent variable shadowing on spec extraction Instead of reassigning the `v` variable, use a new variable such as `specMap` to hold the extracted `spec` content. This improves clarity and prevents potential bugs from variable shadowing. internal/applicationsnapshot/input.go [195-206] if spec, ok := v["spec"]; ok { - v, ok = spec.(map[string]interface{}) + specMap, ok := spec.(map[string]interface{}) if !ok { return app.SnapshotSpec{}, fmt.Errorf("spec is not a valid map structure") } // Marshal the spec back to bytes for unmarshaling into SnapshotSpec - specBytes, err := yaml.Marshal(v) + specBytes, err := yaml.Marshal(specMap) if err != nil { return app.SnapshotSpec{}, fmt.Errorf("unable to marshal spec: %w", err) } input = specBytes } Suggestion importance[1-10]: 4 __ Why: The suggestion improves code readability and maintainability by avoiding variable reassignment, which is a good practice, but it does not fix a bug in the current implementation.	Low

✅ Suggestions up to commit 94f6f9c

Category	Suggestion	Impact
High-level	Simplify parsing by avoiding re-serialization Refactor the parsing logic to eliminate the inefficient process of unmarshaling, re-marshaling, and then unmarshaling again. Instead, use a temporary struct to directly extract the `.spec` field if it exists. Examples: internal/applicationsnapshot/input.go [185-217] func readSnapshotSource(input []byte) (app.SnapshotSpec, error) { var v map[string]interface{} // Since JSON is a subset of YAML, yaml.Unmarshal can be used directly. if err := yaml.Unmarshal(input, &v); err != nil { log.Debugf("Problem parsing application snapshot from file %s", input) return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) } // Extract the "spec" key from YAML, if present, to use as the snapshot. ... (clipped 23 lines) Solution Walkthrough: Before: func readSnapshotSource(input []byte) (SnapshotSpec, error) { var v map[string]interface{} // Unmarshal 1: to a generic map yaml.Unmarshal(input, &v) if spec, ok := v["spec"]; ok { // Marshal: spec back to bytes specBytes, err := yaml.Marshal(spec) if err != nil { ... } input = specBytes } var file SnapshotSpec // Unmarshal 2: to the final struct yaml.Unmarshal(input, &file) return file, nil } After: func readSnapshotSource(input []byte) (SnapshotSpec, error) { // Define a temporary struct to capture the wrapped spec var wrapper struct { Spec app.SnapshotSpec `yaml:"spec"` } // Attempt to unmarshal into the wrapper if err := yaml.Unmarshal(input, &wrapper); err == nil && wrapper.Spec != nil { // If successful and spec exists, return it directly return wrapper.Spec, nil } // Fallback: unmarshal directly into SnapshotSpec for backward compatibility var spec app.SnapshotSpec if err := yaml.Unmarshal(input, &spec); err != nil { return SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification: %w", err) } return spec, nil } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an inefficient marshal/unmarshal cycle and proposes a more idiomatic and performant solution using a temporary struct, which improves code quality and clarity.	Medium
Possible issue	✅ ~~Avoid logging raw byte slice~~ Suggestion Impact: The commit replaced logging of the raw input with logging just the error and updated the error message accordingly. code diff: - log.Debugf("Problem parsing application snapshot from file %s", input) - return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) + log.Debugf("Problem parsing application snapshot from input: %v", err) + return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from input: %w", err) Modify error logging to avoid printing the raw input content, which can be large and may contain sensitive data. Instead, log only the error itself. internal/applicationsnapshot/input.go [189-192] if err := yaml.Unmarshal(input, &v); err != nil { - log.Debugf("Problem parsing application snapshot from file %s", input) - return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from %s: %w", input, err) + log.Debugf("Problem parsing application snapshot from input: %v", err) + return app.SnapshotSpec{}, fmt.Errorf("unable to parse Snapshot specification from input: %w", err) } `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that logging the entire raw `input` byte slice is poor practice, as it can create excessively large log entries and may expose sensitive information.	Medium

* Modify readSnapshotSource to automatically extract the .spec field when the input is a Kubernetes cluster record, matching the behavior for policy.yaml files. This allows direct use of kubectl output without manual jq extraction. * The change maintains backward compatibility with direct SnapshotSpec input and works with both JSON and YAML formats. resolves: EC-1534