diff --git a/README.md b/README.md index c1cc690..09798ac 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,6 @@ In certain cases there is a need to filter out certain fields when the patch gen To help in these scenarios there are the following options to be used when calculating diffs: - `IgnoreStatusFields` - `IgnoreVolumeClaimTemplateTypeMetaAndStatus` -- `IgnorePDBSelector` - `IgnoreField("field-name-to-ignore")` Example: @@ -88,11 +87,6 @@ This CalculateOptions removes status fields from both objects before comparing. This CalculateOption clears volumeClaimTemplate fields from both objects before comparing (applies to statefulsets). -#### IgnorePdbSelector - -Checks `selector` fields of PDB objects before comparing and removes them if they match. `reflect.DeepEquals` is used for the equality check. -This is required because map fields using `patchStrategy:"replace"` will always diff regardless if they are otherwise equal. - #### IgnoreField("field-name-to-ignore") This CalculateOption removes the field provided (as a string) in the call before comparing them. A common usage might be to remove the metadata fields by using the `IgnoreField("metadata")` option. diff --git a/docs/legacy.md b/docs/legacy.md index 9e07ced..f8e21c1 100644 --- a/docs/legacy.md +++ b/docs/legacy.md @@ -20,7 +20,6 @@ The legacy version was last available with the 0.1.1 version. - PersistentVolumeClaim - Service - ServiceAccount -- PodDisruptionBudget - Unstructured - Node @@ -33,11 +32,11 @@ objectMatcher.Match(e.ObjectOld, e.ObjectNew) ### The idea -There are existing libraries in the wild that can calculate a patch by giving them two different objects. If the patch is empty the two objects match and we are ready, right? +There are existing libraries in the wild that can calculate a patch by giving them two different objects. If the patch is empty the two objects match and we are ready, right? Well not quite. JSON Merge Patch, defined by [rfc7396](https://tools.ietf.org/html/rfc7396) replaces lists completely which is not always what we need. Kubernetes defines and uses a modified version called [strategic merge patch](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md). -Strategic Merge Patch extends the JSON Merge Patch format by adding explicit directives for deleting, replacing, ordering and merging lists. +Strategic Merge Patch extends the JSON Merge Patch format by adding explicit directives for deleting, replacing, ordering and merging lists. It uses the go struct tag of the API objects to determine what lists should be merged and which ones should not. Worth to note it's not tied to Kubernetes objects only, so we can use this for matching custom go structs as well. @@ -46,21 +45,21 @@ Kubernetes objects only, so we can use this for matching custom go structs as we #### Defaults and version compatibility As outlined previously Kubernetes objects are amended with different default values when submitted. For example PodSpec.RestartPolicy will be set to "Always" when -ommitted from the object, so we will have a mismatch when we try to compare it later. This library uses the same functions to set the default values on the local -objects before matching so that they won't differ. +ommitted from the object, so we will have a mismatch when we try to compare it later. This library uses the same functions to set the default values on the local +objects before matching so that they won't differ. -Since the defaults functions are defined in the main kubernetes repo, there is a higher chance that objects decorated using these functions will be incompatible +Since the defaults functions are defined in the main kubernetes repo, there is a higher chance that objects decorated using these functions will be incompatible when comparing them with objects coming from different server versions. Also since the library depends on the kubernetes repo it is more tightly coupled to it's version. -To preserve compatibility between the client and server versions [.circleci/config.yml](.circleci/config.yml) contains jobs that run the integration test suite against Kubernetes +To preserve compatibility between the client and server versions [.circleci/config.yml](.circleci/config.yml) contains jobs that run the integration test suite against Kubernetes versions from 1.10 to 1.14. The library itself is known and tested to be working with operators depending on Kubernetes client version 1.12 and 1.13. #### Generated values -There are values that are generated by the API Server dynamically. To workaround this the library removes null fields from the patch as long as it's not inside a list. -(In case of lists, even if we remove null fields we would still left with `setElementOrder` directives) -This works as long as we don't set/unset complete fields on the objects conditionally, because in that case we would miss to detect a change to unset something. +There are values that are generated by the API Server dynamically. To workaround this the library removes null fields from the patch as long as it's not inside a list. +(In case of lists, even if we remove null fields we would still left with `setElementOrder` directives) +This works as long as we don't set/unset complete fields on the objects conditionally, because in that case we would miss to detect a change to unset something. -In case a field gets removed from somewhere inside a list we have to explicitly tell to ignore it. One example is NodePort in Service objects, see [service.go](service.go). +In case a field gets removed from somewhere inside a list we have to explicitly tell to ignore it. One example is NodePort in Service objects, see [service.go](service.go). Another example is Volume and VolumeMount generated automatically for the service account token, see [pod.go](pod.go). diff --git a/patch/ignorepdb.go b/patch/ignorepdb.go deleted file mode 100644 index ac43342..0000000 --- a/patch/ignorepdb.go +++ /dev/null @@ -1,84 +0,0 @@ -// Copyright © 2021 Banzai Cloud -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package patch - -import ( - "reflect" - "strings" - - "emperror.dev/errors" - json "github.com/json-iterator/go" -) - -func IgnorePDBSelector() CalculateOption { - return func(current, modified []byte) ([]byte, []byte, error) { - currentResource := map[string]interface{}{} - if err := json.Unmarshal(current, ¤tResource); err != nil { - return []byte{}, []byte{}, errors.Wrap(err, "could not unmarshal byte sequence for current") - } - - modifiedResource := map[string]interface{}{} - if err := json.Unmarshal(modified, &modifiedResource); err != nil { - return []byte{}, []byte{}, errors.Wrap(err, "could not unmarshal byte sequence for modified") - } - - if isPDB(currentResource) && isPDB(modifiedResource) && reflect.DeepEqual(getPDBSelector(currentResource), getPDBSelector(modifiedResource)) { - var err error - current, err = deletePDBSelector(currentResource) - if err != nil { - return nil, nil, errors.Wrap(err, "delete pdb selector from current") - } - modified, err = deletePDBSelector(modifiedResource) - if err != nil { - return nil, nil, errors.Wrap(err, "delete pdb selector from modified") - } - } - - return current, modified, nil - } -} - -func isPDB(resource map[string]interface{}) bool { - if av, ok := resource["apiVersion"].(string); ok { - return strings.HasPrefix(av, "policy/") && resource["kind"] == "PodDisruptionBudget" - } - return false -} - -func getPDBSelector(resource map[string]interface{}) interface{} { - if spec, ok := resource["spec"]; ok { - if spec, ok := spec.(map[string]interface{}); ok { - if selector, ok := spec["selector"]; ok { - return selector - } - } - } - return nil -} - -func deletePDBSelector(resource map[string]interface{}) ([]byte, error) { - if spec, ok := resource["spec"]; ok { - if spec, ok := spec.(map[string]interface{}); ok { - delete(spec, "selector") - } - } - - obj, err := json.ConfigCompatibleWithStandardLibrary.Marshal(resource) - if err != nil { - return []byte{}, errors.Wrap(err, "could not marshal byte sequence") - } - - return obj, nil -} diff --git a/tests/integration_test.go b/tests/integration_test.go index ca61095..e0eb681 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -24,7 +24,6 @@ import ( appsv1 "k8s.io/api/apps/v1" "k8s.io/api/autoscaling/v2beta1" v1 "k8s.io/api/core/v1" - v1beta12 "k8s.io/api/policy/v1beta1" rbacv1 "k8s.io/api/rbac/v1" crdv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -704,58 +703,6 @@ func TestIntegration(t *testing.T) { }, }, }), - NewTestMatch("pdb match", - &v1beta12.PodDisruptionBudget{ - TypeMeta: metav1.TypeMeta{ - APIVersion: v1beta12.SchemeGroupVersion.String(), - Kind: "PodDisruptionBudget", - }, - ObjectMeta: standardObjectMeta(), - Spec: v1beta12.PodDisruptionBudgetSpec{ - MinAvailable: intstrRef(intstr.FromInt(1)), - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app": "example", - }, - }, - }, - }), - NewTestDiff("pdb diff on matchlabels", - &v1beta12.PodDisruptionBudget{ - TypeMeta: metav1.TypeMeta{ - APIVersion: v1beta12.SchemeGroupVersion.String(), - Kind: "PodDisruptionBudget", - }, - ObjectMeta: standardObjectMeta(), - Spec: v1beta12.PodDisruptionBudgetSpec{ - MinAvailable: intstrRef(intstr.FromInt(1)), - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app": "example", - }, - }, - }, - }). - withRemoteChange(func(i interface{}) { - pdb := i.(*v1beta12.PodDisruptionBudget) - pdb.Spec.Selector.MatchLabels = map[string]string{ - "app": "example2", - } - }), - NewTestMatch("pdb match even though status changes", - &v1beta12.PodDisruptionBudget{ - ObjectMeta: standardObjectMeta(), - Spec: v1beta12.PodDisruptionBudgetSpec{ - MinAvailable: intstrRef(intstr.FromInt(1)), - }, - }). - withRemoteChange(func(i interface{}) { - pdb := i.(*v1beta12.PodDisruptionBudget) - pdb.Status.CurrentHealthy = 1 - pdb.Status.DesiredHealthy = 1 - pdb.Status.ExpectedPods = 1 - pdb.Status.ObservedGeneration = 1 - }), NewTestMatch("pvc match", &v1.PersistentVolumeClaim{ ObjectMeta: standardObjectMeta(), diff --git a/tests/main_test.go b/tests/main_test.go index 27498b6..d564024 100644 --- a/tests/main_test.go +++ b/tests/main_test.go @@ -23,16 +23,13 @@ import ( "testing" "emperror.dev/errors" - "github.com/cisco-open/k8s-objectmatcher/patch" admregv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" "k8s.io/api/autoscaling/v2beta1" v1 "k8s.io/api/core/v1" - policyv1beta1 "k8s.io/api/policy/v1beta1" rbacv1 "k8s.io/api/rbac/v1" crdv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextension "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -41,6 +38,8 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/clientcmd" "k8s.io/klog/v2" + + "github.com/cisco-open/k8s-objectmatcher/patch" ) var ( @@ -196,7 +195,6 @@ func testMatchOnObject(testItem *TestItem, ignoreField string) error { opts := []patch.CalculateOption{ patch.IgnoreStatusFields(), patch.IgnoreVolumeClaimTemplateTypeMetaAndStatus(), - patch.IgnorePDBSelector(), patch.IgnoreField(ignoreField), } @@ -354,22 +352,6 @@ func testMatchOnObject(testItem *TestItem, ignoreField string) error { log.Printf("Failed to remove object %s %+v", existing.GetName(), err) } }() - case *policyv1beta1.PodDisruptionBudget: - existing, err = testContext.Client.PolicyV1beta1().PodDisruptionBudgets(newObject.GetNamespace()).Create(context.Background(), newObject.(*policyv1beta1.PodDisruptionBudget), metav1.CreateOptions{}) - if err != nil { - return errors.WrapWithDetails(err, "failed to create object", "object", newObject) - } - defer func() { - err = testContext.Client.PolicyV1beta1().PodDisruptionBudgets(newObject.GetNamespace()).Delete(context.Background(), existing.GetName(), deleteOptions) - if err != nil { - log.Printf("Failed to remove object %s %+v", existing.GetName(), err) - } - }() - // In case of PodDisruptionBudget resources `patchStrategy:"replace"` results in a constant diff from kubernetes version 1.21 - // The IgnorePDBSelector CalculateOption can only help in this situation if either the current or modified object contains gvk information, this is why it's set here explicitly - te, _ := meta.TypeAccessor(existing) - te.SetKind("PodDisruptionBudget") - te.SetAPIVersion("policy/v1beta1") case *v1.PersistentVolumeClaim: existing, err = testContext.Client.CoreV1().PersistentVolumeClaims(newObject.GetNamespace()).Create(context.Background(), newObject.(*v1.PersistentVolumeClaim), metav1.CreateOptions{}) if err != nil {