meshery / meshery/meshkit

Fix pre-existing duplicate error codes in MeshKit

Open
#1,106 7 comments 0 reactions 0 assignees View on GitHub
kind/bug
Dominant language
Go
Stars
223
Forks
225
Avg merge
7d 1h
Merged PRs (30d)
1

Description

# Duplicate error codes in MeshKit master

## Context

While validating the `errorutil check` workflow, I found that `meshery/meshkit` `origin/master` currently contains **14 duplicate integer error codes across 30 error constants**.

Verified against:
- `meshkit` `origin/master`
- Commit: `917674d`
- `next_error_code = 11328`
- Total integer-coded errors = 224

These duplicates are currently not prevented by MeshKit CI. `error-codes-updater.yaml` runs on `push` / `workflow_dispatch` rather than pull requests, and the existing duplicate detection in `SummarizeAnalysis` logs duplicates but does not return an error.

As a result, these pre-existing duplicate codes can remain in the repository without failing validation.

### Existing duplicate codes

**11187**
- `ErrConvertToByteCode` — `utils/error.go`
- `ErrDryRunHelmChartCode` — `utils/helm/error.go`

**11205**
- `ErrRestConfigFromKubeConfigCode` — `utils/kubernetes/error.go`
- `ErrExposeResourceCode` — `utils/kubernetes/expose/error.go`

**11233**
- `ErrGetCrdNamesCode` — `utils/manifests/error.go`
- `ErrMultipleDocumentsCode` — `utils/kubernetes/kompose/error.go`

**11243**
- `ErrConnectingToRegistryCode` — `models/oci`
- `ErrUpdateEntityStatusCode` — `models/meshmodel/entity`

**11245**
- `ErrAuthenticatingToRegistryCode` — `models/oci`
- `ErrDecodeYamlCode` — `encoding`
- `ErrUnknownFormatCode` — `models/converter`

**11246**
- `ErrWriteFilesCode` — `models/oci`
- `ErrUnmarshalCode` — `encoding`

**11247**
- `ErrAddLayerCode` — `models/oci`
- `ErrUnmarshalInvalidCode` — `encoding`

**11248**
- `ErrTaggingPackageCode` — `models/oci`
- `ErrUnmarshalSyntaxCode` — `encoding`
- `ErrCompressToTarGZCode` — `utils`

**11249**
- `ErrPushingPackageCode` — `models/oci`
- `ErrUnmarshalTypeCode` — `encoding`

**11301**
- `ErrInvalidModelCode` — `files`
- `ErrMarshalStructToCSVCode` — `utils/csv/errors.go`

**11302**
- `ErrInvalidModelArchiveCode` — `files`
- `ErrAppendToSheetCode` — `registry`

**11303**
- `ErrEmptyModelCode` — `files`
- `ErrUpdateToSheetCode` — `registry`

**11305**
- `ErrUncompressedTarCode` — `files`
- `ErrGeneratesModelCode` — `registry`

**11327**
- `ErrUnmarshalSchemaAssetCode` — `schema/error.go`
- `ErrUnsubscribeCode` — `broker/nats/error.go`

### Additional finding: files not updated by errorutil update

The current `errorutil update` implementation rewrites files whose basename is exactly `error.go`:

```go
func isErrorGoFile(path string) bool {
_, file := filepath.Split(path)
return file == "error.go"
}
```

Other `.go` files can still be analyzed, but are not rewritten by the update path. Examples of error declarations currently outside `error.go` include:

- `logger/controller.go` — `ErrControllerCode` — `11071`
- `utils/csv/errors.go` — `ErrMarshalStructToCSVCode` — `11301`
- `utils/csv/errors.go` — `ErrReadCSVRowCode` — `11306`
- `validator/validate.go` — `ErrValidateCode` — no integer code

> [!NOTE]
> This is included as a related finding only. Whether restricting updates to `error.go` is intentional or an unintended limitation should be investigated separately before changing the allocation implementation.

## Proposed remediation

This issue should be handled separately from the `errorutil check` validation implementation.

The cleanup should:
1. Verify the history of each conflicting code before deciding which constant should retain it.
2. Keep one live error constant per integer code.
3. Renumber the remaining conflicting constants using fresh allocations beginning at the current `next_error_code`.
4. Update any affected references and documentation as necessary.
5. Re-run error-code analysis to confirm that duplicate integer codes are eliminated.

> [!IMPORTANT]
> The exact number of fresh codes required should be determined after reviewing each collision and its history rather than assumed from the current grouping. Because error codes may be externally referenced, renumbering may be user-visible. The historical order and relative importance of the conflicting constants should therefore be established before deciding which constant keeps the existing code.

## Related work

This issue was discovered while working on:
- MeshKit #1080 — `errorutil check`, already merged
- MeshKit #1105 — allocation-aware `errorutil check` follow-up
- Meshery #21009 — Meshery PR-level validation

The validation work intentionally treats these duplicate codes as pre-existing baseline debt so that unrelated PRs are not blocked by historical violations.

## Out of scope

This issue does not propose:
- Merge Queue
- A new allocation service
- A replacement allocation mechanism
- Changing the current `errorutil check` PR scope

The separate question of whether `errorutil update` should support error declarations outside `error.go` should be investigated before making any corresponding tooling change.

## Acceptance criteria
- [ ] No duplicate integer error codes remain in the targeted MeshKit error set.
- [ ] Renumbered errors use unique codes from the valid allocation range.
- [ ] Affected references and documentation are updated where required.
- [ ] Error-code analysis confirms no remaining duplicate integer codes.
- [ ] Future validation can detect newly introduced duplicate codes without failing on inherited baseline debt.

Contributor guide

Open the contributing guide

Research direction

Start with the listed error declarations in utils/error.go, utils/helm/error.go, utils/kubernetes/error.go, models/oci, encoding, files, registry, schema/error.go, and broker/nats/error.go, then review their history and affected references. Run the existing errorutil analysis to establish the duplicate baseline. Done means each targeted integer code is unique, references and documentation are updated where needed, and analysis reports no duplicates.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.