detect invalid spec fields in dryRunPatch which is caused by immutabi…

…lity via webhooks

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
+}

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)
+			}
+		})
+	}
+}