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

Generate an additive JSON Patch instead of overwriting containers #175

Closed
wants to merge 1 commit into from

Conversation

iamnoah
Copy link

@iamnoah iamnoah commented Feb 13, 2023

Issue #, if available: 165

Description of changes:

The problem in #165 is that the code is serializing the k8s.io structs as is, with an 'add' operation that effectively replaces the Container in the PodSpec. This works until fields are added, like grpc, at which point this webhook has to be updated, or it will drop the new fields.

Instead of serializing the entire container struct, this uses jsonpatch to create a diff from the modifications, and generated a targeted patch that only adds and modifies the things it needs to. Unknown fields will not be dropped.

I've not updated the test cases as they are hardcoded to expect a specific patch (that will drop unknown fields.) TBH, that approach needs to be rethought.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The problem in #165 is that the code is serializing the k8s.io structs as is, with an 'add' operation that effectively replaces the Container in the PodSpec. This works until fields are added, like grpc, at which point this webhook has to be updated, or it will drop the new fields.

Instead of serializing the entire container struct, this uses jsonpatch to create a diff from the modifications, and generated a targeted patch that only adds and modifies the things it needs to. Unknown fields will not be dropped.

I've not updated the test cases as they are hardcoded to expect a specific patch (that will drop unknown fields.) TBH, that approach needs to be rethought.
@iamnoah iamnoah requested a review from a team as a code owner February 13, 2023 19:52
@iamnoah
Copy link
Author

iamnoah commented Feb 14, 2023

To be clear, the goal of this PR is to change the resulting patch. Here is an example diff from the unit tests:

--- want
+++ got
@@ -21,29 +21,26 @@
   },
   {
     "op": "add",
-    "path": "/spec/containers",
+    "path": "/spec/containers/0/volumeMounts",
     "value": [
       {
-        "name": "balajilovesoreos",
-        "image": "amazonlinux",
-        "env": [
+        "mountPath": "/var/run/secrets/eks.amazonaws.com/serviceaccount",
+        "name": "aws-iam-token",
+        "readOnly": true
+      }
+    ]
+  },
   {
+    "op": "add",
+    "path": "/spec/containers/0/env",
+    "value": [
+      {
             "name": "AWS_ROLE_ARN",
             "value": "arn:aws:iam::111122223333:role/s3-reader"
           },
           {
             "name": "AWS_WEB_IDENTITY_TOKEN_FILE",
             "value": "C:\\var\\run\\secrets\\eks.amazonaws.com\\serviceaccount\\token"
-          }
-        ],
-        "resources": {},
-        "volumeMounts": [
-          {
-            "name": "aws-iam-token",
-            "readOnly": true,
-            "mountPath": "/var/run/secrets/eks.amazonaws.com/serviceaccount"
-          }
-        ]
       }
     ]
   }

The new patch is more targeted but it adds the same values to env and mounts. The omission of the name and image are a good thing, because that means the rest of the container spec is unchanged.

@nckturner nckturner closed this Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants