Skip to content

fix: convert non-string endpoint attributes from v1alpha2 to v1alpha1 - #1791

Open
tolusha wants to merge 1 commit into
devfile:mainfrom
tolusha:fix-endpoint-attributes-conversion
Open

tolusha wants to merge 1 commit into
devfile:mainfrom
tolusha:fix-endpoint-attributes-conversion

Conversation

@tolusha

@tolusha tolusha commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Description of Changes

Fixes the v1alpha2 -> v1alpha1 conversion of endpoints whose attributes hold a non-string value.

Endpoint attributes are free-form (map[string]apiext.JSON) in v1alpha2 but string-based (map[string]string) in v1alpha1. Conversion from v1alpha2 is implemented as a JSON round-trip, so an endpoint declaring something like:

components:
  - name: postgresql
    container:
      image: postgres:latest
      endpoints:
        - name: postgresql
          targetPort: 5432
          attributes:
            discoverable: true

fails to convert with:

cannot unmarshal bool into Go struct field Endpoint.container.endpoints.attributes of type string

This PR rewrites the endpoint attributes of the marshalled component into their string representation before decoding into v1alpha1.

ses these strings back when converting to v1alpha2 again, so discoverable: true comes out of a v1alpha2 -> v1alpha1 -> v1alpha2 round-trip as the string "true". Scalars stay usable, because GetBoolean and GetNumber fall back to strconv when the attribute holds a string, but an object or an array comes back as a string and no longer decodes with GetInto.

Parsing the strings back is not an option: a string attribute the user actually authored as "true" cannot be told apart from a stringified boolean. This is inherent to the v1alpha1 schema, which has no way to represent a non-string attribute.

The conversion is one way. Nothing parses these strings back when converting to v1alpha2 again, so discoverable: true comes out of a v1alpha2 -> v1alpha1 -> v1alpha2 round-trip as the string "true". Scalars stay usable, because GetBoolean and GetNumber fall back to strconv when the attribute holds a string, but an object or an array comes back as a string and no longer decodes with GetInto.

Parsing the strings back is not an option: a string attribute the user actually authored as "true" cannot be told apart from a stringified boolean. This is inherent to the v1alpha1 schema, which has no way to represent a non-string attribute.

Related Issue(s)

https://redhat.atlassian.net/browse/CRW-12643

Acceptance Criteria

Testing and documentation do not need to be complete in order for this PR to be approved. However, tracking issues must be opened for missing testing/documentation.

New testing and documentation issues can be opened under devfile/api/issues.

You can check the respective criteria below if either of the following is true:

  • There is a separate tracking issue opened and that issue is linked in this PR.
  • Testing/documentation updates are contained within this PR.

If criteria is left unchecked please provide an explanation why.

Tests Performed

Explain what tests you personally ran to ensure the changes are functioning as expected.

How To Test

  1. Clone and install DWO
git clone git@github.com:devfile/devworkspace-operator.git
cd devworkspace-operator
make install
  1. Create DW with discoverable: true
oc apply -f -<<EOF
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: endpoint-attributes-conversion-bug
spec:
  started: false
  routingClass: 'basic'
  template:
    parent:
      uri: https://example.com/devfile.yaml
      components:
        - name: postgresql
          container:
            image: quay.io/wto/web-terminal-tooling:next
            endpoints:
              - name: postgresql
                targetPort: 5432
                exposure: internal
                attributes:
                  # boolean instead of "true" - valid in v1alpha2, breaks conversion to v1alpha1
                  discoverable: true
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          command:
            - "tail"
            - "-f"
            - "/dev/null"
EOF
  1. Check DWO logs.
oc logs -n devworkspace-controller deployment/devworkspace-controller-manager -f

...
"json: cannot unmarshal bool into Go struct field Endpoint.container.endpoints.attributes of type string"
...
  1. Update devfile/api dependency
go mod edit -replace github.com/devfile/api/v2=github.com/tolusha/api/v2@fix-endpoint-attributes-conversion
go mod tidy
  1. Build image (or use quay.io/abazko/operator:dw-test) and update DWO deployment
make docker DWO_IMG=quay.io/abazko/operator:dw-test  
oc set image deployment/devworkspace-controller-manager devworkspace-controller=quay.io/abazko/operator:dw-test -n devworkspace-controller
  1. Check logs, there are no more errors
oc logs -n devworkspace-controller deployment/devworkspace-controller-manager -f

Notes To Reviewer

Any notes you would like to include for the reviewer.

Endpoint attributes are free-form (`map[string]apiext.JSON`) in v1alpha2
but string-based (`map[string]string`) in v1alpha1. Conversion from
v1alpha2 is implemented as a JSON round-trip, so an endpoint holding a
non-string attribute such as `discoverable: true` failed to convert with:

  cannot unmarshal bool into Go struct field
  Endpoint.container.endpoints.attributes of type string

Rewrite the endpoint attributes of the marshalled component into their
string representation before decoding into v1alpha1. Values keep their
verbatim JSON text rather than going through `GetString`, so no precision
is lost: `1048576` becomes "1048576" and not "1.048576e+06".

The three conversion paths that decode a component from v1alpha2 are
covered: plain components, plugin component overrides and parent
component overrides, across the container, kubernetes and openshift
component types.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
tolusha requested review from a team, AObuchow and dkwon17 as code owners October 6, 2026 10:18
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 608a43f6-16bb-47c1-b6bd-663c624f6b4e
📥 Commits

Reviewing files that changed from the base of the PR and between 801f7ee and f99b218.

📒 Files selected for processing (5)
  • pkg/apis/workspaces/v1alpha1/component_plugin_conversion.go
  • pkg/apis/workspaces/v1alpha1/components_conversion.go
  • pkg/apis/workspaces/v1alpha1/endpoint_conversion.go
  • pkg/apis/workspaces/v1alpha1/endpoint_conversion_test.go
  • pkg/apis/workspaces/v1alpha1/parent_conversion.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The v1alpha2-to-v1alpha1 component conversion paths now stringify endpoint attribute values before decoding the destination. Tests cover container components, plugin component overrides, parent component overrides, and several JSON value types.

Changes

Endpoint Attribute Conversion

Layer / File(s) Summary
Stringify endpoint attributes
pkg/apis/workspaces/v1alpha1/endpoint_conversion.go
Helpers identify supported component types and convert endpoint attribute values to strings. String values are unquoted; other values retain their JSON text, and empty raw values become "null".
Apply stringification in conversion paths
pkg/apis/workspaces/v1alpha1/components_conversion.go, pkg/apis/workspaces/v1alpha1/component_plugin_conversion.go, pkg/apis/workspaces/v1alpha1/parent_conversion.go, pkg/apis/workspaces/v1alpha1/endpoint_conversion_test.go
Component, plugin subcomponent, and parent component conversions stringify endpoint attributes before unmarshalling. Tests cover booleans, numbers, strings, objects, arrays, null, and empty attributes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f99b2

No identified conversion defect remains; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f99b2

The change is confined to API conversion and preserves endpoint identity and security fields. No introduced authorization bypass was established. Attribute types are not restored after conversion through the older API version, and downstream uses of structured attributes remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The evidenced direct impact is conversion of caller-supplied endpoint attributes across three component paths. The changed helper does not itself grant privileges or perform network operations. Tenant, deployment, and downstream security-policy exposure cannot be determined from this conversion scope.

Trust Boundaries and Controls

  • observed — Normalization is a schema adapter, not authorization or sanitization. It retains non-attribute endpoint members, including name, targetPort, exposure, protocol, secure, and path; no weakening of those fields was found in the changed transformation.

Resilience and Maintainability Implications

  • observed — Inspected template, parent, and plugin callers attach each temporary component destination only after successful conversion. Whole-object conversion is not transactional: prior successful siblings can remain in a destination after a later error, and repeated calls can append to existing slices. Those behaviors predate this PR; the new helper introduces no shared-state reservation or recovery lifecycle.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: converting non-string endpoint attributes from v1alpha2 to v1alpha1.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tolusha tolusha mentioned this pull request Oct 6, 2026
1 of 4 tasks
@codecov

codecov Bot commented Oct 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 59.57447% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 30.36%. Comparing base (0c95025) to head (f99b218).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...kg/apis/workspaces/v1alpha1/endpoint_conversion.go 67.08% 17 Missing and 9 partials ⚠️
.../apis/workspaces/v1alpha1/components_conversion.go 14.28% 4 Missing and 2 partials ⚠️
...workspaces/v1alpha1/component_plugin_conversion.go 25.00% 2 Missing and 1 partial ⚠️
pkg/apis/workspaces/v1alpha1/parent_conversion.go 25.00% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (0c95025) and HEAD (f99b218). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (0c95025) HEAD (f99b218)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
- Coverage   35.75%   30.36%   -5.40%     
==========================================
  Files          52       68      +16     
  Lines        6696     8162    +1466     
==========================================
+ Hits         2394     2478      +84     
- Misses       4158     5525    +1367     
- Partials      144      159      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rohanKanojia

Copy link
Copy Markdown
Member

I tested with the abovementioned steps and can confirm it works as expected ✔️

@openshift-ci

openshift-ci Bot commented Oct 6, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dkwon17, rohanKanojia, tolusha

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants