kubewarden / kwctl

Go-to CLI tool for Kubewarden users
https://kubewarden.io
Apache License 2.0
73 stars 15 forks source link

Generated JSONPatch does not Mutate Kubernetes Resource #48

Closed darren-bell-nanthealth closed 3 years ago

darren-bell-nanthealth commented 3 years ago

While writing a policy to mutate an ingress resource, I noticed that the resource was not being mutated after it was being applied to the cluster. To further test I used the kwctl to inspect the patch that KW was generating.

Steps to Reproduce

The following input file was used as test data test_data/6_full_admission_review.json

{
  "apiVersion": "admission.k8s.io/v1",
  "kind": "AdmissionReview",
  "request": {
    "dryRun": false,
    "kind": {
      "kind": "Ingress",
      "group": "apps",
      "version": "v1"
    },
    "name": "ingresses",
    "namespace": "default",
    "object": {
      "apiVersion": "networking.k8s.io/v1beta1",
      "kind": "Ingress",
      "metadata": {
        "annotations": {
          "kubernetes.io/ingress.class": "public",
          "nginx.ingress.kubernetes.io/affinity": "cookie",
          "nginx.ingress.kubernetes.io/ssl-redirect": "false",
          "nginx.ingress.kubernetes.io/rewrite-target": "/"
        },
        "labels": {
          "app.kubernetes.io/instance": "kelp",
          "app.kubernetes.io/managed-by": "Tiller",
          "app.kubernetes.io/name": "test-ingress",
          "app.kubernetes.io/part-of": "kubernetes-tools",
          "app.kubernetes.io/version": "0.0.3",
          "helm.sh/chart": "kelp-0.0.3",
          "monitoring.nanthealth.com/application": "kelp",
          "monitoring.nanthealth.com/owner": "Kubernetes-Team",
          "monitoring.nanthealth.com/service": "platform"
        },
        "name": "pod1-example",
        "namespace": "default"
      },
      "spec": {
        "rules": [
          {
            "host": "dtkt.navimedix.com",
            "http": {
              "paths": [
                {
                  "backend": {
                    "serviceName": "kelp-kelp-svc",
                    "servicePort": "http"
                  },
                  "path": "/kelp"
                }
              ]
            }
          },
          {
            "host": "dtkt1.navimedix.com",
            "http": {
              "paths": [
                {
                  "backend": {
                    "serviceName": "kelp-kelp-svc",
                    "servicePort": "http"
                  },
                  "path": "/kelp"
                }
              ]
            }
          }
        ]
      }
    },
    "operation": "CREATE",
    "requestKind": {
      "kind": "Ingress",
      "group": "apps",
      "version": "v1"
    },
    "requestResource": {
      "group": "apps",
      "resource": "ingresses",
      "version": "v1"
    },
    "resource": {
      "group": "apps",
      "resource": "ingresses",
      "version": "v1"
    },
    "uid": "123",
    "userInfo": {
      "groups": [
        "system:masters",
        "system:authenticated"
      ],
      "uid": "123",
      "username": "kubernetes-admin"
    }
  }
}
kwctl run --settings-json '{"secret":"supersecret"}' --request-path test_data/6_full_admission_review.json registry://ghcr.io/darren-bell-nanthealth/kubewarden-policies/istio-ingress-mutation:latest 

Which produced

{"uid":"123","allowed":true,"patchType":"JSONPatch","patch":"W3sib3AiOiJyZXBsYWNlIiwicGF0aCI6Ii8iLCJ2YWx1ZSI6IntcImFwaVZlcnNpb25cIjpcIm5ldHdvcmtpbmcuazhzLmlvL3YxYmV0YTFcIixcImtpbmRcIjpcIkluZ3Jlc3NcIixcIm1ldGFkYXRhXCI6e1wiYW5ub3RhdGlvbnNcIjp7XCJrdWJlcm5ldGVzLmlvL2luZ3Jlc3MuY2xhc3NcIjpcInB1YmxpY1wiLFwia3ViZXdhcmR
lbi5wb2xpY3kuaW5ncmVzcy9pbnNwZWN0ZWRcIjpcInRydWVcIixcIm5naW54LmluZ3Jlc3Mua3ViZXJuZXRlcy5pby9hZmZpbml0eVwiOlwiY29va2llXCIsXCJuZ2lueC5pbmdyZXNzLmt1YmVybmV0ZXMuaW8vcmV3cml0ZS10YXJnZXRcIjpcIi9cIixcIm5naW54LmluZ3Jlc3Mua3ViZXJuZXRlcy5pby9zZXJ2aWNlLXVwc3RyZWFtXCI6XCJ0cnVlXCIsXCJuZ2lueC5pbmdyZXNzLmt1YmVybmV0ZXMuaW8vc3N
sLXJlZGlyZWN0XCI6XCJmYWxzZVwiLFwibmdpbnguaW5ncmVzcy5rdWJlcm5ldGVzLmlvL3Vwc3RyZWFtLXZob3N0XCI6XCJrZWxwLWtlbHAtc3ZjLmRlZmF1bHQuc3ZjLmNsdXN0ZXIubG9jYWxcIn0sXCJsYWJlbHNcIjp7XCJhcHAua3ViZXJuZXRlcy5pby9pbnN0YW5jZVwiOlwia2VscFwiLFwiYXBwLmt1YmVybmV0ZXMuaW8vbWFuYWdlZC1ieVwiOlwiVGlsbGVyXCIsXCJhcHAua3ViZXJuZXRlcy5pby9uYW1
lXCI6XCJ0ZXN0LWluZ3Jlc3NcIixcImFwcC5rdWJlcm5ldGVzLmlvL3BhcnQtb2ZcIjpcImt1YmVybmV0ZXMtdG9vbHNcIixcImFwcC5rdWJlcm5ldGVzLmlvL3ZlcnNpb25cIjpcIjAuMC4zXCIsXCJoZWxtLnNoL2NoYXJ0XCI6XCJrZWxwLTAuMC4zXCIsXCJtb25pdG9yaW5nLm5hbnRoZWFsdGguY29tL2FwcGxpY2F0aW9uXCI6XCJrZWxwXCIsXCJtb25pdG9yaW5nLm5hbnRoZWFsdGguY29tL293bmVyXCI6XCJ
LdWJlcm5ldGVzLVRlYW1cIixcIm1vbml0b3JpbmcubmFudGhlYWx0aC5jb20vc2VydmljZVwiOlwicGxhdGZvcm1cIn0sXCJuYW1lXCI6XCJwb2QxLWV4YW1wbGVcIixcIm5hbWVzcGFjZVwiOlwiZGVmYXVsdFwifSxcInNwZWNcIjp7XCJydWxlc1wiOlt7XCJob3N0XCI6XCJkdGt0Lm5hdmltZWRpeC5jb21cIixcImh0dHBcIjp7XCJwYXRoc1wiOlt7XCJiYWNrZW5kXCI6e1wic2VydmljZU5hbWVcIjpcImtlbHA
ta2VscC1zdmNcIixcInNlcnZpY2VQb3J0XCI6XCJodHRwXCJ9LFwicGF0aFwiOlwiL2tlbHBcIn1dfX0se1wiaG9zdFwiOlwiZHRrdDEubmF2aW1lZGl4LmNvbVwiLFwiaHR0cFwiOntcInBhdGhzXCI6W3tcImJhY2tlbmRcIjp7XCJzZXJ2aWNlTmFtZVwiOlwia2VscC1rZWxwLXN2Y1wiLFwic2VydmljZVBvcnRcIjpcImh0dHBcIn0sXCJwYXRoXCI6XCIva2VscFwifV19fV0sXCJ0bHNcIjpbe1wiaG9zdHNcIjp
bXCJkdGt0Lm5hdmltZWRpeC5jb21cIixcImR0a3QxLm5hdmltZWRpeC5jb21cIl0sXCJzZWNyZXROYW1lXCI6XCJleHRlcm5hbC1pbmdyZXNzLXNlY3JldFwifV19fSJ9XQ=="}

The 64bit decoded value being:

[{"op":"replace","path":"/","value":"{\"apiVersion\":\"networking.k8s.io/v1beta1\",\"kind\":\"Ingress\",\"metadata\":{\"annotations\":{\"kubernetes.io/ingress.class\":\"public\",\"kubewarden.policy.ingress/inspected\":\"true\",\"nginx.ingress.kubernetes.io/affinity\":\"cookie\",\"nginx.ingress.kubernetes.io/rewrite-target\":\"/\",\"nginx.ingress.kubernetes.io/service-upstream\":\"true\",\"nginx.ingress.kubernetes.io/ssl-redirect\":\"false\",\"nginx.ingress.kubernetes.io/upstream-vhost\":\"kelp-kelp-svc.default.svc.cluster.local\"},\"labels\":{\"app.kubernetes.io/instance\":\"kelp\",\"app.kubernetes.io/managed-by\":\"Tiller\",\"app.kubernetes.io/name\":\"test-ingress\",\"app.kubernetes.io/part-of\":\"kubernetes-tools\",\"app.kubernetes.io/version\":\"0.0.3\",\"helm.sh/chart\":\"kelp-0.0.3\",\"monitoring.nanthealth.com/application\":\"kelp\",\"monitoring.nanthealth.com/owner\":\"Kubernetes-Team\",\"monitoring.nanthealth.com/service\":\"platform\"},\"name\":\"pod1-example\",\"namespace\":\"default\"},\"spec\":{\"rules\":[{\"host\":\"dtkt.navimedix.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"kelp-kelp-svc\",\"servicePort\":\"http\"},\"path\":\"/kelp\"}]}},{\"host\":\"dtkt1.navimedix.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"kelp-kelp-svc\",\"servicePort\":\"http\"},\"path\":\"/kelp\"}]}}],\"tls\":[{\"hosts\":[\"dtkt.navimedix.com\",\"dtkt1.navimedix.com\"],\"secretName\":\"external-ingress-secret\"}]}}"}]

However where I try to manually apply this JSONPatch

❯ kubectl patch ingress -n kubewarden pod1-example --type=json -p='[{"op":"replace","path":"/","value":"{\"apiVersion\":\"networking.k8s.io/v1beta1\",\"kind\":\"Ingress\",\"metadata\":{\"annotations\":{\"kubernetes.io/ingress.class\":\"public\",\"kubewarden.policy.ingress/inspected\":\"true\",\"nginx.ingress.ku
bernetes.io/affinity\":\"cookie\",\"nginx.ingress.kubernetes.io/rewrite-target\":\"/\",\"nginx.ingress.kubernetes.io/service-upstream\":\"true\",\"nginx.ingress.kubernetes.io/ssl-redirect\":\"false\",\"nginx.ingress.kubernetes.io/upstream-vhost\":\"kelp-kelp-svc.default.svc.cluster.local\"},\"labels\":{\"app.ku
bernetes.io/instance\":\"kelp\",\"app.kubernetes.io/managed-by\":\"Tiller\",\"app.kubernetes.io/name\":\"test-ingress\",\"app.kubernetes.io/part-of\":\"kubernetes-tools\",\"app.kubernetes.io/version\":\"0.0.3\",\"helm.sh/chart\":\"kelp-0.0.3\",\"monitoring.nanthealth.com/application\":\"kelp\",\"monitoring.nant
health.com/owner\":\"Kubernetes-Team\",\"monitoring.nanthealth.com/service\":\"platform\"},\"name\":\"pod1-example\",\"namespace\":\"default\"},\"spec\":{\"rules\":[{\"host\":\"dtkt.navimedix.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"kelp-kelp-svc\",\"servicePort\":\"http\"},\"path\":\"/kelp\"}
]}},{\"host\":\"dtkt1.navimedix.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"kelp-kelp-svc\",\"servicePort\":\"http\"},\"path\":\"/kelp\"}]}}],\"tls\":[{\"hosts\":[\"dtkt.navimedix.com\",\"dtkt1.navimedix.com\"],\"secretName\":\"external-ingress-secret\"}]}}"}]'

I get the following response back from the cluster

ingress.extensions/pod1-example patched (no change)

Context (Environment)

Kubernetes - 1.19.4 kwctl 0.1.6

Possible Solution

I believe the issues lie with trying to replace the entire resource using the "/" path. I tried replacing this with nothing i.e. "", and the request came back as invalid.
Essentially I don't think that K8s will allow you to change things in the metadata such as name, namespace, etc as this could make things very unpredictable. Such behaviour would be feasible if the whole resource document can be replaced.
Possibly a better approach would be a surgical approach to generating the patches required. Instead of replacing the entire document with a single patch. It may be better to generate several patches. An example of such an implementation taking a current and desired state, then generating many patches, can be found here https://json-patch-builder-online.github.io/. I believe that it is using the following library to generate the patches https://github.com/Starcounter-Jack/JSON-Patch

ereslibre commented 3 years ago

Thank you for your report @darren-bell-nanthealth!

We are producing this kind of "surgical" patches already. The problem in your case seems to be the usage of an outdated rust SDK. I tried to push your policy dependencies to use policy-sdk-rust 0.2.3 and did a related change:

diff --git a/Cargo.toml b/Cargo.toml
index d694974..85cca39 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,7 +11,7 @@ crate-type = ["cdylib"]

 [dependencies]
 k8s-openapi = { version = "0.11.0", features = ["v1_20"] }
-kubewarden-policy-sdk = "0.1.0"
+kubewarden-policy-sdk = "0.2.3"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 wapc-guest = "0.4.0"
diff --git a/src/lib.rs b/src/lib.rs
index 88ec3b1..762b901 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -33,7 +33,7 @@ fn validate(payload: &[u8]) -> CallResult {
             // NOTE 3
             let mutated_object =
                 serde_json::to_value(mutated_ingress_with_annotations_and_tls).unwrap();
-            mutate_request(&mutated_object)
+            mutate_request(mutated_object)
         }
         Err(_) => {
             // We were forwarded a request we cannot unmarshal or

Now, I execute your policy after building with make:

ereslibre@desktop ~/kubewarden-istio-ingress-mutation (main)> kwctl run --settings-json '{"secret":"supersecret"}' --request-path test_data/6_full_admission_review.json target/wasm32-unknown-unknown/release/istio_ingress_mutation.wasm | jq -r .patch | base64 -d | jq
[
  {
    "op": "add",
    "path": "/metadata/annotations/kubewarden.policy.ingress~1inspected",
    "value": "true"
  },
  {
    "op": "add",
    "path": "/metadata/annotations/nginx.ingress.kubernetes.io~1service-upstream",
    "value": "true"
  },
  {
    "op": "add",
    "path": "/metadata/annotations/nginx.ingress.kubernetes.io~1upstream-vhost",
    "value": "kelp-kelp-svc.default.svc.cluster.local"
  },
  {
    "op": "add",
    "path": "/spec/tls",
    "value": [
      {
        "hosts": [
          "dtkt.navimedix.com",
          "dtkt1.navimedix.com"
        ],
        "secretName": "external-ingress-secret"
      }
    ]
  }
]

And this should work fine.

Just a comment regarding the way you tried to reproduce the issue: take into account that when mutation webhooks are executed, the object is not yet stored in etcd, so not the same rules apply if you use kubectl patch, that works against a persisted object that is enforced coherence by the API server, and so, many fields are immutable.

Thank you for your report, please close it if this solved your problem, and thank you for the amazing feedback :clap:

ereslibre commented 3 years ago

Closing for now, please reopen if you still hit this issue.

darren-bell-nanthealth commented 3 years ago

@ereslibre I appreciate the time and effort you have taken to guide me through this. Thank you very much! I was going to suggest that you update the rust template. But I see that has already been done. Must have been unlucky timing on my part.