From cc659369dd266306bacdeaecb29a34b4122f3dff Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 24 Jun 2022 11:24:12 +0200 Subject: [PATCH] detect invalid spec fields in dryRunPatch which is caused by immutability via webhooks --- .../structuredmerge/serversidepathhelper.go | 51 ++++++++- .../serversidepathhelper_test.go | 107 ++++++++++++++++++ 2 files changed, 156 insertions(+), 2 deletions(-) diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index 8b4852e3c730..4f2727412b10 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -18,10 +18,12 @@ package structuredmerge import ( "encoding/json" + "strings" jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" "golang.org/x/net/context" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" @@ -32,8 +34,10 @@ import ( "sigs.k8s.io/cluster-api/internal/contract" ) -// TopologyManagerName is the manager name in managed fields for the topology controller. -const TopologyManagerName = "capi-topology" +const ( + // TopologyManagerName is the manager name in managed fields for the topology controller. + TopologyManagerName = "capi-topology" +) // allowedPaths defines the list of paths that the topology controller should express opinion on. var allowedPaths = []contract.Path{ @@ -167,6 +171,12 @@ func dryRunSSAPatch(ctx context.Context, c client.Client, originalUnstructured, err := c.Patch(ctx, dryRunUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership) if err != nil { + // If a webhook blocks due to immutability of spec fields the dryRun for sure + // has spec changes. + if isInvalidSpecChangesStatusError(err) { + return true, true, nil + } + // This catches errors like metadata.uid changes. return false, false, errors.Wrap(err, "failed to request dry-run server side apply") } @@ -252,3 +262,40 @@ func cleanupLegacyManagedFields(ctx context.Context, obj *unstructured.Unstructu return c.Patch(ctx, obj, client.MergeFrom(base)) } + +// isInvalidSpecChangesStatusError identifies an error to be only caused by fields +// inside spec which have an invalid value. This is the case if e.g. a validating +// webhook blocks updating a field due to immutability. +func isInvalidSpecChangesStatusError(err error) bool { + if !apierrors.IsInvalid(err) { + return false + } + + statusErr, ok := err.(*apierrors.StatusError) + if !ok { + return false + } + + // It is expected to contain details about the fields causing the error. + if statusErr.Status().Details == nil { + return false + } + + // It is expected to find at least a single cause + if len(statusErr.Status().Details.Causes) == 0 { + return false + } + for _, cause := range statusErr.Status().Details.Causes { + // If a cause is CauseTypeFieldValueInvalid it could be realated to other errors. + if cause.Type != metav1.CauseTypeFieldValueInvalid { + return false + } + // If a field is not below spec it could be related to invalid values which + // would not be solved through template rotation + if !strings.HasPrefix(cause.Field, "spec.") { + return false + } + } + + return true +} diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index ad084ee4cb7d..b9b4342d4460 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -21,6 +21,8 @@ import ( "testing" . "github.com/onsi/gomega" + "github.com/pkg/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" @@ -550,3 +552,108 @@ func getTopologyManagedFields(original client.Object) map[string]interface{} { } return r } + +func Test_isInvalidSpecChangesStatusError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "spec immutability", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: "Me, the webhook blocked due to immutability.", + Field: "spec.foo.bar", + }}, + }, + }, + }, + want: true, + }, + { + name: "not only spec immutability", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: "Me, the webhook blocked due to immutability.", + Field: "spec.foo.bar", + }, { + Type: metav1.CauseTypeFieldValueInvalid, + Field: "metadata.foo", + }}, + }, + }, + }, + want: false, + }, + { + name: "not details", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + }, + }, + want: false, + }, + { + name: "not causes", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{}, + }, + }, + want: false, + }, + { + name: "not spec immutability", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Field: "metadata.uid", + }}, + }, + }, + }, + want: true, + }, + { + name: "not CauseTypeFieldValueInvalid", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueNotSupported, + Field: "spec.foo", + }}, + }, + }, + }, + want: false, + }, + { + name: "other error", + err: errors.New("foo"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isInvalidSpecChangesStatusError(tt.err); got != tt.want { + t.Errorf("isInvalidSpecChangesStatusError() = %v, want %v", got, tt.want) + } + }) + } +}