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

Support manifest variable substitution $(...) in validate message field #1506

Closed
goffinf opened this issue Jan 29, 2021 · 17 comments · Fixed by #1714
Closed

Support manifest variable substitution $(...) in validate message field #1506

goffinf opened this issue Jan 29, 2021 · 17 comments · Fixed by #1714
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@goffinf
Copy link

goffinf commented Jan 29, 2021

Kubernetes version:

  • 1.20

Kyverno version:

  • v1.3.1

Can someone confirm whether this is possible or not, or if I am not implementing it correctly, please (perhaps variable substitution is not supported in message fields ?)

Example: A policy to validate image registries - should return which registries are allowed in the message.

Validation rule: (note the message with the manifest reference)

...
    validate:
      message: "Attempt to pull an image from a non-approved registry. Approved registries are: $(./../pattern/spec/containers/image)"
      pattern:
        spec:
          containers:
          - name: "*"
            image: "registry.localhost:5000/* | myuser.jfrog.io/docker-local/* | docker.io/*"

Expected Behaviour (on fail of the policy condition):

Error from server: error when creating "validate-registries-deploy-fail.yml": admission webhook "validate.kyverno.svc" denied the request:

resource Deployment/default/validate-registries-fail was blocked due to the following policies

validate-registries:
  autogen-validate-registries: 'validation error: Attempt to pull an image from a non-approved registry. Approved registries are: registry.localhost:5000/* | myuser.jfrog.io/docker-local/* | docker.io/*. Rule autogen-validate-registries failed at path /spec/template/spec/containers/0/image/'

Actual Behaviour:

Error from server: error when creating "validate-registries-deploy-fail.yml": admission webhook "validate.kyverno.svc" denied the request:

resource Deployment/default/validate-registries-fail was blocked due to the following policies

validate-registries:
  autogen-validate-registries: 'validation error: Attempt to pull an image from a non-approved registry. Approved registries are: $(./../pattern/spec/containers/image). Rule autogen-validate-registries failed at path /spec/template/spec/containers/0/image/'

Regards

Fraser

@chipzoller
Copy link
Member

Variable expansion is supported in the message field as demonstrated here, but it may not be supported on manifest lookups. Can you supply your full policy, please?

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

Sure:


apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: validate-registries
  labels:
    app: kyverno
  annotations:
    policies.kyverno.io/category: Compliance
    policies.kyverno.io/description: Ensure only CISO agreed registries are specified
spec:
  validationFailureAction: enforce
  rules:
  - name: validate-registries
    match:
      resources:
        kinds:
        - Pod
    exclude:
      resources:
        namespaces:
        - kube-system
    validate:
      message: "Attempt to pull an image from a non-approved registry. Approved registries are: $(./../pattern/spec/containers/image)"
      pattern:
        spec:
          containers:
          - name: "*"
            image: "registry.localhost:5000/* | myuser.jfrog.io/docker-local/* | docker.io/*"

@chipzoller
Copy link
Member

Yes, it does appear the message field isn't resolving the variable from the manifest lookup, even when specifying the index of the entry in the containers object. @realshuting to confirm.

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

@chipzoller / @realshuting

Thx for checking.

This is an early look at kyverno as an alternative to OPA Gatekeeper. I believe our K8s Platform team would find it much easier to maintain policy and compliance.

This seems like a reasonable use-case. Would you advise I raise this as a feature request if the currently observed behaviour is confirmed ?

@chipzoller
Copy link
Member

A couple comments.

  1. I think it's definitely a good use case, but have you also considered storing these allowed registries in a ConfigMap rather than in the policy itself? Kyverno can refer to them there, which means should your list change, you have but to update the ConfigMap rather than the policy definition. This should also solve the problem of variable substitution in the message field.
  2. If this is a confirmed but, the issue here is probably sufficient unless @realshuting feels differently.

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

iro (1): Yes I had considered that. My initial reaction was that since the data that I needed was already present in the policy definition, I would prefer not to have to maintain it in a separate resource and have to manage its life-cycle also. I suppose that would work if I could also use the ConfigMap as the source for the image value in the pattern as well as in the message (I haven't tried that - but I will now that you have mentioned it).

I think in the general case it would be useful to be able to use a manifest lookup in a message field, so I think the use-case remains.

iro: (2): Ok, thanks

@chipzoller
Copy link
Member

Yes, agree, this seems like either a gap or a bug. You should have the choice of where to store and retrieve your allowed registries whether that be in the policy definition itself or an external ConfigMap.

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

I suppose another potential downside of using a ConfigMap here is that they are namespace scoped and in this case I am using a cluster scoped policy (whilst replicating the ConfigMap to all namespaces (and all clusters) is not that big a deal, I would prefer not to have to).

@chipzoller
Copy link
Member

That shouldn't be a concern as Kyverno has the ability to reference a ConfigMap anywhere by defining a context which includes the Namespace location. This is illustrated in the docs.

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

Ah, thats cool. I really like this product :-)

@chipzoller
Copy link
Member

That said, managing one resource is simpler than managing two. And so for that reason, it should be entirely possible to confine all your configurations in the ClusterPolicy.

@goffinf
Copy link
Author

goffinf commented Jan 29, 2021

+1

@realshuting realshuting added the enhancement New feature or request label Jan 29, 2021
@realshuting realshuting added this to the Kyverno Release 1.3.3 milestone Jan 29, 2021
@realshuting realshuting added the good first issue Good for newcomers label Jan 29, 2021
@realshuting
Copy link
Member

I can confirm this is not supported in message but pattern, we can use this issue to track the progress.

@JimBugwadia JimBugwadia changed the title Manifest variable substitution in validate message field Support manifest variable substitution $(...) in validate message field Feb 14, 2021
@JimBugwadia
Copy link
Member

We need to handle inline variables everywhere JMESPath variables are supported. Ideally we can combine the substitution logic.

@kacejot
Copy link
Contributor

kacejot commented Mar 12, 2021

I'm testing my fix right now and I found next test failing.
Here is the policy:

---
apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: substitute-variable
spec:
  rules:
  - name: test-path-not-exist
    match:
      resources:
        kinds:
        - Deployment
    validate:
      anyPattern:
      - spec:
          template:
            spec:
              containers:
              - name: "{{request.object.metadata.name1}}*"
      - spec:
          template:
            spec:
              containers:
              - name: "{{request.object.metadata.name}}*"

Resource:

---
apiVersion: v1
kind: Deployment
metadata:
  name: test
spec:
  template:
    spec:
      containers:
      - name: test-pod
        image: nginxinc/nginx-unprivileged

Validation here fails because I have changed variable/reference substitution logic. Now substitution is done for entire rule at once instead of just pattern/anyPattern/overlay. In this case validation fails even before actually validation process begun, because of substitution failure. Should I leave this logic? Or should it be as earlier?

kacejot added a commit to kacejot/kyverno that referenced this issue Mar 12, 2021
@kacejot
Copy link
Contributor

kacejot commented Mar 12, 2021

I left new logic when variable substitution has higher priority. It is mostly like code compilation and we throw compilation error, if var is unbound. I will create a PR soon.

kacejot added a commit to kacejot/kyverno that referenced this issue Mar 15, 2021
…st pattern/overlay

Signed-off-by: Max Goncharenko <kacejot@fex.net>
kacejot added a commit to kacejot/kyverno that referenced this issue Mar 16, 2021
…st pattern/overlay

Signed-off-by: Max Goncharenko <kacejot@fex.net>
@kacejot
Copy link
Contributor

kacejot commented Mar 19, 2021

Status: issue is fixed, waiting for review and testing

JimBugwadia added a commit that referenced this issue Mar 26, 2021
Bug Fix: #1506 issue; Resolve path reference in entire rule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants