From d1b3dffd4ec394a9c7f672d219de56ff6cf66aa3 Mon Sep 17 00:00:00 2001
From: Utku Ozdemir <utku.ozdemir@siderolabs.com>
Date: Fri, 3 Jan 2025 12:53:25 +0100
Subject: [PATCH] fix: fix immutability checks in infra provider state

Found a couple of bugs in the infra provider state:

- For some reason, sometimes a resource update is detected where nothing other than the `.metadata.version` was changing. In these cases, the validation blocked the update. Fix this by ignoring the version field. Since we ignore the version field, we now compare the specs as well as the metadata.

- Infra providers were able to modify `infra.Machine` resources when they shouldn't have. The fix above also addresses that.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
---
 .../runtime/omni/infraprovider/state.go       | 46 ++++++++++++++++---
 .../runtime/omni/infraprovider/state_test.go  | 15 ++++--
 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/internal/backend/runtime/omni/infraprovider/state.go b/internal/backend/runtime/omni/infraprovider/state.go
index 002740f0..a927fb01 100644
--- a/internal/backend/runtime/omni/infraprovider/state.go
+++ b/internal/backend/runtime/omni/infraprovider/state.go
@@ -142,13 +142,7 @@ func (st *State) Update(ctx context.Context, newResource resource.Resource, opts
 
 	switch newResource.Metadata().Type() {
 	case infra.MachineRequestType, infra.InfraMachineType:
-		oldMd := oldResource.Metadata().Copy()
-		oldMd.Finalizers().Set(resource.Finalizers{})
-
-		newMd := newResource.Metadata().Copy()
-		newMd.Finalizers().Set(resource.Finalizers{})
-
-		if !oldMd.Equal(newMd) {
+		if !st.resourcesAreEqual(oldResource, newResource) {
 			return status.Errorf(codes.PermissionDenied, "infra providers are not allowed to update %q resources other than setting finalizers", newResource.Metadata().Type())
 		}
 	}
@@ -163,6 +157,44 @@ func (st *State) Update(ctx context.Context, newResource resource.Resource, opts
 	return status.Errorf(codes.NotFound, "not found")
 }
 
+func (st *State) resourcesAreEqual(res1, res2 resource.Resource) bool {
+	var ignoreVersion resource.Version
+
+	md1Copy := res1.Metadata().Copy()
+	md2Copy := res2.Metadata().Copy()
+
+	md1Copy.SetVersion(ignoreVersion)
+	md2Copy.SetVersion(ignoreVersion)
+	md1Copy.Finalizers().Set(resource.Finalizers{})
+	md2Copy.Finalizers().Set(resource.Finalizers{})
+
+	if !md1Copy.Equal(md2Copy) {
+		return false
+	}
+
+	type equaler interface {
+		Equal(any) bool
+	}
+
+	spec1 := res1.Spec()
+	spec2 := res2.Spec()
+
+	if spec1 == spec2 {
+		return true
+	}
+
+	if res1.Spec() == nil || res2.Spec() == nil {
+		return false
+	}
+
+	s1, ok := res1.Spec().(equaler)
+	if !ok {
+		return false
+	}
+
+	return s1.Equal(res2.Spec())
+}
+
 // Destroy implements state.CoreState interface.
 func (st *State) Destroy(ctx context.Context, pointer resource.Pointer, option ...state.DestroyOption) error {
 	infraProviderID, err := st.checkAuthorization(ctx, pointer.Namespace(), pointer.Type())
diff --git a/internal/backend/runtime/omni/infraprovider/state_test.go b/internal/backend/runtime/omni/infraprovider/state_test.go
index 0f5504a0..6e328ccd 100644
--- a/internal/backend/runtime/omni/infraprovider/state_test.go
+++ b/internal/backend/runtime/omni/infraprovider/state_test.go
@@ -65,8 +65,8 @@ func TestInfraProviderAccess(t *testing.T) {
 
 		return nil
 	}, func(t *testing.T, err error) {
-		assert.True(t, validated.IsValidationError(err))
-		assert.ErrorContains(t, err, "machine request spec is immutable")
+		assert.Equal(t, codes.PermissionDenied, status.Code(err))
+		assert.ErrorContains(t, err, `infra providers are not allowed to update "MachineRequests.omni.sidero.dev" resources other than setting finalizers`)
 	})
 
 	// InfraMachine
@@ -80,7 +80,8 @@ func TestInfraProviderAccess(t *testing.T) {
 
 		return nil
 	}, func(t *testing.T, err error) {
-		require.NoError(t, err)
+		assert.Equal(t, codes.PermissionDenied, status.Code(err))
+		assert.ErrorContains(t, err, `infra providers are not allowed to update "InfraMachines.omni.sidero.dev" resources other than setting finalizers`)
 	})
 
 	// MachineRequestStatus
@@ -209,6 +210,14 @@ func TestInternalAccess(t *testing.T) {
 
 	err = st.Create(ctx, mr)
 	assert.NoError(t, err)
+
+	_, err = safe.StateUpdateWithConflicts(ctx, st, mr.Metadata(), func(res *infra.MachineRequest) error {
+		res.TypedSpec().Value.TalosVersion = "v1.2.5"
+
+		return nil
+	})
+	assert.True(t, validated.IsValidationError(err))
+	assert.ErrorContains(t, err, "machine request spec is immutable")
 }
 
 func TestInfraProviderSpecificNamespace(t *testing.T) {