fix: Set GCPMachinepoolMachine.Status.LatestModelApplied to false if some labels applied to node are not same as expected

api/v1beta1/gcpcluster_webhook.go

@@ -41,8 +41,10 @@ func (c *GCPCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
 // +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-gcpcluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=gcpclusters,versions=v1beta1,name=validation.gcpcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
 // +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-gcpcluster,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=gcpclusters,versions=v1beta1,name=default.gcpcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
 
-var _ webhook.Validator = &GCPCluster{}
-var _ webhook.Defaulter = &GCPCluster{}
+var (
+	_ webhook.Validator = &GCPCluster{}
+	_ webhook.Defaulter = &GCPCluster{}
+)
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type.
 func (c *GCPCluster) Default() {

api/v1beta1/gcpclustertemplate_webhook_test.go

@@ -39,7 +39,8 @@ func TestGCPClusterTemplate_ValidateUpdate(t *testing.T) {
 						Spec: GCPClusterSpec{
 							Project: "test-gcp-cluster",
 							Region:  "ap-south-1",
-						}},
+						},
+					},
 				},
 			},
 			oldTemplate: &GCPClusterTemplate{
@@ -48,7 +49,8 @@ func TestGCPClusterTemplate_ValidateUpdate(t *testing.T) {
 						Spec: GCPClusterSpec{
 							Project: "test-gcp-cluster",
 							Region:  "ap-south-1",
-						}},
+						},
+					},
 				},
 			},
 			wantErr: false,
@@ -61,7 +63,8 @@ func TestGCPClusterTemplate_ValidateUpdate(t *testing.T) {
 						Spec: GCPClusterSpec{
 							Project: "test-gcp-cluster",
 							Region:  "ap-south-1",
-						}},
+						},
+					},
 				},
 			},
 			oldTemplate: &GCPClusterTemplate{
@@ -70,7 +73,8 @@ func TestGCPClusterTemplate_ValidateUpdate(t *testing.T) {
 						Spec: GCPClusterSpec{
 							Project: "test-gcp-cluster",
 							Region:  "ap-east-1",
-						}},
+						},
+					},
 				},
 			},
 			wantErr: true,

api/v1beta1/gcpmachinetemplate_webhook_test.go

@@ -40,7 +40,8 @@ func TestGCPMachineTemplate_ValidateCreate(t *testing.T) {
 						Spec: GCPMachineSpec{
 							InstanceType:      "n2d-standard-4",
 							OnHostMaintenance: &onHostMaintenanceTerminate,
-						}},
+						},
+					},
 				},
 			},
 			wantErr: false,
@@ -54,7 +55,8 @@ func TestGCPMachineTemplate_ValidateCreate(t *testing.T) {
 							InstanceType:        "n2d-standard-4",
 							ConfidentialCompute: &confidentialComputeEnabled,
 							OnHostMaintenance:   &onHostMaintenanceTerminate,
-						}},
+						},
+					},
 				},
 			},
 			wantErr: false,
@@ -68,7 +70,8 @@ func TestGCPMachineTemplate_ValidateCreate(t *testing.T) {
 							InstanceType:        "n2d-standard-4",
 							ConfidentialCompute: &confidentialComputeEnabled,
 							OnHostMaintenance:   &onHostMaintenanceMigrate,
-						}},
+						},
+					},
 				},
 			},
 			wantErr: true,
@@ -81,7 +84,8 @@ func TestGCPMachineTemplate_ValidateCreate(t *testing.T) {
 						Spec: GCPMachineSpec{
 							InstanceType:        "n2d-standard-4",
 							ConfidentialCompute: &confidentialComputeEnabled,
-						}},
+						},
+					},
 				},
 			},
 			wantErr: true,
@@ -95,7 +99,8 @@ func TestGCPMachineTemplate_ValidateCreate(t *testing.T) {
 							InstanceType:        "e2-standard-4",
 							ConfidentialCompute: &confidentialComputeEnabled,
 							OnHostMaintenance:   &onHostMaintenanceTerminate,
-						}},
+						},
+					},
 				},
 			},
 			wantErr: true,

cloud/scope/machinepool_test.go

@@ -1,7 +1,6 @@
 package scope_test
 
 import (
-	"fmt"
 	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -52,14 +51,13 @@ var _ = Describe("GCPManagedMachinePool Scope", func() {
 		// Make sure the machinepool scope is created correctly.
 		assert.Nil(t, err)
 		assert.NotNil(t, TestMachinePoolScope)
-
 	})
 
 	Describe("Min CPU Mappings", func() {
 		Context("instance types", func() {
 			It("should match all", func() {
 				for k := range processors.Processors {
-					TestMachinePoolScope.GCPMachinePool.Spec.InstanceType = fmt.Sprintf("%sstandard-8", k)
+					TestMachinePoolScope.GCPMachinePool.Spec.InstanceType = k + "standard-8"
 					Expect(TestMachinePoolScope.MinCPUPlatform()).To(Equal(processors.Processors[k]))
 				}
 			})

cloud/scope/machinepoolmachine.go

@@ -302,8 +302,10 @@ func (m *MachinePoolMachineScope) ProviderID() string {
 }
 
 // HasLatestModelApplied checks if the latest model is applied to the GCPMachinePoolMachine.
-func (m *MachinePoolMachineScope) HasLatestModelApplied(_ context.Context, instance *compute.Disk) (bool, error) {
-	image := ""
+func (m *MachinePoolMachineScope) HasLatestModelApplied(ctx context.Context, instance *compute.Disk, labels map[string]string) (bool, error) {
+	log := log.FromContext(ctx)
+
+	var image string
 
 	if m.GCPMachinePool.Spec.Image == nil {
 		version := ""
@@ -318,18 +320,45 @@ func (m *MachinePoolMachineScope) HasLatestModelApplied(_ context.Context, insta
 	// Get the image from the disk URL path to compare with the latest image name
 	diskImage, err := url.Parse(instance.SourceImage)
 	if err != nil {
+		log.Error(err, "Error Parsing the Instance.SourceImage from the disk attached.")
 		return false, err
 	}
 	instanceImage := path.Base(diskImage.Path)
 
-	// Check if the image is the latest
-	if image == instanceImage {
+	// Check if the image is the latest and additionalLabels
+	hasAdditionalLabelsDiff := m.doesNotHaveAdditionalLabelsDiff(ctx, labels)
+	if image == instanceImage && hasAdditionalLabelsDiff {
+		log.Info("Instance Image and AdditionalLabels are same as the ones specified in GCPMachinePool Spec, setting LatestModelApplied to true", "GCPMachinePoolMachine", m.GCPMachinePoolMachine)
 		return true, nil
 	}
 
+	log.Info("One of either Instance Image and AdditionalLabels are not the same as the ones specified in GCPMachinePool Spec, setting LatestModelApplied to false", "GCPMachinePoolMachine", m.GCPMachinePoolMachine)
 	return false, nil
 }
 
+// doesNotHaveAdditionalLabelsDiff Checks if the Labels applied to the instance are the latest as in the Instance Template.
+// We would need to ignore the two labels
+// - `capg-role`
+// - `capg-cluster-<CLUSTER-NAME>`
+// as they are added by default by CAPG and when we compare it with AdditionalLabels in GCPMachinePool.Spec with the instance present.
+// ref: https://github.com/newrelic-forks/cluster-api-provider-gcp/blob/ef2e7f1e64ebeeb5389c446fe4cf89026fcb8a8a/cloud/services/compute/instances/reconcile_test.go#L244-L24
+func (m *MachinePoolMachineScope) doesNotHaveAdditionalLabelsDiff(ctx context.Context, labels map[string]string) bool {
+	diff := make(map[string]bool)
+	log := log.FromContext(ctx)
+	for _, k := range labels {
+		if _, ok := m.GCPMachinePool.Spec.AdditionalLabels[k]; !ok {
+			diff[k] = true
+		}
+	}
+	_, hasCapgRoleKey := diff["capg-role"]
+	_, hasCapgClusterKey := diff["capg-cluster-"+m.GCPMachinePool.ObjectMeta.Labels["cluster.x-k8s.io/cluster-name"]]
+	if hasCapgRoleKey && hasCapgClusterKey && len(diff) == 2 {
+		log.Info("There's no diff in AdditionalLabels which are present.", "GCPMachinePoolMachine", m.GCPMachinePoolMachine)
+		return false
+	}
+	return true
+}
+
 // CordonAndDrainNode cordon and drain the node for the GCPMachinePoolMachine.
 func (m *MachinePoolMachineScope) CordonAndDrainNode(ctx context.Context) error {
 	log := log.FromContext(ctx)
@@ -423,7 +452,6 @@ func (m *MachinePoolMachineScope) drainNode(ctx context.Context, node *corev1.No
 		Name:      m.ClusterGetter.Name(),
 		Namespace: m.GCPMachinePoolMachine.Namespace,
 	})
-
 	if err != nil {
 		log.Error(err, "Error creating a remote client while deleting Machine, won't retry")
 		return nil

cloud/scope/managedmachinepool_test.go

@@ -11,9 +11,11 @@ import (
 	clusterv1exp "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 )
 
-var TestGCPMMP *v1beta1.GCPManagedMachinePool
-var TestMP *clusterv1exp.MachinePool
-var TestClusterName string
+var (
+	TestGCPMMP      *v1beta1.GCPManagedMachinePool
+	TestMP          *clusterv1exp.MachinePool
+	TestClusterName string
+)
 
 var _ = Describe("GCPManagedMachinePool Scope", func() {
 	BeforeEach(func() {

cloud/services/compute/instancegroupinstances/client.go

@@ -26,7 +26,6 @@ import (
 
 // Client wraps GCP SDK.
 type Client interface {
-
 	// Instance methods.
 	GetInstance(ctx context.Context, project, zone, name string) (*compute.Instance, error)
 	// InstanceGroupInstances methods.

cloud/services/compute/instancegroupinstances/instancegroupinstances.go

@@ -84,7 +84,7 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) {
 	}
 
 	// Update hasLatestModelApplied status.
-	latestModel, err := s.scope.HasLatestModelApplied(ctx, disk)
+	latestModel, err := s.scope.HasLatestModelApplied(ctx, disk, instance.Labels)
 	if err != nil {
 		log.Error(err, "Failed to check if the latest model is applied")
 		return ctrl.Result{}, err

cloud/services/compute/instancegroups/instancegroups.go

@@ -23,6 +23,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/pkg/errors"
+
 	"google.golang.org/api/compute/v1"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/cluster-api-provider-gcp/cloud"
@@ -142,8 +144,7 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) {
 		s.scope.SetMIGState(instanceGroup)
 		s.scope.SetMIGInstances(instanceGroupInstances.ManagedInstances)
 	} else {
-		err = fmt.Errorf("instance group or instance group list is nil")
-		return ctrl.Result{}, gcperrors.UnwrapGCPError(err)
+		return ctrl.Result{}, gcperrors.UnwrapGCPError(errors.New("instance group or instance group list is nil"))
 	}
 	return ctrl.Result{}, nil
 }
@@ -256,6 +257,7 @@ func (s *Service) removeOldInstanceTemplate(ctx context.Context, instanceTemplat
 	// Prepare to identify instance templates to remove.
 	lastIndex := strings.LastIndex(instanceTemplateName, "-")
 	if lastIndex == -1 {
+		//nolint
 		log.Error(fmt.Errorf("invalid instance template name format"), "Invalid template name", "templateName", instanceTemplateName)
 		return fmt.Errorf("invalid instance template name format: %s", instanceTemplateName)
 	}

cloud/services/container/clusters/errors.go

@@ -22,10 +22,8 @@ import (
 	"github.com/pkg/errors"
 )
 
-var (
-	// ErrAutopilotClusterMachinePoolsNotAllowed is used when there are machine pools specified for an autopilot enabled cluster.
-	ErrAutopilotClusterMachinePoolsNotAllowed = errors.New("cannot use machine pools with an autopilot enabled cluster")
-)
+// ErrAutopilotClusterMachinePoolsNotAllowed is used when there are machine pools specified for an autopilot enabled cluster.
+var ErrAutopilotClusterMachinePoolsNotAllowed = errors.New("cannot use machine pools with an autopilot enabled cluster")
 
 // NewErrUnexpectedClusterStatus creates a new error for an unexpected cluster status.
 func NewErrUnexpectedClusterStatus(status string) error {

exp/api/v1beta1/gcpmachinepool_types.go

@@ -87,7 +87,6 @@ type MetadataItem struct {
 
 // GCPMachinePoolSpec defines the desired state of GCPMachinePool and the GCP instances that it will create.
 type GCPMachinePoolSpec struct {
-
 	// AdditionalDisks are optional non-boot attached disks.
 	// +optional
 	AdditionalDisks []AttachedDiskSpec `json:"additionalDisks,omitempty"`
@@ -253,7 +252,6 @@ type MachineRollingUpdateDeployment struct {
 
 // GCPMachinePoolStatus defines the observed state of GCPMachinePool and the GCP instances that it manages.
 type GCPMachinePoolStatus struct {
-
 	// Ready is true when the provider resource is ready.
 	// +optional
 	Ready bool `json:"ready"`
@@ -336,6 +334,7 @@ func (r *GCPMachinePool) GetConditions() clusterv1.Conditions {
 func (r *GCPMachinePool) SetConditions(conditions clusterv1.Conditions) {
 	r.Status.Conditions = conditions
 }
+
 func init() {
 	infrav1.SchemeBuilder.Register(&GCPMachinePool{}, &GCPMachinePoolList{})
 }

exp/api/v1beta1/gcpmachinepoolmachine_types.go

@@ -42,7 +42,6 @@ type GCPMachinePoolMachineSpec struct {
 
 // GCPMachinePoolMachineStatus defines the observed state of GCPMachinePoolMachine and the GCP instances that it manages.
 type GCPMachinePoolMachineStatus struct {
-
 	// NodeRef will point to the corresponding Node if it exists.
 	// +optional
 	NodeRef *corev1.ObjectReference `json:"nodeRef,omitempty"`
@@ -144,6 +143,7 @@ func (r *GCPMachinePoolMachine) GetConditions() clusterv1.Conditions {
 func (r *GCPMachinePoolMachine) SetConditions(conditions clusterv1.Conditions) {
 	r.Status.Conditions = conditions
 }
+
 func init() {
 	infrav1.SchemeBuilder.Register(&GCPMachinePoolMachine{}, &GCPMachinePoolMachineList{})
 }

exp/api/v1beta1/webhook_suite_test.go

@@ -43,11 +43,13 @@ import (
 // These tests use Ginkgo (BDD-style Go testing framework). Refer to
 // http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
 
-var cfg *rest.Config
-var k8sClient client.Client
-var testEnv *envtest.Environment
-var ctx context.Context
-var cancel context.CancelFunc
+var (
+	cfg       *rest.Config
+	k8sClient client.Client
+	testEnv   *envtest.Environment
+	ctx       context.Context
+	cancel    context.CancelFunc
+)
 
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
@@ -132,7 +134,6 @@ var _ = BeforeSuite(func() {
 		conn.Close()
 		return nil
 	}).Should(Succeed())
-
 })
 
 var _ = AfterSuite(func() {

exp/controllers/helpers.go

@@ -137,7 +137,7 @@ func KubeadmConfigToInfrastructureMapFunc(_ context.Context, c client.Client, lo
 
 // GCPMachinePoolMachineMapper returns a handler.ToRequestsFunc that watches for GCPMachinePool events and returns.
 func GCPMachinePoolMachineMapper(scheme *runtime.Scheme, log logr.Logger) handler.MapFunc {
-	return func(ctx context.Context, o client.Object) []ctrl.Request {
+	return func(_ context.Context, o client.Object) []ctrl.Request {
 		gvk, err := apiutil.GVKForObject(new(infrav1exp.GCPMachinePool), scheme)
 		if err != nil {
 			log.Error(errors.WithStack(err), "failed to find GVK for GCPMachinePool")
@@ -204,8 +204,8 @@ func MachinePoolMachineHasStateOrVersionChange(logger logr.Logger) predicate.Fun
 			}
 			return shouldUpdate
 		},
-		CreateFunc:  func(e event.CreateEvent) bool { return false },
-		DeleteFunc:  func(e event.DeleteEvent) bool { return false },
-		GenericFunc: func(e event.GenericEvent) bool { return false },
+		CreateFunc:  func(_ event.CreateEvent) bool { return false },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return false },
+		GenericFunc: func(_ event.GenericEvent) bool { return false },
 	}
 }

test/e2e/capi_test.go

@@ -30,9 +30,7 @@ import (
 )
 
 var _ = Describe("Running the Cluster API E2E tests", func() {
-	var (
-		ctx = context.TODO()
-	)
+	ctx := context.TODO()
 
 	BeforeEach(func() {
 		Expect(e2eConfig.Variables).To(HaveKey(capi_e2e.CNIPath))
@@ -111,5 +109,4 @@ var _ = Describe("Running the Cluster API E2E tests", func() {
 			}
 		})
 	})
-
 })

test/e2e/conformance_test.go

@@ -54,7 +54,7 @@ var _ = Describe("Conformance Tests", func() {
 		Expect(e2eConfig).ToNot(BeNil(), "Invalid argument. e2eConfig can't be nil when calling %s spec", specName)
 		Expect(clusterctlConfigPath).To(BeAnExistingFile(), "Invalid argument. clusterctlConfigPath must be an existing file when calling %s spec", specName)
 		Expect(bootstrapClusterProxy).ToNot(BeNil(), "Invalid argument. bootstrapClusterProxy can't be nil when calling %s spec", specName)
-		Expect(os.MkdirAll(artifactFolder, 0755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
+		Expect(os.MkdirAll(artifactFolder, 0o755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
 		Expect(kubetestConfigFilePath).ToNot(BeNil(), "Invalid argument. kubetestConfigFilePath can't be nil")
 
 		Expect(e2eConfig.Variables).To(HaveKey(KubernetesVersion))

test/e2e/e2e_gke_test.go

@@ -53,7 +53,7 @@ var _ = Describe("GKE workload cluster creation", func() {
 		Expect(e2eConfig).ToNot(BeNil(), "Invalid argument. e2eConfig can't be nil when calling %s spec", specName)
 		Expect(clusterctlConfigPath).To(BeAnExistingFile(), "Invalid argument. clusterctlConfigPath must be an existing file when calling %s spec", specName)
 		Expect(bootstrapClusterProxy).ToNot(BeNil(), "Invalid argument. bootstrapClusterProxy can't be nil when calling %s spec", specName)
-		Expect(os.MkdirAll(artifactFolder, 0755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
+		Expect(os.MkdirAll(artifactFolder, 0o755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
 
 		Expect(e2eConfig.Variables).To(HaveKey(KubernetesVersion))
 

test/e2e/e2e_test.go

@@ -48,7 +48,7 @@ var _ = Describe("Workload cluster creation", func() {
 		Expect(e2eConfig).ToNot(BeNil(), "Invalid argument. e2eConfig can't be nil when calling %s spec", specName)
 		Expect(clusterctlConfigPath).To(BeAnExistingFile(), "Invalid argument. clusterctlConfigPath must be an existing file when calling %s spec", specName)
 		Expect(bootstrapClusterProxy).ToNot(BeNil(), "Invalid argument. bootstrapClusterProxy can't be nil when calling %s spec", specName)
-		Expect(os.MkdirAll(artifactFolder, 0755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
+		Expect(os.MkdirAll(artifactFolder, 0o755)).To(Succeed(), "Invalid argument. artifactFolder can't be created for %s spec", specName)
 
 		Expect(e2eConfig.Variables).To(HaveKey(KubernetesVersion))