Address comments

Author: fabriziopandini

api/v1beta1/machine_types.go

@@ -89,27 +89,29 @@ const (
 // Machine's Available condition and corresponding reasons that will be used in v1Beta2 API version.
 const (
 	// MachineAvailableV1Beta2Condition is true if the machine is Ready for at least MinReadySeconds, as defined by the Machine's MinReadySeconds field.
+	// Note: MinReadySeconds is assumed 0 until it will be implemented in v1beta2 API.
 	MachineAvailableV1Beta2Condition = AvailableV1Beta2Condition
 
-	// MachineNotReadyV1Beta2Reason surfaces when a machine is not yet ready (and thus not yet available).
+	// MachineNotReadyV1Beta2Reason surfaces when a machine is not ready (and thus not available).
 	MachineNotReadyV1Beta2Reason = "NotReady"
 
-	// MachineWaitingForMinReadySecondsV1Beta2Reason surfaces when a machine is ready for less then MinReadySeconds (and thus not yet available).
+	// MachineWaitingForMinReadySecondsV1Beta2Reason surfaces when a machine is ready for less than MinReadySeconds (and thus not yet available).
 	MachineWaitingForMinReadySecondsV1Beta2Reason = "WaitingForMinReadySeconds"
 
 	// MachineReadyNotYetReportedV1Beta2Reason surfaces when a machine ready is not reported yet.
 	// Note: this should never happen and it is a signal of some internal error.
 	MachineReadyNotYetReportedV1Beta2Reason = "ReadyNotYetReported"
 
-	// MachineAvailableV1Beta2Reason surfaces when a machine ready for at least MinReadySeconds.
+	// MachineAvailableV1Beta2Reason surfaces when a machine is ready for at least MinReadySeconds.
+	// Note: MinReadySeconds is assumed 0 until it will be implemented in v1beta2 API.
 	MachineAvailableV1Beta2Reason = "MachineAvailable"
 )
 
 // Machine's Ready condition and corresponding reasons that will be used in v1Beta2 API version.
 // Note: when possible, Ready condition will use reasons from the conditions it summarizes.
 const (
-	// MachineReadyV1Beta2Condition is true if the Machine is not deleted, Machine's BootstrapConfigReady, InfrastructureReady,
-	// NodeHealthy and HealthCheckSucceeded (if present) are true; if other conditions are defined in spec.readinessGates,
+	// MachineReadyV1Beta2Condition is true if the Machine's deletionTimestamp is not set, Machine's BootstrapConfigReady, InfrastructureReady,
+	// NodeHealthy and HealthCheckSucceeded (if present) conditions are true; if other conditions are defined in spec.readinessGates,
 	// these conditions must be true as well.
 	MachineReadyV1Beta2Condition = ReadyV1Beta2Condition
 
@@ -132,12 +134,9 @@ const (
 	// MachineBootstrapConfigReadyV1Beta2Condition condition mirrors the corresponding Ready condition from the Machine's BootstrapConfig resource.
 	MachineBootstrapConfigReadyV1Beta2Condition = BootstrapConfigReadyV1Beta2Condition
 
-	// MachineBootstrapDataSecretDataSecretUserProvidedV1Beta2Reason surfaces when a bootstrap data secret is provided by the user (without a ConfigRef).
-	MachineBootstrapDataSecretDataSecretUserProvidedV1Beta2Reason = "DataSecretUserProvided"
-
-	// MachineBootstrapInvalidConfigV1Beta2Reason surfaces when Machine's spec.bootstrap doesn't have configRef nor a
-	// dataSecretName set.
-	MachineBootstrapInvalidConfigV1Beta2Reason = "InvalidConfig"
+	// MachineBootstrapDataSecretProvidedV1Beta2Reason surfaces when a bootstrap data secret is provided (not originated
+	// from a BoostrapConfig object referenced from the machine).
+	MachineBootstrapDataSecretProvidedV1Beta2Reason = "DataSecretProvided"
 
 	// MachineBootstrapConfigInvalidConditionReportedV1Beta2Reason surfaces a BootstrapConfig Ready condition (read from a bootstrap config object) which is invalid.
 	// (e.g. it is status is missing).
@@ -150,13 +149,13 @@ const (
 	MachineBootstrapConfigInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason
 
 	// MachineBootstrapConfigDoesNotExistV1Beta2Reason surfaces when a referenced bootstrap config object does not exist.
-	// Note: this could happen when creating the machine. However, this state should be treated as an error if it last indefinitely.
-	MachineBootstrapConfigDoesNotExistV1Beta2Reason = RefObjectDoesNotExistV1Beta2Reason
+	// Note: this could happen when creating the machine. However, this state should be treated as an error if it lasts indefinitely.
+	MachineBootstrapConfigDoesNotExistV1Beta2Reason = ObjectDoesNotExistV1Beta2Reason
 
 	// MachineBootstrapConfigDeletedV1Beta2Reason surfaces when a referenced bootstrap config object has been deleted.
-	// Note: controllers can't identify if the deletion process has been initiated by the controller itself, e.g.
+	// Note: controllers can't identify if the bootstrap config object was deleted the controller itself, e.g.
 	// during the deletion workflow, or by a users.
-	MachineBootstrapConfigDeletedV1Beta2Reason = RefObjectDeletedV1Beta2Reason
+	MachineBootstrapConfigDeletedV1Beta2Reason = ObjectDeletedV1Beta2Reason
 )
 
 // Machine's InfrastructureReady condition and corresponding reasons that will be used in v1Beta2 API version.
@@ -172,17 +171,17 @@ const (
 	// MachineInfrastructureReadyNoV1Beta2ReasonReported applies to a infrastructure Ready condition (read from an infra machine object) that reports no reason.
 	MachineInfrastructureReadyNoV1Beta2ReasonReported = NoV1Beta2ReasonReported
 
-	// MachineInfrastructureInternalErrorV1Beta2Reason surfaces unexpected failures when reading a BootstrapConfig object.
+	// MachineInfrastructureInternalErrorV1Beta2Reason surfaces unexpected failures when reading a infra machine object.
 	MachineInfrastructureInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason
 
 	// MachineInfrastructureDoesNotExistV1Beta2Reason surfaces when a referenced infrastructure object does not exist.
-	// Note: this could happen when creating the machine. However, this state should be treated as an error if it last indefinitely.
-	MachineInfrastructureDoesNotExistV1Beta2Reason = RefObjectDoesNotExistV1Beta2Reason
+	// Note: this could happen when creating the machine. However, this state should be treated as an error if it lasts indefinitely.
+	MachineInfrastructureDoesNotExistV1Beta2Reason = ObjectDoesNotExistV1Beta2Reason
 
 	// MachineInfrastructureDeletedV1Beta2Reason surfaces when a referenced infrastructure object has been deleted.
-	// Note: controllers can't identify if the deletion process has been initiated by the controller itself, e.g.
+	// Note: controllers can't identify if the infrastructure object was deleted by the controller itself, e.g.
 	// during the deletion workflow, or by a users.
-	MachineInfrastructureDeletedV1Beta2Reason = RefObjectDeletedV1Beta2Reason
+	MachineInfrastructureDeletedV1Beta2Reason = ObjectDeletedV1Beta2Reason
 )
 
 // Machine's NodeHealthy and NodeReady conditions and corresponding reasons that will be used in v1Beta2 API version.
@@ -198,11 +197,11 @@ const (
 	MachineNodeConditionNotYetReportedV1Beta2Reason = "NodeConditionNotYetReported"
 
 	// MachineNodeNotFoundV1Beta2Reason surfaces when the node hosted on the machine cannot be found.
-	// Note: this could happen when creating the machine. However, this state should be treated as an error if it last indefinitely.
+	// Note: this could happen when creating the machine. However, this state should be treated as an error if it lasts indefinitely.
 	MachineNodeNotFoundV1Beta2Reason = "NodeNotFound"
 
 	// MachineNodeDeletedV1Beta2Reason surfaces when  the node hosted on the machine has been deleted.
-	// Note: controllers can't identify if the deletion process has been initiated by the controller itself, e.g.
+	// Note: controllers can't identify if the Node was deleted by the controller itself, e.g.
 	// during the deletion workflow, or by a users.
 	MachineNodeDeletedV1Beta2Reason = "NodeDeleted"
 )

api/v1beta1/v1beta2_condition_consts.go

@@ -99,13 +99,13 @@ const (
 	// In most cases, it will be required to look at controllers logs to proper triage those issues.
 	InternalErrorV1Beta2Reason = "InternalError"
 
-	// RefObjectDoesNotExistV1Beta2Reason surfaces when a referenced object does not exist.
-	RefObjectDoesNotExistV1Beta2Reason = "RefObjectDoesNotExist"
+	// ObjectDoesNotExistV1Beta2Reason surfaces when a referenced object does not exist.
+	ObjectDoesNotExistV1Beta2Reason = "ObjectDoesNotExist"
 
-	// RefObjectDeletedV1Beta2Reason surfaces when a referenced object has been deleted.
-	// Note: controllers can't identify if the deletion process has been initiated by the controller itself, e.g.
+	// ObjectDeletedV1Beta2Reason surfaces when a referenced object has been deleted.
+	// Note: controllers can't identify if the object was deleted by the controller itself, e.g.
 	// during the deletion workflow, or by a users.
-	RefObjectDeletedV1Beta2Reason = "RefObjectDeleted"
+	ObjectDeletedV1Beta2Reason = "ObjectDeleted"
 
 	// NotPausedV1Beta2Reason surfaces when an object is not paused.
 	NotPausedV1Beta2Reason = "NotPaused"

internal/controllers/machine/machine_controller.go

@@ -345,22 +345,22 @@ type scope struct {
 	// Machine. It is set after reconcileInfrastructure is called.
 	infraMachine *unstructured.Unstructured
 
-	// infraMachineNotFound is true if getting the infra machine object failed with an IsNotFound err
+	// infraMachineNotFound is true if getting the infra machine object failed with an NotFound err
 	infraMachineIsNotFound bool
 
 	// bootstrapConfig is the BootstrapConfig object that is referenced by the
 	// Machine. It is set after reconcileBootstrap is called.
 	bootstrapConfig *unstructured.Unstructured
 
-	// bootstrapConfigNotFound is true if getting the BootstrapConfig object failed with an IsNotFound err
+	// bootstrapConfigNotFound is true if getting the BootstrapConfig object failed with an NotFound err
 	bootstrapConfigIsNotFound bool
 
 	// node is the Kubernetes node hosted on the machine.
 	node *corev1.Node
 }
 
 func (r *Reconciler) reconcileMachineOwnerAndLabels(_ context.Context, s *scope) (ctrl.Result, error) {
-	// If the machine is a stand-alone one, meaning not originated from a MachineDeployment, then set it as directly
+	// If the machine is a stand-alone Machine, then set it as directly
 	// owned by the Cluster (if not already present).
 	if r.shouldAdopt(s.machine) {
 		s.machine.SetOwnerReferences(util.EnsureOwnerRef(s.machine.GetOwnerReferences(), metav1.OwnerReference{
@@ -882,7 +882,7 @@ func (r *Reconciler) reconcileDeleteBootstrap(ctx context.Context, s *scope) (bo
 		if err := r.Client.Delete(ctx, s.bootstrapConfig); err != nil && !apierrors.IsNotFound(err) {
 			return false, errors.Wrapf(err,
 				"failed to delete %v %q for Machine %q in namespace %q",
-				s.bootstrapConfig.GroupVersionKind(), s.bootstrapConfig.GetName(), s.machine.Name, s.machine.Namespace)
+				s.bootstrapConfig.GroupVersionKind().Kind, s.bootstrapConfig.GetName(), s.machine.Name, s.machine.Namespace)
 		}
 	}
 
@@ -899,7 +899,7 @@ func (r *Reconciler) reconcileDeleteInfrastructure(ctx context.Context, s *scope
 		if err := r.Client.Delete(ctx, s.infraMachine); err != nil && !apierrors.IsNotFound(err) {
 			return false, errors.Wrapf(err,
 				"failed to delete %v %q for Machine %q in namespace %q",
-				s.infraMachine.GroupVersionKind(), s.infraMachine.GetName(), s.machine.Name, s.machine.Namespace)
+				s.infraMachine.GroupVersionKind().Kind, s.infraMachine.GetName(), s.machine.Name, s.machine.Namespace)
 		}
 	}
 

internal/controllers/machine/machine_controller_status.go

@@ -86,21 +86,10 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, s *scope) {
 
 func setBootstrapReadyCondition(_ context.Context, machine *clusterv1.Machine, bootstrapConfig *unstructured.Unstructured, bootstrapConfigIsNotFound bool) {
 	if machine.Spec.Bootstrap.ConfigRef == nil {
-		if ptr.Deref(machine.Spec.Bootstrap.DataSecretName, "") != "" {
-			v1beta2conditions.Set(machine, metav1.Condition{
-				Type:   clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
-				Status: metav1.ConditionTrue,
-				Reason: clusterv1.MachineBootstrapDataSecretDataSecretUserProvidedV1Beta2Reason,
-			})
-			return
-		}
-
-		// Note: validation web hooks should prevent invalid configuration to happen.
 		v1beta2conditions.Set(machine, metav1.Condition{
-			Type:    clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
-			Status:  metav1.ConditionFalse,
-			Reason:  clusterv1.MachineBootstrapInvalidConfigV1Beta2Reason,
-			Message: "Either spec.bootstrap.configRef must be set or spec.bootstrap.dataSecretName must not be empty",
+			Type:   clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
+			Status: metav1.ConditionTrue,
+			Reason: clusterv1.MachineBootstrapDataSecretProvidedV1Beta2Reason,
 		})
 		return
 	}

internal/controllers/machine/machine_controller_status_test.go

@@ -80,31 +80,7 @@ func TestSetBootstrapReadyCondition(t *testing.T) {
 			expectCondition: metav1.Condition{
 				Type:   clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
 				Status: metav1.ConditionTrue,
-				Reason: clusterv1.MachineBootstrapDataSecretDataSecretUserProvidedV1Beta2Reason,
-			},
-		},
-		{
-			name: "machine without bootstrap config ref and with dataSecretName not set",
-			machine: func() *clusterv1.Machine {
-				m := defaultMachine.DeepCopy()
-				m.Spec.Bootstrap.ConfigRef = nil
-				return m
-			}(),
-			bootstrapConfig: &unstructured.Unstructured{Object: map[string]interface{}{
-				"kind":       "GenericBootstrapConfig",
-				"apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1",
-				"metadata": map[string]interface{}{
-					"name":      "bootstrap-config1",
-					"namespace": metav1.NamespaceDefault,
-				},
-				"status": map[string]interface{}{},
-			}},
-			bootstrapConfigIsNotFound: false,
-			expectCondition: metav1.Condition{
-				Type:    clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,
-				Status:  metav1.ConditionFalse,
-				Reason:  clusterv1.MachineBootstrapInvalidConfigV1Beta2Reason,
-				Message: "Either spec.bootstrap.configRef must be set or spec.bootstrap.dataSecretName must not be empty",
+				Reason: clusterv1.MachineBootstrapDataSecretProvidedV1Beta2Reason,
 			},
 		},
 		{

internal/webhooks/machine_test.go

@@ -69,6 +69,11 @@ func TestMachineBootstrapValidation(t *testing.T) {
 			bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: ptr.To("test")},
 			expectErr: false,
 		},
+		{
+			name:      "should not return error if dataSecretName is set",
+			bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: ptr.To("")},
+			expectErr: false,
+		},
 		{
 			name:      "should not return error if config ref is set",
 			bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}, DataSecretName: nil},

util/conditions/v1beta2/getter.go

@@ -177,16 +177,8 @@ func validateAndFixConvertedCondition(c *metav1.Condition) error {
 	default:
 		return errors.Errorf("status for the %s condition must be one of %s, %s, %s", c.Type, metav1.ConditionTrue, metav1.ConditionFalse, metav1.ConditionUnknown)
 	}
-
 	if c.Reason == "" {
-		switch c.Status {
-		case metav1.ConditionFalse: // When using old Cluster API condition utils, for conditions with Status false, Reason can be empty only when a condition has negative polarity (means "good")
-			c.Reason = NoReasonReported
-		case metav1.ConditionTrue: // When using old Cluster API condition utils, for conditions with Status true, Reason can be empty only when a condition has positive polarity (means "good").
-			c.Reason = NoReasonReported
-		case metav1.ConditionUnknown:
-			return errors.Errorf("reason must be set for the %s condition, status unknown", c.Type)
-		}
+		c.Reason = NoReasonReported
 	}
 
 	// NOTE: Empty LastTransitionTime is tolerated because it will be set when assigning the newly generated mirror condition to an object.