Skip to content

backend: Stop parseKubeConfig after bad JSON#5599

Draft
harrshita123 wants to merge 1 commit into
kubernetes-sigs:mainfrom
harrshita123:issue-5598-parse-kubeconfig-json
Draft

backend: Stop parseKubeConfig after bad JSON#5599
harrshita123 wants to merge 1 commit into
kubernetes-sigs:mainfrom
harrshita123:issue-5598-parse-kubeconfig-json

Conversation

@harrshita123
Copy link
Copy Markdown
Contributor

Summary

This PR fixes a backend bug where /parseKubeConfig continued processing after receiving an invalid JSON request body.

The handler already returned a 400 Bad Request, but it did not stop execution afterward. As a result, it could continue with an empty request payload and try to write a second response. This change adds the missing early return and covers the behavior with a regression test.

Related Issue

Fixes #5598

Changes

  • Added an early return after invalid JSON is handled in parseKubeConfig.
  • Added a regression test to confirm malformed JSON returns only the expected 400 Bad Request response.

Steps to Test

  1. Run the focused backend test:

    cd backend && go test ./cmd -run 'TestParseKubeConfigInvalidJSON|TestStatelessClustersKubeConfig'
  2. Run backend lint:

    npm run backend:lint
  3. Run the full backend test suite:

    npm run backend:test

Screenshots (if applicable)

Not applicable. This is a backend request-handling fix.

Notes for the Reviewer

The production change is intentionally small: one missing return after the bad request response. Valid kubeconfig parsing behavior is unchanged.

@k8s-ci-robot k8s-ci-robot requested review from skoeva and sniok May 13, 2026 06:53
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 13, 2026
@illume illume requested a review from Copilot May 13, 2026 08:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the backend /parseKubeConfig handler where it would continue executing after failing to decode an invalid JSON request body, potentially attempting to write a second response. It adds the missing early return and introduces a regression test to ensure the handler responds with only the expected 400 Bad Request.

Changes:

  • Add an early return after http.Error(...) on JSON decode failure in parseKubeConfig.
  • Add a regression test asserting malformed JSON yields a single 400 response with the expected error body.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
backend/cmd/stateless.go Stops handler execution immediately after invalid JSON is detected to avoid double-writing responses.
backend/cmd/stateless_test.go Adds a regression test ensuring malformed JSON returns only the expected 400 error response.

Copy link
Copy Markdown
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi! would you be able to squash your merge commits? this should be helpful to look through

also, could you take a look at the failing backend test?

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 13, 2026
@harrshita123
Copy link
Copy Markdown
Contributor Author

@skoeva
Thanks. I fixed the backend CI timeout and synced the plugin dependencies so the failing check passes again. I’ll squash the commits before merging.

@illume illume requested a review from Copilot May 13, 2026 20:52
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.

the PR has a merge-main commit; please rebase against main to keep the history clean.

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.

Files not reviewed (2)
  • plugins/headlamp-plugin/package-lock.json: Language not supported
  • plugins/headlamp-plugin/template/package-lock.json: Language not supported

Comment thread plugins/headlamp-plugin/package.json
Comment thread plugins/headlamp-plugin/package-lock.json
Comment thread plugins/headlamp-plugin/template/package-lock.json Outdated
Comment thread .github/workflows/backend-test.yml
@harrshita123 harrshita123 force-pushed the issue-5598-parse-kubeconfig-json branch from 27e348a to 01b3111 Compare May 13, 2026 21:00
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2026
@harrshita123
Copy link
Copy Markdown
Contributor Author

Contributor

Thanks for this PR.

the PR has a merge-main commit; please rebase against main to keep the history clean.

Why this matters

@illume
Thanks for the review. I rebased the branch on top of main and removed the unrelated workflow and plugin dependency changes. The PR now only contains the backend fix for parseKubeConfig and the regression test. I also verified the backend test suite passes locally.

@illume illume requested a review from Copilot May 14, 2026 08:19
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

would you mind rebasing this branch on top of the latest main rather than merging it in?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes.

can you please rebase against main to remove the merge main commit?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

would you mind rebasing this branch on top of the latest main rather than merging it in?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

@illume illume requested a review from Copilot May 15, 2026 06:23
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

it looks like there's a merge-main commit in this PR — could you rebase onto main instead?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 15, 2026
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 15, 2026
@illume illume requested a review from Copilot May 15, 2026 12:42
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

it looks like there's a merge-main commit in this PR — could you rebase onto main instead?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

The backend test job in CI is failing. Run cd backend && go test ./... to reproduce the errors locally.

How to run the backend tests

Run cd backend && go test ./... to see all failures. Fix the failing tests and commit the result.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread backend/cmd/stateless_test.go Outdated
@illume illume marked this pull request as draft May 15, 2026 18:35
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2026
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes.

can you please rebase against main to remove the merge main commit?

Why this matters

Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Can you please address the open review comments? Once you've resolved each one, please mark it as resolved.

@harrshita123 harrshita123 force-pushed the issue-5598-parse-kubeconfig-json branch from 3fbc9f1 to 3df5211 Compare May 15, 2026 20:07
@harrshita123 harrshita123 reopened this May 15, 2026
@harrshita123 harrshita123 force-pushed the issue-5598-parse-kubeconfig-json branch from 438b147 to aae9951 Compare May 15, 2026 20:16
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harrshita123
Once this PR has been reviewed and has the lgtm label, please assign sniok for approval. For more information see the Code Review Process.

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

@harrshita123 harrshita123 force-pushed the issue-5598-parse-kubeconfig-json branch from aae9951 to 3ea1f81 Compare May 15, 2026 20:19
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 15, 2026
@harrshita123
Copy link
Copy Markdown
Contributor Author

harrshita123 commented May 15, 2026

Thanks for these changes.

can you please rebase against main to remove the merge main commit?

Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.

Can you please address the open review comments? Once you've resolved each one, please mark it as resolved.

@illume
Thanks for the review. I rebased the branch onto the latest upstream main and I resolved all copilot comments .

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 17, 2026
@harrshita123 harrshita123 force-pushed the issue-5598-parse-kubeconfig-json branch from fb76cca to 6dc8d2c Compare May 17, 2026 11:26
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

backend: parseKubeConfig continues after invalid JSON body

5 participants