fix(kwctl): normalize registry:// URIs to include explicit tag#1595
fix(kwctl): normalize registry:// URIs to include explicit tag#1595jvanz merged 1 commit intokubewarden:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes kwctl’s handling of registry:// policy URIs that omit an explicit tag by normalizing them to include the default :latest tag, ensuring subsequent commands can consistently locate policies stored on disk.
Changes:
- Normalize
registry://URIs viaReference::from_strinsidemap_path_to_uri, so missing tags resolve to:latest. - Add unit tests covering normalization for untagged URIs and ensuring tagged/digest URIs remain unchanged.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1595 +/- ##
==========================================
+ Coverage 75.48% 80.50% +5.01%
==========================================
Files 170 127 -43
Lines 20889 16411 -4478
==========================================
- Hits 15769 13211 -2558
+ Misses 4907 3200 -1707
+ Partials 213 0 -213
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // This ensures the URI matches the path used by the store, which always | ||
| // stores policies with an explicit tag. | ||
| if let Some(image) = uri_or_sha_prefix.strip_prefix("registry://") | ||
| && let Ok(reference) = Reference::from_str(image) |
There was a problem hiding this comment.
Ok, this was a bit unclear for me. The normalization happens on the Reference::from_str() call, as it does a try_from(), and does a:
if tag.is_none() && digest.is_none() {
tag = Some(DEFAULT_TAG.into());
}I've seen also that we do normalization in policy-fetcher: https://github.com/kubewarden/kubewarden-controller/blob/main/crates/policy-fetcher/src/lib.rs#L114-L117
Are we sure we always use map_path_to_uri() for each time that we interact with the local store?
I wonder if we can try to normalize the uri once per kwctl subcommand, when we first obtain or parse the uri or reference. We could add a normalize_uri(uri) function that gets called the first time or only time we pass the uri downwards in each subcommand (pull, push, pull-and-run, etc).
There was a problem hiding this comment.
The function from policy-fetcher is being called and it's properly fixing the tag. You can see that when you see the issue description issue.The pulling of the policies is running. However, the manifest command later try to find the policy. But it does not fix the tag. That's why it cannot find the policy later.
kwctl scaffold manifest -t ClusterAdmissionPolicy registry://ghcr.io/kubewarden/policies/pod-privileged
2026-03-23T13:07:23.920603Z INFO kwctl: cannot find policy with uri: registry://ghcr.io/kubewarden/policies/pod-privileged, trying to pull it from remote registry
Successfully pulled policy from registry://ghcr.io/kubewarden/policies/pod-privileged
Error: Cannot find policy with uri: registry://ghcr.io/kubewarden/policies/pod-privileged
So, yes, we can think of an normalization function to be called before any operation. Taking a look in the code, this function is the closest one that we have that is similar to what you proposed as normalize_uri. Because it is called several places to get the URI. Including the function get_uri in the same file. I'm not saying that it's called everywhere. But I can use it to fix other places to do the same thing in other places.
By the way, the get_uri is called by get_wasm_path which is called by pull_if_needed that later call the fetch_policy.
There was a problem hiding this comment.
I see, thanks. Can we ensure the normalization always happens by dealing with References in these cases, as they are normalized, just like in this proposed change? that may be a simple solution.
There was a problem hiding this comment.
I agree the we could improve the normalization. But to properly do that we should go a little bit beyond. The References object is used only for "registry" policy types. I think if we decide to do that we should create an enum to represent any kind of policy references (http, file and OCI). Therefore, we can pass this information everywhere and abstract the logic of normalization and so on. Which also includes a refactoring of all the API from policy-evaluator and policy-fetcher. I'm in favor of it. It looks the like good solution for the long run. But I would like to avoid doing this on this PR. Looks like we are doing two tasks: fixing the bug and refactoring the code base in a single PR. Where the goal is to fix a bug only.
Therefore, I propose the following, I'll create an issue to fix this tech-bebt and for this PR, I'll update the kwctl code to normalize the reference string in the commands.
How that sounds?
There was a problem hiding this comment.
I've updated the code to give an example of normalization of the string in the sub commands. Take a look. I did not review all the sub commands yet. But I would like to share an idea that can impact if we will do that or not...
While I was doing that I though that is not required. In the end of the day, the previous code already fix the function called by the sub commands. Therefore, we could just ending by adding a second normalization check. I think we could remove the code from the sub command calling the normalization function and leave the proper fix where we replace the string by some type representing the all the possibles policies URI to a future tech-debt issue. As we are discussing in the previous comments
89a3852 to
5f9e050
Compare
viccuad
left a comment
There was a problem hiding this comment.
I'm approving, I like this approach more than the one we had before, as we are aware now that we are normalizing.
I would still want to open a tech debt issue to use a type or enum, if we aren't tackling that in this PR.
flavio
left a comment
There was a problem hiding this comment.
LGTM, I left some really minor comments. You can ignore them if you want
When a policy URI without a tag is used (e.g. registry://ghcr.io/kubewarden/policies/pod-privileged), the pull step stores the policy under a path with ':latest' appended, but subsequent commands (scaffold, inspect, rm) looked up the URI without the tag, causing them to fail with 'Cannot find policy with uri'. Fix by normalizing registry:// URIs in normalize_uri via Reference::from_str, which defaults to ':latest' when no tag is specified. This makes the lookup key consistent with the store path. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com> Assisted-by: Github Copilot
|
@flavio I've addressed your request. As they are pretty simple, I'm merging this PR right away. |
Description
When a policy URI without a tag is used (e.g. registry://ghcr.io/kubewarden/policies/pod-privileged), the pull step stores the policy under a path with ':latest' appended, but subsequent commands (scaffold, inspect, rm) looked up the URI without the tag, causing them to fail with 'Cannot find policy with uri'.
Fix by normalizing registry:// URIs in map_path_to_uri via Reference::from_str, which defaults to ':latest' when no tag is specified. This makes the lookup key consistent with the store path.
Fix #1547
Test