Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kbld fails on image: field in Kyverno CRD #344

Closed
blairdrummond opened this issue Mar 7, 2023 · 11 comments
Closed

kbld fails on image: field in Kyverno CRD #344

blairdrummond opened this issue Mar 7, 2023 · 11 comments
Labels
carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request stale This issue has had no activity for a while and will be closed soon

Comments

@blairdrummond
Copy link

blairdrummond commented Mar 7, 2023

What steps did you take:

I want to digest-ify all my images in all my manifests, but kbld fails ungracefully on image: fields that don't contain image strings in my CRDs. That might seem weird, but it shows up a lot in admission control in the form of regexps. This prevents me from effectively using the tool at the moment.

cat <<EOF | kbld -f -
apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  annotations:
    policies.kyverno.io/category: Best Practices
    policies.kyverno.io/description: Images from unknown, public registries can be of dubious quality and may not be scanned and secured, representing a high degree of risk. Requiring use of known, approved registries helps reduce threat exposure by ensuring image pulls only come from them. This sample validates that container images only originate from the registry eu.foo.io or bar.io. You can also force images to be immutable references by requiring a checksum.
    policies.kyverno.io/minversion: 1.3.0
    policies.kyverno.io/severity: medium
    policies.kyverno.io/subject: Pod
    policies.kyverno.io/title: Restrict Image Registries
  name: restrict-image-registries
spec:
  background: false
  rules:
    - match:
        resources:
          kinds:
            - Pod
      name: validate-registries
      preconditions:
        all:
          - key: '{{ request.operation }}'
            operator: Equals
            value: CREATE
          - key: '{{ request.namespace }}'
            operator: AnyNotIn
            value:
              - kube-system
              - cert-manager
              - gatekeeper-system
              - calico-system
      validate:
        message: Images must come from allowed registries.
        pattern:
          spec:
            containers:
              # LOOK HERE
              - image: example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*
  validationFailureAction: enforce
EOF

I included a heredoc of the specific offending document, but obviously I am running this against a big manifest.yaml file. I would like kbld to have a continue on error behaviour where it does as much as it can and produces warnings rather than failures in these situations.

What happened:

This produces the error

kbld: Error:
- Resolving image 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*': Expected valid digest reference, but found 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*', reason: unsupported digest algorithm: * | docker.io/blairdrummond/*

What did you expect:

I would expect a warning rather than an error, with an otherwise modified file. E.g.

kbld: Warning:
- Resolving image 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*': Expected valid digest reference, but found 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*', reason: unsupported digest algorithm: * | docker.io/blairdrummond/*

I'm OK with needing a flag like --no-fail in order to achieve this

Environment:

  • kbld version: 0.36.4, installed from brew, darwin/arm64

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@blairdrummond blairdrummond added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels Mar 7, 2023
@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Apr 17, 2023
@ragaskar
Copy link

I had a similar problem and it took me awhile to find a solution, so dumping this here for others:

I am using tekton, and their git-clone task is part of my yaml configuration.

It includes these lines:

  steps:
    - name: clone
      image: "$(params.gitInitImage)"

Which would normally get populated by Tekton TaskRun parameters.

However, kbld sees the image tag and wants to build it, and then exits with the complaint Resolving image '$(params.gitInitImage)': repository can only contain the characters abcdefghijklmnopqrstuvwxyz0123456789_-./: $(params.gitInitImage)

In my case, I was able to use the overrides configuration option to skip processing this value, e.g., my kbld config looks like this:

---
apiVersion: kbld.k14s.io/v1alpha1
kind: Config
sources:
...
overrides:
  - image: "$(params.gitInitImage)"
    newImage: "$(params.gitInitImage)"
    preresolved: true

This allowed my kbld command to complete successfully.

@joaopapereira joaopapereira reopened this Aug 24, 2023
@joaopapereira joaopapereira removed the stale This issue has had no activity for a while and will be closed soon label Aug 24, 2023
@joaopapereira
Copy link
Member

Sorry for letting this issue stale 😞 I will try to be more proactive here to ensure this doesn't happen.

I believe this is not a bug but a new feature we might want to add to kbld. What kbld should have is a way to say that you want to ignore a particular field from being processed.
Feels like a new configuration like:

---
apiVersion: kbld.k14s.io/v1alpha1
kind: Config
sources:
...
ignore:
  - image: image: "$(params.gitInitImage)"

It would be a better suit for you all's use case.

@joaopapereira joaopapereira added enhancement This issue is a feature request and removed bug This issue describes a defect or unexpected behavior labels Aug 24, 2023
@ragaskar
Copy link

As a way to resolve the issue, I prefer the original poster's solution:

if kbld can't process a field, it could print a warning and just leave the field alone. This is reasonable behavior, because there are only two outcomes here:

  1. the user wants kbld to process the field, but they've made a mistake. The warning alerts them to this.
  2. the user DOES NOT want kbld to process the field. kbld prints a warning that the user can ignore.

You might add a "fail on warning" case for folks who do not expect any warnings and the presence of one indicates a mistake.

I'm not sure the additional configuration would be that useful. The existing config can already be used to "ignore" one-off cases (in the way I've configured it). That said, if the ignore was to accept regex that might be more useful, e.g., you could ignore everything with a $.

@joaopapereira
Copy link
Member

I am convinced. From a backward compatible point of view, I like having a flag that is --no-fail-on-warning to enable this behavior.
The idea here is that we should only fail on a specific error. If we cannot parse the URL to match a valid OCI Image URL, we should raise a warning saying:

kbld: Warning:
- Resolving image 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*': Expected valid digest reference, but found 'example.azurecr.io/* | mcr.microsoft.com/azure-cli:* | docker.io/nginxinc/nginx-unprivileged:* | philpep/imago:*@* | docker.io/blairdrummond/*', reason: invalid URL format

If there is any other type of error like it cannot find the image or there is a registry error we should just error out.
Do yll think this behavior makes sense for you use case?

@joaopapereira
Copy link
Member

Are yll ok with the change I proposed above?

@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Oct 30, 2023
@github-actions github-actions bot closed this as completed Nov 5, 2023
@ragaskar
Copy link

ragaskar commented Nov 5, 2023

Sounds fine to me, but because I solved my problem (with the workaround), I never thought about this again (I haven't done that much more carvel stuff yet, and I guess I have my github notifications set up to mail me on close only .. :) )

I am now too far away from the use cases to say for certain if what you proposed re: error vs warn would have been an issue, I think intuitively, it makes sense, the case I care about is going to be where I want to pass through a token to later configure, so it definitely won't look like anything that can be parsed as a url.

@joaopapereira
Copy link
Member

What do you mean by:

the case I care about is going to be where I want to pass through a token to later configure

@joaopapereira joaopapereira reopened this Nov 6, 2023
@github-actions github-actions bot removed the stale This issue has had no activity for a while and will be closed soon label Nov 7, 2023
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Dec 17, 2023
@ragaskar
Copy link

What do you mean by:

the case I care about is going to be where I want to pass through a token to later configure

By "token", I meant the string that I later want a different template processor to render, like e.g. above $(params.gitInitImage)

What I meant is that any string I use will probably never parse as a URI, so therefore, only warning on non-parse, but erroring on successful parse if there are later failures would work fine for my use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request stale This issue has had no activity for a while and will be closed soon
Projects
Archived in project
Development

No branches or pull requests

3 participants