Fix misleading WARN when polling connector status after KafkaConnector delete

[Paramesh324]

Type of change

• Bugfix

Description

After a successful DELETE of a connector, the operator polls GET /connectors/{name}/status until it gets 404. While the connector is still shutting down, Connect often returns 200 with normal status JSON (no Kafka Connect message field).

Previously, doGet treated any non-allowed status as an error response and passed the body through tryToExtractErrorMessage, which logged a WARN about failing to decode an error—confusing for operators.

This change fails unexpected 2xx responses with a plain ConnectRestException message instead of parsing the body as an error, so the backoff/retry behavior is unchanged (we still only treat 404 as success for this poll; 200 must continue to fail so withBackoff retries). A WireMock unit test covers the post-delete poll scenario.

Note: The idea of adding 200 and 404 both as “success” for status() would make the first 200 complete the future successfully and stop backoff immediately, so we do not use that approach.

Closes #12681

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.16%. Comparing base (169cc6a) to head (2ff2715).

Files with missing lines	Patch %	Lines
...cluster/operator/assembly/KafkaConnectApiImpl.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12710      +/-   ##
============================================
- Coverage     75.16%   75.16%   -0.01%     
- Complexity     6452     6457       +5     
============================================
  Files           346      346              
  Lines         24325    24329       +4     
  Branches       3120     3121       +1     
============================================
+ Hits          18283    18286       +3     
- Misses         4805     4807       +2     
+ Partials       1237     1236       -1     

Files with missing lines	Coverage Δ
...cluster/operator/assembly/KafkaConnectApiImpl.java	59.68% <83.33%> (-0.76%)	⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[tinaselenge]

Thanks for the PR. Left a few minor comments but mostly looks good to me.

[tinaselenge]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tinaselenge]

Thanks, LGTM now.

[tinaselenge]

/gha run pipeline=regression

[github-actions]

⏳ System test verification started: link

The following 6 job(s) will be executed:

• regression-brokers-and-security-amd64 (cncf-ubuntu-8-32-x86)
• regression-operators-amd64 (cncf-ubuntu-8-32-x86)
• regression-operands-amd64 (cncf-ubuntu-8-32-x86)
• regression-brokers-and-security-arm64 (cncf-ubuntu-8-32-arm)
• regression-operators-arm64 (cncf-ubuntu-8-32-arm)
• regression-operands-arm64 (cncf-ubuntu-8-32-arm)

Tests will start after successful build completion.

[github-actions]

🎉 System test verification passed: link

[katheris]

Given that the tryToExtractErrorMessage method already has handling for when the response is not JSON, I think it would be better to update that method to better handle when it's JSON but is missing the error field, rather than having status code specific handling here that's for one edge case. WDYT @Paramesh324 @tinaselenge ?

[Paramesh324]

Thanks for the suggestion — I considered moving this into tryToExtractErrorMessage for JSON without a message field, but that would also change behavior for real 4xx/5xx errors where we still want the WARN today.

In the delete flow we poll GET .../status with only 404 in okStatusCodes. Interim 200 responses carry normal connector status JSON, not a Connect error body. We fail those on purpose so backoff keeps retrying until 404 — the body isn’t an error payload, so handling unexpected 2xx in doGet (without calling tryToExtractErrorMessage) keeps the fix targeted and gives a clearer message (Unexpected HTTP status code 200). Backoff is unchanged.

Happy to refactor into tryToExtractErrorMessage if you’d prefer that centralized approach.

[Paramesh324]

@katheris @tinaselenge

[tinaselenge]

I think tryToExtractErrorMessage() method has a clear intention as it is, to extract an error message when there is an error. I'm worried if we handle it in the method, it could make it more confusing, because we would have to pass it an additional context (like the status code) to determine whether to parse as error. I'm ok with the current changes, since this is the only method we consider 2xx status as failure.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.