From 515b55ccc2ec7e2b4f5f460d299148c111486d1d Mon Sep 17 00:00:00 2001 From: Jim Crossley Date: Fri, 6 Dec 2019 16:30:02 -0500 Subject: [PATCH 1/2] Pull in latest manifestival client-go branch This employs the k8s 3-way strategic merging for all manifest resources except ConfigMaps. This makes manifestival's Apply behave like kubectl apply. --- Gopkg.lock | 12 +- third_party/VENDOR-LICENSE | 32 +++- .../jcrossley3/manifestival/manifestival.go | 51 ++---- .../jcrossley3/manifestival/patch/patch.go | 131 ++++++++++++++ .../pkg/util/jsonmergepatch/patch.go | 160 ++++++++++++++++++ 5 files changed, 344 insertions(+), 42 deletions(-) create mode 100644 vendor/github.com/jcrossley3/manifestival/patch/patch.go create mode 100644 vendor/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go diff --git a/Gopkg.lock b/Gopkg.lock index 466d0649..a033bbe2 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -317,11 +317,14 @@ [[projects]] branch = "client-go" - digest = "1:ca0e2c3f72dd218000ee434b91b9f082a7d1f6a36dfd4dcb11b1e736928df367" + digest = "1:e1795db5018a02477e3ae7cd61ff67c460748ca6597b0fc1fed45b62a7ab9e21" name = "github.com/jcrossley3/manifestival" - packages = ["."] + packages = [ + ".", + "patch", + ] pruneopts = "NUT" - revision = "6a601d02309cdd03932c50d1f986570bc513289d" + revision = "144622fb1ed6d5ea8f60aa29abfbbd56ad8306c1" [[projects]] digest = "1:1f2aebae7e7c856562355ec0198d8ca2fa222fb05e5b1b66632a1fce39631885" @@ -877,7 +880,7 @@ version = "kubernetes-1.15.3" [[projects]] - digest = "1:e47c9cc452f507b8be0fad9e7023205274823aeb848927d1880d9737ec2dab2d" + digest = "1:f01ac8448d8d22426edf7a2b446575dfc0bba323742f77e733f06ec0916e0997" name = "k8s.io/apimachinery" packages = [ "pkg/api/equality", @@ -911,6 +914,7 @@ "pkg/util/framer", "pkg/util/intstr", "pkg/util/json", + "pkg/util/jsonmergepatch", "pkg/util/mergepatch", "pkg/util/naming", "pkg/util/net", diff --git a/third_party/VENDOR-LICENSE b/third_party/VENDOR-LICENSE index d312c59d..e177629e 100644 --- a/third_party/VENDOR-LICENSE +++ b/third_party/VENDOR-LICENSE @@ -1163,6 +1163,37 @@ OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +=========================================================== +Import: knative.dev/serving-operator/vendor/github.com/evanphx/json-patch + +Copyright (c) 2014, Evan Phoenix +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + +* Redistributions of source code must retain the above copyright notice, this + list of conditions and the following disclaimer. +* Redistributions in binary form must reproduce the above copyright notice + this list of conditions and the following disclaimer in the documentation + and/or other materials provided with the distribution. +* Neither the name of the Evan Phoenix nor the names of its contributors + may be used to endorse or promote products derived from this software + without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + + + =========================================================== Import: knative.dev/serving-operator/vendor/github.com/go-logr/logr @@ -8524,4 +8555,3 @@ DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - diff --git a/vendor/github.com/jcrossley3/manifestival/manifestival.go b/vendor/github.com/jcrossley3/manifestival/manifestival.go index 0eea05cd..c52f3522 100644 --- a/vendor/github.com/jcrossley3/manifestival/manifestival.go +++ b/vendor/github.com/jcrossley3/manifestival/manifestival.go @@ -5,9 +5,10 @@ import ( "github.com/go-logr/logr" "github.com/go-logr/zapr" + "github.com/jcrossley3/manifestival/patch" "github.com/operator-framework/operator-sdk/pkg/restmapper" "go.uber.org/zap" - "k8s.io/apimachinery/pkg/api/equality" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -94,14 +95,22 @@ func (f *Manifest) Apply(spec *unstructured.Unstructured) error { } if current == nil { logResource("Creating", spec) + annotate(spec, v1.LastAppliedConfigAnnotation, patch.MakeLastAppliedConfig(spec)) annotate(spec, "manifestival", resourceCreated) - if _, err = resource.Create(spec.DeepCopy(), metav1.CreateOptions{}); err != nil { + if _, err = resource.Create(spec, metav1.CreateOptions{}); err != nil { return err } } else { - // Update existing one - if UpdateChanged(spec.UnstructuredContent(), current.UnstructuredContent()) { - logResource("Updating", spec) + patch, err := patch.NewPatch(spec, current) + if err != nil { + return err + } + if patch.IsRequired() { + log.Info("Merging", "diff", patch) + if err := patch.Merge(current); err != nil { + return err + } + logResource("Updating", current) if _, err = resource.Update(current, metav1.UpdateOptions{}); err != nil { return err } @@ -179,38 +188,6 @@ func (f *Manifest) ResourceInterface(spec *unstructured.Unstructured) (dynamic.R return f.client.Resource(mapping.Resource).Namespace(spec.GetNamespace()), nil } -// UpdateChanged recursively merges JSON-style values in `src` into `tgt`. -// -// We need to preserve the top-level target keys, specifically -// 'metadata.resourceVersion', 'spec.clusterIP', and any existing -// entries in a ConfigMap's 'data' field. So we only overwrite fields -// set in our src resource. -// TODO: Use Patch instead -func UpdateChanged(src, tgt map[string]interface{}) bool { - changed := false - for k, v := range src { - // Special case for ConfigMaps - if k == "data" && !equality.Semantic.DeepEqual(v, tgt[k]) { - tgt[k], changed = v, true - continue - } - if v, ok := v.(map[string]interface{}); ok { - if tgt[k] == nil { - tgt[k], changed = v, true - } else if UpdateChanged(v, tgt[k].(map[string]interface{})) { - // This could be an issue if a field in a nested src - // map doesn't overwrite its corresponding tgt - changed = true - } - continue - } - if !equality.Semantic.DeepEqual(v, tgt[k]) { - tgt[k], changed = v, true - } - } - return changed -} - func logResource(msg string, spec *unstructured.Unstructured) { name := fmt.Sprintf("%s/%s", spec.GetNamespace(), spec.GetName()) log.Info(msg, "name", name, "type", spec.GroupVersionKind()) diff --git a/vendor/github.com/jcrossley3/manifestival/patch/patch.go b/vendor/github.com/jcrossley3/manifestival/patch/patch.go new file mode 100644 index 00000000..93fa900c --- /dev/null +++ b/vendor/github.com/jcrossley3/manifestival/patch/patch.go @@ -0,0 +1,131 @@ +package patch + +import ( + "bytes" + + jsonpatch "github.com/evanphx/json-patch" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/jsonmergepatch" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/client-go/kubernetes/scheme" +) + +type Patch interface { + IsRequired() bool + Merge(*unstructured.Unstructured) error +} + +type jsonPatch struct { + patch []byte + config string +} + +type strategicPatch struct { + jsonPatch + schema strategicpatch.LookupPatchMeta +} + +func NewPatch(src, tgt *unstructured.Unstructured) (Patch, error) { + var original, modified, current []byte + var err error + original = getLastAppliedConfig(tgt) + config := MakeLastAppliedConfig(src) + if modified, err = src.MarshalJSON(); err != nil { + return nil, err + } + if current, err = tgt.MarshalJSON(); err != nil { + return nil, err + } + obj, err := scheme.Scheme.New(src.GroupVersionKind()) + switch { + case src.GetKind() == "ConfigMap": + fallthrough // force "overwrite" merge + case runtime.IsNotRegisteredError(err): + return createJsonPatch(original, modified, current, config) + case err != nil: + return nil, err + default: + return createStrategicPatch(original, modified, current, obj, config) + } +} + +func createJsonPatch(original, modified, current []byte, config string) (*jsonPatch, error) { + patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(original, modified, current) + return &jsonPatch{patch, config}, err +} + +func createStrategicPatch(original, modified, current []byte, obj runtime.Object, config string) (*strategicPatch, error) { + schema, err := strategicpatch.NewPatchMetaFromStruct(obj) + if err != nil { + return nil, err + } + patch, err := strategicpatch.CreateThreeWayMergePatch(original, modified, current, schema, true) + return &strategicPatch{jsonPatch{patch, config}, schema}, err +} + +func (p *jsonPatch) String() string { + return string(p.patch) +} + +func (p *jsonPatch) IsRequired() bool { + return !bytes.Equal(p.patch, []byte("{}")) +} + +func (p *jsonPatch) Merge(spec *unstructured.Unstructured) (err error) { + var current, result []byte + if current, err = spec.MarshalJSON(); err != nil { + return + } + if result, err = jsonpatch.MergePatch(current, p.patch); err != nil { + return + } + err = spec.UnmarshalJSON(result) + if err == nil { + setLastAppliedConfig(spec, p.config) + } + return +} + +func (p *strategicPatch) Merge(spec *unstructured.Unstructured) (err error) { + var current, result []byte + if current, err = spec.MarshalJSON(); err != nil { + return + } + if result, err = strategicpatch.StrategicMergePatchUsingLookupPatchMeta(current, p.jsonPatch.patch, p.schema); err != nil { + return + } + err = spec.UnmarshalJSON(result) + if err == nil { + setLastAppliedConfig(spec, p.config) + } + return +} + +func getLastAppliedConfig(spec *unstructured.Unstructured) []byte { + annotations := spec.GetAnnotations() + if annotations == nil { + return nil + } + return []byte(annotations[v1.LastAppliedConfigAnnotation]) +} + +func setLastAppliedConfig(spec *unstructured.Unstructured, config string) { + annotations := spec.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[v1.LastAppliedConfigAnnotation] = config + spec.SetAnnotations(annotations) +} + +func MakeLastAppliedConfig(spec *unstructured.Unstructured) string { + ann := spec.GetAnnotations() + if len(ann) > 0 { + delete(ann, v1.LastAppliedConfigAnnotation) + spec.SetAnnotations(ann) + } + bytes, _ := spec.MarshalJSON() + return string(bytes) +} diff --git a/vendor/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go b/vendor/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go new file mode 100644 index 00000000..e56e1773 --- /dev/null +++ b/vendor/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go @@ -0,0 +1,160 @@ +/* +Copyright 2017 The Kubernetes Authors. + +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 jsonmergepatch + +import ( + "fmt" + "reflect" + + "github.com/evanphx/json-patch" + "k8s.io/apimachinery/pkg/util/json" + "k8s.io/apimachinery/pkg/util/mergepatch" +) + +// Create a 3-way merge patch based-on JSON merge patch. +// Calculate addition-and-change patch between current and modified. +// Calculate deletion patch between original and modified. +func CreateThreeWayJSONMergePatch(original, modified, current []byte, fns ...mergepatch.PreconditionFunc) ([]byte, error) { + if len(original) == 0 { + original = []byte(`{}`) + } + if len(modified) == 0 { + modified = []byte(`{}`) + } + if len(current) == 0 { + current = []byte(`{}`) + } + + addAndChangePatch, err := jsonpatch.CreateMergePatch(current, modified) + if err != nil { + return nil, err + } + // Only keep addition and changes + addAndChangePatch, addAndChangePatchObj, err := keepOrDeleteNullInJsonPatch(addAndChangePatch, false) + if err != nil { + return nil, err + } + + deletePatch, err := jsonpatch.CreateMergePatch(original, modified) + if err != nil { + return nil, err + } + // Only keep deletion + deletePatch, deletePatchObj, err := keepOrDeleteNullInJsonPatch(deletePatch, true) + if err != nil { + return nil, err + } + + hasConflicts, err := mergepatch.HasConflicts(addAndChangePatchObj, deletePatchObj) + if err != nil { + return nil, err + } + if hasConflicts { + return nil, mergepatch.NewErrConflict(mergepatch.ToYAMLOrError(addAndChangePatchObj), mergepatch.ToYAMLOrError(deletePatchObj)) + } + patch, err := jsonpatch.MergePatch(deletePatch, addAndChangePatch) + if err != nil { + return nil, err + } + + var patchMap map[string]interface{} + err = json.Unmarshal(patch, &patchMap) + if err != nil { + return nil, fmt.Errorf("Failed to unmarshal patch for precondition check: %s", patch) + } + meetPreconditions, err := meetPreconditions(patchMap, fns...) + if err != nil { + return nil, err + } + if !meetPreconditions { + return nil, mergepatch.NewErrPreconditionFailed(patchMap) + } + + return patch, nil +} + +// keepOrDeleteNullInJsonPatch takes a json-encoded byte array and a boolean. +// It returns a filtered object and its corresponding json-encoded byte array. +// It is a wrapper of func keepOrDeleteNullInObj +func keepOrDeleteNullInJsonPatch(patch []byte, keepNull bool) ([]byte, map[string]interface{}, error) { + var patchMap map[string]interface{} + err := json.Unmarshal(patch, &patchMap) + if err != nil { + return nil, nil, err + } + filteredMap, err := keepOrDeleteNullInObj(patchMap, keepNull) + if err != nil { + return nil, nil, err + } + o, err := json.Marshal(filteredMap) + return o, filteredMap, err +} + +// keepOrDeleteNullInObj will keep only the null value and delete all the others, +// if keepNull is true. Otherwise, it will delete all the null value and keep the others. +func keepOrDeleteNullInObj(m map[string]interface{}, keepNull bool) (map[string]interface{}, error) { + filteredMap := make(map[string]interface{}) + var err error + for key, val := range m { + switch { + case keepNull && val == nil: + filteredMap[key] = nil + case val != nil: + switch typedVal := val.(type) { + case map[string]interface{}: + // Explicitly-set empty maps are treated as values instead of empty patches + if len(typedVal) == 0 { + if !keepNull { + filteredMap[key] = typedVal + } + continue + } + + var filteredSubMap map[string]interface{} + filteredSubMap, err = keepOrDeleteNullInObj(typedVal, keepNull) + if err != nil { + return nil, err + } + + // If the returned filtered submap was empty, this is an empty patch for the entire subdict, so the key + // should not be set + if len(filteredSubMap) != 0 { + filteredMap[key] = filteredSubMap + } + + case []interface{}, string, float64, bool, int64, nil: + // Lists are always replaced in Json, no need to check each entry in the list. + if !keepNull { + filteredMap[key] = val + } + default: + return nil, fmt.Errorf("unknown type: %v", reflect.TypeOf(typedVal)) + } + } + } + return filteredMap, nil +} + +func meetPreconditions(patchObj map[string]interface{}, fns ...mergepatch.PreconditionFunc) (bool, error) { + // Apply the preconditions to the patch, and return an error if any of them fail. + for _, fn := range fns { + if !fn(patchObj) { + return false, fmt.Errorf("precondition failed for: %v", patchObj) + } + } + return true, nil +} From dc4f28296fcdb223368839e4acbe7ff43a2f4b71 Mon Sep 17 00:00:00 2001 From: Jim Crossley Date: Fri, 6 Dec 2019 16:32:40 -0500 Subject: [PATCH 2/2] Eliminate superfluous updates on Deployments and Images among others This should remove the noise in the logs from manifestival trying to update resources right after they're created. When we convert from unstructured to typed structs and back, we get zeroed defaults added to the unstructured objects (usually creationTimestamp) which manifestival interprets as a change worth persisting. --- pkg/reconciler/knativeserving/common/certs.go | 3 +++ pkg/reconciler/knativeserving/common/images.go | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/pkg/reconciler/knativeserving/common/certs.go b/pkg/reconciler/knativeserving/common/certs.go index 26bdfcc5..83ee3cc0 100644 --- a/pkg/reconciler/knativeserving/common/certs.go +++ b/pkg/reconciler/knativeserving/common/certs.go @@ -23,6 +23,7 @@ import ( "go.uber.org/zap" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/kubernetes/scheme" servingv1alpha1 "knative.dev/serving-operator/pkg/apis/serving/v1alpha1" @@ -54,6 +55,8 @@ func CustomCertsTransform(instance *servingv1alpha1.KnativeServing, log *zap.Sug if err := scheme.Scheme.Convert(deployment, u, nil); err != nil { return err } + // Avoid superfluous updates from converted zero defaults + u.SetCreationTimestamp(metav1.Time{}) } return nil } diff --git a/pkg/reconciler/knativeserving/common/images.go b/pkg/reconciler/knativeserving/common/images.go index cc330a42..14e50973 100644 --- a/pkg/reconciler/knativeserving/common/images.go +++ b/pkg/reconciler/knativeserving/common/images.go @@ -22,6 +22,7 @@ import ( "go.uber.org/zap" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/kubernetes/scheme" caching "knative.dev/caching/pkg/apis/caching/v1alpha1" @@ -75,6 +76,9 @@ func updateDeployment(instance *servingv1alpha1.KnativeServing, u *unstructured. if err != nil { return err } + // The zero-value timestamp defaulted by the conversion causes + // superfluous updates + u.SetCreationTimestamp(metav1.Time{}) log.Debugw("Finished conversion", "name", u.GetName(), "unstructured", u.Object) return nil @@ -109,6 +113,10 @@ func updateCachingImage(instance *servingv1alpha1.KnativeServing, u *unstructure if err != nil { return err } + // Cleanup zero-value default to prevent superfluous updates + u.SetCreationTimestamp(metav1.Time{}) + delete(u.Object, "status") + log.Debugw("Finished conversion", "name", u.GetName(), "unstructured", u.Object) return nil }