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 <[email protected]>

Author: utkuozdemir

Diff for: 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 for: 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) {