Fix panic when installed package uses digest reference by AndrewCharlesHay · Pull Request #7293 · crossplane/crossplane

AndrewCharlesHay · 2026-04-09T18:03:18Z

Description of your changes

When the EnableAlphaDependencyVersionUpgrades feature flag is enabled and an installed package uses a digest reference (e.g. @sha256:...) instead of a semver tag, findDependencyVersionToUpdate panics at semver.MustParse because the digest string is not valid semver.

This adds a check for digest-format installed versions before attempting semver parsing. When the installed version is a digest, the function returns the lowest available version that satisfies all parent constraints, allowing the upgrade to proceed.

Covered by a new SuccessDigestInstalledVersion test case in TestReconcilerFindDependencyVersionToUpgrade.

Fixes #7006

I have:

Read and followed Crossplane's contribution process.
~~Run ./nix.sh flake check to ensure this PR is ready for review.~~
Added or updated unit tests.
~~Added or updated e2e tests.~~
~~Linked a PR or a docs tracking issue to document this change.~~
~~Added backport release-x.y labels to auto-backport this PR.~~
~~Followed the API promotion workflow if this PR introduces, removes, or promotes an API.~~

coderabbitai · 2026-04-09T18:09:18Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7982fec1-a95c-4bf8-a1af-789460abcced

📥 Commits

Reviewing files that changed from the base of the PR and between 99d2718 and 8f9f67d.

📒 Files selected for processing (2)

internal/controller/pkg/resolver/reconciler.go
internal/controller/pkg/resolver/reconciler_test.go

✅ Files skipped from review due to trivial changes (1)

internal/controller/pkg/resolver/reconciler_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

internal/controller/pkg/resolver/reconciler.go

📝 Walkthrough

Walkthrough

findDependencyVersionToUpdate now detects digest-style installed versions and avoids semver MustParse panics: digests produce a specific error when parent semver constraints exist; non-digest, non-semver identifiers return a wrapped "Invalid Semantic Version" error. Selection logic for semver upgrades is unchanged.

Changes

Cohort / File(s)	Summary
Resolver logic `internal/controller/pkg/resolver/reconciler.go`	Replace `semver.MustParse` with validated parsing via `semver.NewVersion`; detect digest-style `insVer` using `conregv1.NewHash` and return an actionable error when a digest cannot be reconciled against parent semver constraints.
Unit tests `internal/controller/pkg/resolver/reconciler_test.go`	Add tests covering digest installed-version vs. semver parent-constraints (`ErrorDigestInstalledVersionWithSemverConstraints`) and non-semver, non-digest installed-version (`ErrorNonSemverInstalledVersion`).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

pkg: Correctly handle dependency upgrades with ImageConfig prefix rewriting #7233: Modifies how the reconciler derives/parses the installedVersion (changes preference between spec.package and status.resolvedPackage); closely related to installed-version handling and upgrade resolution.

Suggested reviewers

negz
haarchri

Thank you — would you like me to add a short note to the changelog entry or verify any specific error messages in the tests?

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing a panic when installed packages use digest references instead of semver tags.
Description check	✅ Passed	The description comprehensively explains the problem, the solution approach, and references the linked issue `#7006` with appropriate testing documentation.
Linked Issues check	✅ Passed	The PR fully addresses issue `#7006` by preventing the panic via digest detection before semver parsing and returning actionable errors when digest and semver constraints conflict.
Out of Scope Changes check	✅ Passed	All changes are strictly scoped to fixing the panic in findDependencyVersionToUpdate and adding appropriate test coverage; no unrelated modifications are present.
Breaking Changes	✅ Passed	The custom check for breaking changes applies only to files under 'apis/' or 'cmd/' directories. This PR modifies only internal implementation files in 'internal/controller/pkg/resolver/', which are outside the scope of the check.
Feature Gate Requirement	✅ Passed	PR fixes a bug in existing experimental feature without introducing new experimental functionality. The findDependencyVersionToUpdate function is already gated by EnableAlphaDependencyVersionUpgrades feature flag.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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 and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/controller/pkg/resolver/reconciler.go`:
- Around line 560-581: The code currently calls semver.MustParse(insVer)
(creating currentVersion) which can panic for non-semver tags like "latest" or
"main"; replace the MustParse call with a non-panicking parse (e.g.,
semver.Parse/ParseTolerant/semver.NewVersion) and handle the returned error
gracefully: if parsing fails, log a debug/error with context (including
dep.Identifier() and parent constraints) and return a descriptive error (or
apply the same fallback logic used for digest refs) instead of panicking. Ensure
you update the currentVersion usage to work with the parsed value and
early-return on parse error.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fa37a8f2-a29d-402e-9a85-4810881d8dd5

📥 Commits

Reviewing files that changed from the base of the PR and between 7f7caf2 and 26dffa2.

📒 Files selected for processing (2)

internal/controller/pkg/resolver/reconciler.go
internal/controller/pkg/resolver/reconciler_test.go

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/controller/pkg/resolver/reconciler.go`:
- Around line 581-583: The returned error when semver.NewVersion(insVer) fails
should include which dependency (e.g., the dependency name or ID available in
scope) and a concrete remediation step; update the error returned from the
semver parse failure (the semver.NewVersion call handling in reconciler.go that
sets currentVersion and insVer) to wrap the original err with a message like:
which dependency/version string failed validation, what operation was being
attempted, and a suggested next step (e.g., "ensure the installed dependency
version is a valid semantic version (vMAJOR.MINOR.PATCH) or use a digest;
correct the installed version or re-run the install"), preserving the original
error for debugging. Ensure you reference semver.NewVersion, insVer and
currentVersion when making the change so the context is clear.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fc7be7c8-9283-4cc3-9b54-69afc2832ba3

📥 Commits

Reviewing files that changed from the base of the PR and between 26dffa2 and ab38985.

📒 Files selected for processing (2)

internal/controller/pkg/resolver/reconciler.go
internal/controller/pkg/resolver/reconciler_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

internal/controller/pkg/resolver/reconciler_test.go

…igest reference When the installed package version is a digest (e.g. sha256:...) rather than a semver tag, findDependencyVersionToUpdate panics at semver.MustParse. This adds a check for digest-format installed versions before attempting semver parsing, returning the lowest available version that satisfies all parent constraints instead. Fixes crossplane#7006 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Andrew Hay <andrew.hay@benchmarkanalytics.com>

Addresses CodeRabbit review: insVer can also be a non-semver tag like "latest" or "main", not just a digest. Replace semver.MustParse with semver.NewVersion to return an error instead of panicking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Andrew Hay <andrew.hay@benchmarkanalytics.com>

Addresses CodeRabbit review: include the dependency identifier and a concrete remediation suggestion in the error message so operators can self-remediate faster. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Andrew Hay <andrew.hay@benchmarkanalytics.com>

adamwg

I was sure I had reviewed a fix for this before, and I did, but the PR was later deleted - that unmerged fix is here. Thanks for picking this up given that the other PR disappeared!

That said, I don't think this fix is the correct behavior. If the installed version uses a digest, there are two possible reasons for that:

The user explicitly installed it by digest.
Another package (or packages) specified a dependency on it by digest.

I believe if we get past lines 518-519 in this method, then we're in the first case (since in the second case findDigestToUpdate will already have returned an error regarding the incompatible constraints). We can't know (without further work - see below) whether the installed (by digest) version is compatible with any given semver. I don't think it's correct to have the resolver switch from a digest pinned version to a semver.

The easy fix is to return an error if a package is installed by digest and a semver constraint is requested in a dep (i.e., the fix linked above). I'm in favor of doing it this way: it's intuitive that an upgrade will fail if a specific version is already pinned and a semver constraint is requested.

A more involved fix would be to check the digest of each available semver. If we find one that matches the installed digest, we could consider the dependency satisfied. We still shouldn't change the installed version but we could at least let dependency resolution move forward. I don't think this is worth doing given that it's kind of a corner case.

…emver Per review feedback: when the installed version is a digest reference (e.g. sha256:abc...), the package has been explicitly pinned — either by the user or via a digest-pinned parent constraint that was already resolved by findDigestToUpdate. Silently switching to the lowest semver that satisfies parent constraints violates operator intent and makes the resolver's behavior surprising. Return an actionable error instead, pointing the operator at the two valid resolutions: update the parent package(s) to request the same digest, or reinstall the package with a semver tag. The test case previously named SuccessDigestInstalledVersion is renamed to ErrorDigestInstalledVersionWithSemverConstraints and now asserts the error return rather than a synthesised semver. Signed-off-by: Andrew Hay <andrew.hay@benchmarkanalytics.com>

AndrewCharlesHay · 2026-04-21T21:21:29Z

Thanks @adamwg — you're right, the silent-switch-to-semver was wrong behavior. Just pushed an update that takes the easy-fix path you outlined (and that the linked commit took): when the installed version is a digest and the dependency has a semver constraint, return an actionable error instead of synthesising a semver.

Specifically:

Replaced the "find the lowest semver satisfying parent constraints" block with a single error return surfaced via a new errFmtInstalledDigestVsSemver constant.
The error message points the operator at the two valid resolutions: update the parent package(s) to request the same digest, or reinstall with a semver tag.
Renamed the SuccessDigestInstalledVersion test case to ErrorDigestInstalledVersionWithSemverConstraints and updated the expectation to assert the error.
The original panic is still prevented — the digest case is caught before semver.NewVersion runs, so no MustParse path remains.

Agreed on leaving the digest-comparison approach (matching semver tag digest vs installed digest) as a follow-up — happy to open a separate issue tracking that if you think it's worth doing.

PTAL when you get a chance.

adamwg

Thanks for the update! I like that this also handles the case where the installed version is neither a digest nor a semver.

One ordering suggestion, otherwise lgtm.

adamwg · 2026-04-23T17:26:38Z

+	// digest without loading every candidate manifest, and silently
+	// switching a digest-pinned package to a semver tag would violate the
+	// operator's intent. Surface the conflict as an actionable error.
+	if _, err := conregv1.NewHash(insVer); err == nil {


It would be better to move this check up above the ListVersions call so that we avoid doing the OCI call, version parsing, etc. when we know we can't use a semver.

AndrewCharlesHay requested review from a team and adamwg as code owners April 9, 2026 18:03

AndrewCharlesHay requested a review from negz April 9, 2026 18:03

coderabbitai Bot added backport release-2.0 backport release-2.1 backport release-2.2 labels Apr 9, 2026

coderabbitai Bot reviewed Apr 9, 2026

View reviewed changes

Comment thread internal/controller/pkg/resolver/reconciler.go Outdated

coderabbitai Bot reviewed Apr 9, 2026

View reviewed changes

Comment thread internal/controller/pkg/resolver/reconciler.go Outdated

AndrewCharlesHay and others added 3 commits April 9, 2026 13:34

AndrewCharlesHay force-pushed the fix/digest-version-panic branch from 8360f56 to 99d2718 Compare April 9, 2026 18:34

adamwg requested changes Apr 15, 2026

View reviewed changes

adamwg requested changes Apr 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic when installed package uses digest reference#7293

Fix panic when installed package uses digest reference#7293
AndrewCharlesHay wants to merge 4 commits into
crossplane:mainfrom
AndrewCharlesHay:fix/digest-version-panic

AndrewCharlesHay commented Apr 9, 2026

Uh oh!

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

adamwg left a comment

Uh oh!

AndrewCharlesHay commented Apr 21, 2026

Uh oh!

adamwg left a comment

Uh oh!

adamwg Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

AndrewCharlesHay commented Apr 9, 2026

Description of your changes

Uh oh!

coderabbitai Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamwg left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewCharlesHay commented Apr 21, 2026

Uh oh!

adamwg left a comment

Choose a reason for hiding this comment

Uh oh!

adamwg Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading