ProviderID of infra machines should not change: Validate that.

Author: guettli

api/v1beta1/hetznerbaremetalmachine_types.go

@@ -70,7 +70,10 @@ type HetznerBareMetalMachineSpec struct {
 	// ProviderID will be the hetznerbaremetalmachine which is set by the controller in the
 	// `hrobot://<server-id>` format. Before caph v1.1.0 the ProviderID had the format
 	// `hcloud://bm-NNNNN`. Starting with caph v1.1.x this was changed to `hrobot://NNNNN`. This
-	// aligns to the upstream hcloud ccm. In the long run we want to discontinue our ccm fork.
+	// aligns to the upstream hcloud ccm. In the long run we want to discontinue our ccm fork. After
+	// the ProviderID was set once, it never changes. It is safe to update the ccm in a running
+	// workload-cluster. Only new nodes will get the new format.
+	//
 	// +optional
 	ProviderID *string `json:"providerID,omitempty"`
 

api/v1beta1/hetznerbaremetalmachine_validation.go

@@ -86,5 +86,14 @@ func validateHetznerBareMetalMachineSpecUpdate(oldSpec, newSpec HetznerBareMetal
 		)
 	}
 
+	if oldSpec.ProviderID != nil && *oldSpec.ProviderID != "" {
+		// once the Provider was set, the value must not change.
+		if newSpec.ProviderID == nil || *oldSpec.ProviderID != *newSpec.ProviderID {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "providerID"), newSpec.ProviderID, "providerID immutable"),
+			)
+		}
+	}
+
 	return allErrs
 }

api/v1beta1/hetznerbaremetalmachine_validation_test.go

@@ -17,11 +17,14 @@ limitations under the License.
 package v1beta1
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/ptr"
 )
 
 func TestValidateHetznerBareMetalMachineSpecCreate(t *testing.T) {
@@ -429,3 +432,43 @@ func TestValidateHetznerBareMetalMachineSpecUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateHetznerBareMetalMachineSpecUpdate_ProviderID(t *testing.T) {
+	got := validateHetznerBareMetalMachineSpecUpdate(
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://foo"),
+		},
+		HetznerBareMetalMachineSpec{})
+	require.Equal(t, `[spec.providerID: Invalid value: "null": providerID immutable]`, fmt.Sprintf("%+v", got))
+
+	got = validateHetznerBareMetalMachineSpecUpdate(
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://foo"),
+		},
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://bar"),
+		})
+	require.Equal(t, `[spec.providerID: Invalid value: "provider://bar": providerID immutable]`, fmt.Sprintf("%+v", got))
+
+	// Allowed Updates
+	got = validateHetznerBareMetalMachineSpecUpdate(
+		HetznerBareMetalMachineSpec{},
+		HetznerBareMetalMachineSpec{})
+	require.Equal(t, `[]`, fmt.Sprintf("%+v", got))
+
+	got = validateHetznerBareMetalMachineSpecUpdate(
+		HetznerBareMetalMachineSpec{},
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://bar"),
+		})
+	require.Equal(t, `[]`, fmt.Sprintf("%+v", got))
+
+	got = validateHetznerBareMetalMachineSpecUpdate(
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://bar"),
+		},
+		HetznerBareMetalMachineSpec{
+			ProviderID: ptr.To("provider://bar"),
+		})
+	require.Equal(t, `[]`, fmt.Sprintf("%+v", got))
+}

pkg/services/baremetal/baremetal/baremetal.go

@@ -899,9 +899,8 @@ func checkForRequeueError(err error, errMessage string) (res reconcile.Result, r
 	return reconcile.Result{}, fmt.Errorf("%s: %w", errMessage, err)
 }
 
-// providerIDFromServerID returns the ProviderID. Before caph v1.1.0 the ProviderID had the format
-// `hcloud://bm-NNNNN`. Starting with caph v1.1.x this was changed to `hrobot://NNNNN`. This aligns
-// to the upstream hcloud ccm. In the long run we want to discontinue our ccm fork.
+// providerIDFromServerID returns the ProviderID. See HetznerBareMetalMachineSpec.ProviderID for
+// docs.
 func providerIDFromServerID(serverID int) string {
 	return fmt.Sprintf("%s%d", providerIDPrefix, serverID)
 }