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

Parameters in constraints are deleted in v3.5.1 #1468

Closed
wouter2397 opened this issue Jul 29, 2021 · 6 comments · Fixed by open-policy-agent/frameworks#130
Closed

Parameters in constraints are deleted in v3.5.1 #1468

wouter2397 opened this issue Jul 29, 2021 · 6 comments · Fixed by open-policy-agent/frameworks#130
Labels
bug Something isn't working

Comments

@wouter2397
Copy link

What steps did you take and what happened:
Upgrade audit and controller deployments to image version v3.5.1
After upgrade we got different policy violations then before upgrade

What did you expect to happen:
Same set of violations before and after upgrade

Anything else you would like to add:
An example of one of our constraints that is having issues:

apiVersion: constraints.gatekeeper.sh/v1beta1
kind: ValidatingServiceType
metadata:
  name: ocp4-066
spec:
  enforcementAction: dryrun
  match:
    kinds:
      - apiGroups:
          - '*'
        kinds:
          - Service
    scope: Namespaced
  parameters:
    serviceType: LoadBalancer

After the upgrade the parameters are completely removed from the constraint.
If I try to re-apply these parameters they are removed immediately.

Do you have any idea why the parameters config is not working anymore in our situation?

Environment:

  • Gatekeeper version: v3.5.1
  • Kubernetes version: v1.20.0+558d959
  • OpenShift version: 4.7.21
@wouter2397 wouter2397 added the bug Something isn't working label Jul 29, 2021
@wouter2397 wouter2397 changed the title Parameters in constraints are removed in v3.5.1 Parameters in constraints are deleted in v3.5.1 Jul 29, 2021
@maxsmythe
Copy link
Contributor

Can you post a copy of the constraint CRD?

kubectl get crd validatingservicetype.constraints.gatekeeper.sh -oyaml should do the trick, I think.

@wouter2397
Copy link
Author

@maxsmythe Thank you for your quick reply.
Below you find a copy of the constraint CRD:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  creationTimestamp: "2021-03-24T09:17:22Z"
  generation: 1645
  labels:
    fluxcd.io/sync-gc-mark: sha256.9LwElZAPgrk_eRsP5anpoZ5FPfhzp-IxBLqlDx2R1Mw
    gatekeeper.sh/constraint: "yes"
  managedFields:
  - apiVersion: apiextensions.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:status:
        f:acceptedNames:
          f:categories: {}
          f:kind: {}
          f:listKind: {}
          f:plural: {}
          f:singular: {}
        f:conditions: {}
    manager: kube-apiserver
    operation: Update
    time: "2021-06-24T13:15:25Z"
  - apiVersion: apiextensions.k8s.io/v1beta1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          .: {}
          f:fluxcd.io/sync-gc-mark: {}
          f:gatekeeper.sh/constraint: {}
        f:ownerReferences:
          .: {}
          k:{"uid":"4a70748a-f13b-443f-8c91-b5bab75f2c69"}:
            .: {}
            f:apiVersion: {}
            f:blockOwnerDeletion: {}
            f:controller: {}
            f:kind: {}
            f:name: {}
            f:uid: {}
      f:spec:
        f:conversion:
          .: {}
          f:strategy: {}
        f:group: {}
        f:names:
          f:categories: {}
          f:kind: {}
          f:listKind: {}
          f:plural: {}
          f:singular: {}
        f:scope: {}
        f:subresources:
          .: {}
          f:status: {}
        f:validation:
          .: {}
          f:openAPIV3Schema:
            .: {}
            f:properties:
              .: {}
              f:metadata:
                .: {}
                f:properties:
                  .: {}
                  f:name:
                    .: {}
                    f:maxLength: {}
                    f:type: {}
              f:spec:
                .: {}
                f:properties:
                  .: {}
                  f:enforcementAction:
                    .: {}
                    f:type: {}
                  f:match:
                    .: {}
                    f:properties:
                      .: {}
                      f:excludedNamespaces:
                        .: {}
                        f:items: {}
                        f:type: {}
                      f:kinds:
                        .: {}
                        f:type: {}
                      f:labelSelector:
                        .: {}
                        f:properties:
                          .: {}
                          f:matchExpressions:
                            .: {}
                            f:type: {}
                      f:namespaceSelector:
                        .: {}
                        f:properties:
                          .: {}
                          f:matchExpressions:
                            .: {}
                            f:type: {}
                      f:namespaces:
                        .: {}
                        f:items: {}
                        f:type: {}
                      f:scope:
                        .: {}
                        f:enum: {}
                        f:type: {}
        f:version: {}
        f:versions: {}
      f:status:
        f:storedVersions: {}
    manager: gatekeeper
    operation: Update
    time: "2021-07-29T14:01:30Z"
  - apiVersion: apiextensions.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:spec:
        f:versions: {}
    manager: gatekeeper
    operation: Update
    time: "2021-07-29T14:01:31Z"
  name: validatingservicetype.constraints.gatekeeper.sh
  ownerReferences:
  - apiVersion: templates.gatekeeper.sh/v1beta1
    blockOwnerDeletion: true
    controller: true
    kind: ConstraintTemplate
    name: validatingservicetype
    uid: 4a70748a-f13b-443f-8c91-b5bab75f2c69
  resourceVersion: "378773343"
  selfLink: /apis/apiextensions.k8s.io/v1/customresourcedefinitions/validatingservicetype.constraints.gatekeeper.sh
  uid: f996e93a-5fa2-4621-b6eb-761b7464eaff
spec:
  conversion:
    strategy: None
  group: constraints.gatekeeper.sh
  names:
    categories:
    - constraint
    - constraints
    kind: ValidatingServiceType
    listKind: ValidatingServiceTypeList
    plural: validatingservicetype
    singular: validatingservicetype
  scope: Cluster
  versions:
  - name: v1beta1
    schema:
      openAPIV3Schema:
        properties:
          metadata:
            properties:
              name:
                maxLength: 63
                type: string
            type: object
          spec:
            properties:
              enforcementAction:
                type: string
              match:
                properties:
                  excludedNamespaces:
                    items:
                      type: string
                    type: array
                  kinds:
                    items:
                      properties:
                        apiGroups:
                          items:
                            nullable: true
                            type: string
                          type: array
                        kinds:
                          items:
                            nullable: true
                            type: string
                          type: array
                      type: object
                    type: array
                  labelSelector:
                    properties:
                      matchExpressions:
                        items:
                          properties:
                            key:
                              type: string
                            operator:
                              enum:
                              - In
                              - NotIn
                              - Exists
                              - DoesNotExist
                              type: string
                            values:
                              items:
                                type: string
                              type: array
                          type: object
                        type: array
                      matchLabels:
                        additionalProperties:
                          type: string
                        type: object
                        x-kubernetes-preserve-unknown-fields: true
                    type: object
                  namespaceSelector:
                    properties:
                      matchExpressions:
                        items:
                          properties:
                            key:
                              type: string
                            operator:
                              enum:
                              - In
                              - NotIn
                              - Exists
                              - DoesNotExist
                              type: string
                            values:
                              items:
                                type: string
                              type: array
                          type: object
                        type: array
                      matchLabels:
                        additionalProperties:
                          type: string
                        type: object
                        x-kubernetes-preserve-unknown-fields: true
                    type: object
                  namespaces:
                    items:
                      type: string
                    type: array
                  scope:
                    enum:
                    - '*'
                    - Cluster
                    - Namespaced
                    type: string
                type: object
            type: object
          status:
            x-kubernetes-preserve-unknown-fields: true
        type: object
    served: true
    storage: true
    subresources:
      status: {}
  - name: v1alpha1
    schema:
      openAPIV3Schema:
        properties:
          metadata:
            properties:
              name:
                maxLength: 63
                type: string
            type: object
          spec:
            properties:
              enforcementAction:
                type: string
              match:
                properties:
                  excludedNamespaces:
                    items:
                      type: string
                    type: array
                  kinds:
                    items:
                      properties:
                        apiGroups:
                          items:
                            nullable: true
                            type: string
                          type: array
                        kinds:
                          items:
                            nullable: true
                            type: string
                          type: array
                      type: object
                    type: array
                  labelSelector:
                    properties:
                      matchExpressions:
                        items:
                          properties:
                            key:
                              type: string
                            operator:
                              enum:
                              - In
                              - NotIn
                              - Exists
                              - DoesNotExist
                              type: string
                            values:
                              items:
                                type: string
                              type: array
                          type: object
                        type: array
                      matchLabels:
                        additionalProperties:
                          type: string
                        type: object
                        x-kubernetes-preserve-unknown-fields: true
                    type: object
                  namespaceSelector:
                    properties:
                      matchExpressions:
                        items:
                          properties:
                            key:
                              type: string
                            operator:
                              enum:
                              - In
                              - NotIn
                              - Exists
                              - DoesNotExist
                              type: string
                            values:
                              items:
                                type: string
                              type: array
                          type: object
                        type: array
                      matchLabels:
                        additionalProperties:
                          type: string
                        type: object
                        x-kubernetes-preserve-unknown-fields: true
                    type: object
                  namespaces:
                    items:
                      type: string
                    type: array
                  scope:
                    enum:
                    - '*'
                    - Cluster
                    - Namespaced
                    type: string
                type: object
            type: object
          status:
            x-kubernetes-preserve-unknown-fields: true
        type: object
    served: true
    storage: false
    subresources:
      status: {}
status:
  acceptedNames:
    categories:
    - constraint
    - constraints
    kind: ValidatingServiceType
    listKind: ValidatingServiceTypeList
    plural: validatingservicetype
    singular: validatingservicetype
  conditions:
  - lastTransitionTime: "2021-03-24T09:17:22Z"
    message: no conflicts found
    reason: NoConflicts
    status: "True"
    type: NamesAccepted
  - lastTransitionTime: "2021-03-24T09:17:22Z"
    message: the initial names have been accepted
    reason: InitialNamesAccepted
    status: "True"
    type: Established
  storedVersions:
  - v1beta1

@maxsmythe
Copy link
Contributor

Thank you for posting this!

I'm guessing the parameters schema for your constraint template is empty for these constraint templates? It's the part that looks like this:

validation:
# Schema for the `parameters` field
openAPIV3Schema:
properties:
repos:
type: array
items:
type: string

I think what's happening is that we aren't injecting an empty parameters field with x-kubernetes-preserve-unknown-fields set to true, which is causing the API server to prune the unrecognized field. This is a bug in Gatekeeper, as that backwards compatibility should have been preserved.

As a mitigation, you can add parameters schemas to your constraint templates (which would be a good thing to do generally, for type safety), or rollback. Though the underlying issue should definitely be addressed.

maxsmythe added a commit to maxsmythe/frameworks that referenced this issue Jul 30, 2021
To handle legacy (pre-v1) CRDs, we inject
x-kubernetes-preserve-unknown-fields
in order to preserve the legacy behavior
that unknown fields were not pruned by the API
server.

Unfortunately, we are not handling the case
where no parameters are provided, which currently
means that parameters will be stripped from users' CRs.

This fixes that.

Fixes: open-policy-agent/gatekeeper#1468

Signed-off-by: Max Smythe <[email protected]>
maxsmythe added a commit to maxsmythe/frameworks that referenced this issue Jul 30, 2021
To handle legacy (pre-v1) CRDs, we inject
x-kubernetes-preserve-unknown-fields
in order to preserve the legacy behavior
that unknown fields were not pruned by the API
server.

Unfortunately, we are not handling the case
where no parameters are provided, which currently
means that parameters will be stripped from users' CRs.

This fixes that.

Fixes: open-policy-agent/gatekeeper#1468

Signed-off-by: Max Smythe <[email protected]>
maxsmythe added a commit to maxsmythe/frameworks that referenced this issue Jul 30, 2021
To handle legacy (pre-v1) CRDs, we inject
x-kubernetes-preserve-unknown-fields
in order to preserve the legacy behavior
that unknown fields were not pruned by the API
server.

Unfortunately, we are not handling the case
where no parameters are provided, which currently
means that parameters will be stripped from users' CRs.

This fixes that.

Fixes: open-policy-agent/gatekeeper#1468

Signed-off-by: Max Smythe <[email protected]>
maxsmythe added a commit to maxsmythe/frameworks that referenced this issue Jul 30, 2021
To handle legacy (pre-v1) CRDs, we inject
x-kubernetes-preserve-unknown-fields
in order to preserve the legacy behavior
that unknown fields were not pruned by the API
server.

Unfortunately, we are not handling the case
where no parameters are provided, which currently
means that parameters will be stripped from users' CRs.

This fixes that.

Fixes: open-policy-agent/gatekeeper#1468

Signed-off-by: Max Smythe <[email protected]>
@wouter2397
Copy link
Author

@maxsmythe Thank you for clarifying the situation.
The proposed change indeed fixed our problem and all policies are working well!

We do frequently update our gatekeeper instances and are also scanning the release notes.
Nevertheless we did miss this change and or best-practice.

Do you have any advice for us on how to notice these kinds of changes/best-practices so we can follow them right away?

@maxsmythe
Copy link
Contributor

To be honest, this is a failing on our part. While trying to address Kubernetes' v1beta1 CRD deprecation, we thought we had maintained backwards compatibility but missed this particular edge case.

Particularly with respect to user's policies, our goal is to have everything that worked before an upgrade work in the same way after an upgrade. If for some reason this is not possible, it would be on us to mention it in the release notes. In this case, your breakage is a genuine bug on our part.

WRT best practices (which, IMO, includes providing an explicit parameter schema to help prevent things like typos)... I think we should be doing a better job with the authorship experience to inform people of best practices, which is something we're working on. Aside from looking at the templates in gatekeeper-library, I'm not sure what documentation we currently have.

One thing changing with v1 ConstraintTemplates is that unknown parameters will be rejected by default with an error returned by Kubectl (at least for my version of Kubectl on my K8s cluster... v1.21 I think I tested this on?). Hopefully the faster failure, more explicit messaging, and the fact that this is occurring on a new constraint template version (previous versions will continue to behave as they have before), will make the messaging a bit stronger.

TL;DR

I'm sorry this caused you pain. Let us know if the path forward makes sense to you and if there are any other changes you may find helpful.

@ritazh
Copy link
Member

ritazh commented Aug 5, 2021

@wouter2397 We have just released https://github.com/open-policy-agent/gatekeeper/releases/tag/v3.5.2 which includes the fix for this bug. Please give it a try and let us know if you have issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants