diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 004ddc4f9d35d..a2a7ac7ffeee8 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -126,6 +126,11 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } + if err := r.migrateStatus(ctx, &applicationSetInfo); err != nil { + logCtx.Errorf("failed to migrate status subresource %v", err) + return ctrl.Result{}, err + } + // Log a warning if there are unrecognized generators _ = utils.CheckInvalidGenerators(&applicationSetInfo) // desiredApplications is the main list of all expected Applications from all generators in this appset. @@ -1151,6 +1156,13 @@ func (r *ApplicationSetReconciler) updateApplicationSetApplicationStatus(ctx con } else { // we have an existing AppStatus currentAppStatus = applicationSet.Status.ApplicationStatus[idx] + + // upgrade any existing AppStatus that might have been set by an older argo-cd version + // note: currentAppStatus.TargetRevisions may be set to empty list earlier during migrations, + // to prevent other usage of r.Client.Status().Update to fail before reaching here. + if currentAppStatus.TargetRevisions == nil || len(currentAppStatus.TargetRevisions) == 0 { + currentAppStatus.TargetRevisions = app.Status.GetRevisions() + } } appOutdated := false @@ -1348,6 +1360,27 @@ func findApplicationStatusIndex(appStatuses []argov1alpha1.ApplicationSetApplica return -1 } +// migrateStatus run migrations on the status subresource of ApplicationSet early during the run of ApplicationSetReconciler.Reconcile +// this handles any defaulting of values - which would otherwise cause the references to r.Client.Status().Update to fail given missing required fields. +func (r *ApplicationSetReconciler) migrateStatus(ctx context.Context, appset *argov1alpha1.ApplicationSet) error { + update := false + if statusList := appset.Status.ApplicationStatus; statusList != nil { + for idx := range statusList { + if statusList[idx].TargetRevisions == nil { + statusList[idx].TargetRevisions = []string{} + update = true + } + } + } + + if update { + if err := r.Client.Status().Update(ctx, appset); err != nil { + return fmt.Errorf("unable to set application set status: %w", err) + } + } + return nil +} + func (r *ApplicationSetReconciler) updateResourcesStatus(ctx context.Context, logCtx *log.Entry, appset *argov1alpha1.ApplicationSet, apps []argov1alpha1.Application) error { statusMap := getResourceStatusMap(appset) statusMap = buildResourceStatus(statusMap, apps) diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 6930e9ef50bb6..5f11b9a16b8ce 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -4761,6 +4761,58 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, }, }, + { + name: "handles an outdated list of statuses with a healthy application, setting required variables", + appSet: v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "argocd", + }, + Spec: v1alpha1.ApplicationSetSpec{ + Strategy: &v1alpha1.ApplicationSetStrategy{ + Type: "RollingSync", + RollingSync: &v1alpha1.ApplicationSetRolloutStrategy{}, + }, + }, + Status: v1alpha1.ApplicationSetStatus{ + ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ + { + Application: "app1", + Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", + Status: "Healthy", + Step: "1", + }, + }, + }, + }, + apps: []v1alpha1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "app1", + }, + Status: v1alpha1.ApplicationStatus{ + Health: v1alpha1.HealthStatus{ + Status: health.HealthStatusHealthy, + }, + OperationState: &v1alpha1.OperationState{ + Phase: common.OperationSucceeded, + }, + Sync: v1alpha1.SyncStatus{ + Status: v1alpha1.SyncStatusCodeSynced, + }, + }, + }, + }, + expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ + { + Application: "app1", + Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", + Status: "Healthy", + Step: "1", + TargetRevisions: []string{}, + }, + }, + }, { name: "progresses an OutOfSync RollingSync application to waiting", appSet: v1alpha1.ApplicationSet{ @@ -4850,10 +4902,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "", - Status: "Pending", - Step: "1", + Application: "app1", + Message: "", + Status: "Pending", + Step: "1", + TargetRevisions: []string{"Next"}, }, }, }, @@ -4872,15 +4925,16 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application resource became Progressing, updating status from Pending to Progressing.", - Status: "Progressing", - Step: "1", + Application: "app1", + Message: "Application resource became Progressing, updating status from Pending to Progressing.", + Status: "Progressing", + Step: "1", + TargetRevisions: []string{"Next"}, }, }, }, { - name: "progresses a pending syncing application to progressing", + name: "progresses a pending synced application to progressing", appSet: v1alpha1.ApplicationSet{ ObjectMeta: metav1.ObjectMeta{ Name: "name", @@ -4895,10 +4949,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "", - Status: "Pending", - Step: "1", + Application: "app1", + Message: "", + Status: "Pending", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -4923,10 +4978,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application resource became Progressing, updating status from Pending to Progressing.", - Status: "Progressing", - Step: "1", + Application: "app1", + Message: "Application resource became Progressing, updating status from Pending to Progressing.", + Status: "Progressing", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -4946,10 +5002,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "", - Status: "Progressing", - Step: "1", + Application: "app1", + Message: "", + Status: "Progressing", + Step: "1", + TargetRevisions: []string{"Next"}, }, }, }, @@ -4974,10 +5031,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application resource became Healthy, updating status from Progressing to Healthy.", - Status: "Healthy", - Step: "1", + Application: "app1", + Message: "Application resource became Healthy, updating status from Progressing to Healthy.", + Status: "Healthy", + Step: "1", + TargetRevisions: []string{"Next"}, }, }, }, @@ -4997,10 +5055,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "", - Status: "Waiting", - Step: "1", + Application: "app1", + Message: "", + Status: "Waiting", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -5025,10 +5084,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", - Status: "Healthy", - Step: "1", + Application: "app1", + Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", + Status: "Healthy", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -5304,16 +5364,18 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application has pending changes, setting status to Waiting.", - Status: "Waiting", - Step: "1", + Application: "app1", + Message: "Application has pending changes, setting status to Waiting.", + Status: "Waiting", + Step: "1", + TargetRevisions: []string{"Current"}, }, { - Application: "app2", - Message: "Application has pending changes, setting status to Waiting.", - Status: "Waiting", - Step: "1", + Application: "app2", + Message: "Application has pending changes, setting status to Waiting.", + Status: "Waiting", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -5338,10 +5400,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) { }, expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", - Status: "Healthy", - Step: "1", + Application: "app1", + Message: "Application resource is already Healthy, updating status from Waiting to Healthy.", + Status: "Healthy", + Step: "1", + TargetRevisions: []string{"Current"}, }, }, }, @@ -5503,9 +5566,10 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { Status: v1alpha1.ApplicationSetStatus{ ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ { - Application: "app1", - Message: "Application is out of date with the current AppSet generation, setting status to Waiting.", - Status: "Waiting", + Application: "app1", + Message: "Application is out of date with the current AppSet generation, setting status to Waiting.", + Status: "Waiting", + TargetRevisions: []string{"Next"}, }, }, }, @@ -5523,6 +5587,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) { Message: "Application moved to Pending status, watching for the Application resource to start Progressing.", Status: "Pending", Step: "1", + TargetRevisions: []string{"Next"}, }, }, }, @@ -6529,3 +6594,74 @@ func TestOwnsHandler(t *testing.T) { }) } } + +func TestMigrateStatus(t *testing.T) { + scheme := runtime.NewScheme() + err := v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + + err = v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + + for _, tc := range []struct { + name string + appset v1alpha1.ApplicationSet + expectedStatus v1alpha1.ApplicationSetStatus + }{ + { + name: "status without applicationstatus target revisions set will default to empty list", + appset: v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + Status: v1alpha1.ApplicationSetStatus{ + ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ + {}, + }, + }, + }, + expectedStatus: v1alpha1.ApplicationSetStatus{ + ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ + { + TargetRevisions: []string{}, + }, + }, + }, + }, + { + name: "status with applicationstatus target revisions set will do nothing", + appset: v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + Status: v1alpha1.ApplicationSetStatus{ + ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ + { + TargetRevisions: []string{"Current"}, + }, + }, + }, + }, + expectedStatus: v1alpha1.ApplicationSetStatus{ + ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{ + { + TargetRevisions: []string{"Current"}, + }, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&tc.appset).WithObjects(&tc.appset).Build() + r := ApplicationSetReconciler{ + Client: client, + } + + err := r.migrateStatus(context.Background(), &tc.appset) + require.NoError(t, err) + assert.Equal(t, tc.expectedStatus, tc.appset.Status) + }) + } +} diff --git a/util/argo/resource_tracking.go b/util/argo/resource_tracking.go index f6b98683e9e91..476f739c13924 100644 --- a/util/argo/resource_tracking.go +++ b/util/argo/resource_tracking.go @@ -149,6 +149,7 @@ func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, v if err != nil { return fmt.Errorf("failed to set app instance label: %w", err) } + return nil case TrackingMethodAnnotation: return setAppInstanceAnnotation() case TrackingMethodAnnotationAndLabel: @@ -171,13 +172,14 @@ func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, v if err != nil { return fmt.Errorf("failed to set app instance label: %w", err) } + return nil default: err := argokube.SetAppInstanceLabel(un, key, val) if err != nil { return fmt.Errorf("failed to set app instance label: %w", err) } + return nil } - return nil } // BuildAppInstanceValue build resource tracking id in format ;/// diff --git a/util/kube/kube.go b/util/kube/kube.go index e32aabac00b63..5ea4394b726f0 100644 --- a/util/kube/kube.go +++ b/util/kube/kube.go @@ -4,7 +4,11 @@ import ( "fmt" "regexp" + "github.com/argoproj/gitops-engine/pkg/utils/kube" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/argoproj/argo-cd/v2/common" ) var resourceNamePattern = regexp.MustCompile("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$") @@ -15,6 +19,7 @@ func IsValidResourceName(name string) bool { } // SetAppInstanceLabel the recommended app.kubernetes.io/instance label against an unstructured object +// Uses the legacy labeling if environment variable is set func SetAppInstanceLabel(target *unstructured.Unstructured, key, val string) error { labels, _, err := nestedNullableStringMap(target.Object, "metadata", "labels") if err != nil { @@ -25,10 +30,75 @@ func SetAppInstanceLabel(target *unstructured.Unstructured, key, val string) err } labels[key] = val target.SetLabels(labels) + if key != common.LabelKeyLegacyApplicationName { + // we no longer label the pod template sub resources in v0.11 + return nil + } + + gvk := schema.FromAPIVersionAndKind(target.GetAPIVersion(), target.GetKind()) + // special case for deployment and job types: make sure that derived replicaset, and pod has + // the application label + switch gvk.Group { + case "apps", "extensions": + switch gvk.Kind { + case kube.DeploymentKind, kube.ReplicaSetKind, kube.StatefulSetKind, kube.DaemonSetKind: + templateLabels, ok, err := unstructured.NestedMap(target.UnstructuredContent(), "spec", "template", "metadata", "labels") + if err != nil { + return err + } + if !ok || templateLabels == nil { + templateLabels = make(map[string]interface{}) + } + templateLabels[key] = val + err = unstructured.SetNestedMap(target.UnstructuredContent(), templateLabels, "spec", "template", "metadata", "labels") + if err != nil { + return err + } + // The following is a workaround for issue #335. In API version extensions/v1beta1 or + // apps/v1beta1, if a spec omits spec.selector then k8s will default the + // spec.selector.matchLabels to match spec.template.metadata.labels. This means Argo CD + // labels can potentially make their way into spec.selector.matchLabels, which is a bad + // thing. The following logic prevents this behavior. + switch target.GetAPIVersion() { + case "apps/v1beta1", "extensions/v1beta1": + selector, _, err := unstructured.NestedMap(target.UnstructuredContent(), "spec", "selector") + if err != nil { + return err + } + if len(selector) == 0 { + // If we get here, user did not set spec.selector in their manifest. We do not want + // our Argo CD labels to get defaulted by kubernetes, so we explicitly set the labels + // for them (minus the Argo CD labels). + delete(templateLabels, key) + err = unstructured.SetNestedMap(target.UnstructuredContent(), templateLabels, "spec", "selector", "matchLabels") + if err != nil { + return err + } + } + } + } + case "batch": + switch gvk.Kind { + case kube.JobKind: + templateLabels, ok, err := unstructured.NestedMap(target.UnstructuredContent(), "spec", "template", "metadata", "labels") + if err != nil { + return err + } + if !ok || templateLabels == nil { + templateLabels = make(map[string]interface{}) + } + templateLabels[key] = val + err = unstructured.SetNestedMap(target.UnstructuredContent(), templateLabels, "spec", "template", "metadata", "labels") + if err != nil { + return err + } + } + } return nil } // SetAppInstanceAnnotation the recommended app.kubernetes.io/instance annotation against an unstructured object +// Uses the legacy labeling if environment variable is set func SetAppInstanceAnnotation(target *unstructured.Unstructured, key, val string) error { annotations, _, err := nestedNullableStringMap(target.Object, "metadata", "annotations") if err != nil { diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go index 389a2d0f9870a..51474359bde5e 100644 --- a/util/kube/kube_test.go +++ b/util/kube/kube_test.go @@ -84,6 +84,56 @@ func TestSetLabels(t *testing.T) { } } +func TestSetLegacyLabels(t *testing.T) { + for _, yamlStr := range []string{depWithoutSelector, depWithSelector} { + var obj unstructured.Unstructured + err := yaml.Unmarshal([]byte(yamlStr), &obj) + require.NoError(t, err) + + err = SetAppInstanceLabel(&obj, common.LabelKeyLegacyApplicationName, "my-app") + require.NoError(t, err) + + manifestBytes, err := json.MarshalIndent(obj.Object, "", " ") + require.NoError(t, err) + log.Println(string(manifestBytes)) + + var depV1Beta1 extv1beta1.Deployment + err = json.Unmarshal(manifestBytes, &depV1Beta1) + require.NoError(t, err) + assert.Len(t, depV1Beta1.Spec.Selector.MatchLabels, 1) + assert.Equal(t, "nginx", depV1Beta1.Spec.Selector.MatchLabels["app"]) + assert.Len(t, depV1Beta1.Spec.Template.Labels, 2) + assert.Equal(t, "nginx", depV1Beta1.Spec.Template.Labels["app"]) + assert.Equal(t, "my-app", depV1Beta1.Spec.Template.Labels[common.LabelKeyLegacyApplicationName]) + } +} + +func TestSetLegacyJobLabel(t *testing.T) { + yamlBytes, err := os.ReadFile("testdata/job.yaml") + require.NoError(t, err) + var obj unstructured.Unstructured + err = yaml.Unmarshal(yamlBytes, &obj) + require.NoError(t, err) + err = SetAppInstanceLabel(&obj, common.LabelKeyLegacyApplicationName, "my-app") + require.NoError(t, err) + + manifestBytes, err := json.MarshalIndent(obj.Object, "", " ") + require.NoError(t, err) + log.Println(string(manifestBytes)) + + job := unstructured.Unstructured{} + err = json.Unmarshal(manifestBytes, &job) + require.NoError(t, err) + + labels := job.GetLabels() + assert.Equal(t, "my-app", labels[common.LabelKeyLegacyApplicationName]) + + templateLabels, ok, err := unstructured.NestedMap(job.UnstructuredContent(), "spec", "template", "metadata", "labels") + assert.True(t, ok) + require.NoError(t, err) + assert.Equal(t, "my-app", templateLabels[common.LabelKeyLegacyApplicationName]) +} + func TestSetSvcLabel(t *testing.T) { yamlBytes, err := os.ReadFile("testdata/svc.yaml") require.NoError(t, err) diff --git a/util/settings/settings.go b/util/settings/settings.go index 1f036ddeff7c6..8cb1c4df956f5 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -761,11 +761,6 @@ func (mgr *SettingsManager) GetAppInstanceLabelKey() (string, error) { if label == "" { return common.LabelKeyAppInstance, nil } - // return new label key if user is still using legacy key - if label == common.LabelKeyLegacyApplicationName { - log.Warnf("deprecated legacy application instance tracking key(%v) is present in configmap, new key(%v) will be used automatically", common.LabelKeyLegacyApplicationName, common.LabelKeyAppInstance) - return common.LabelKeyAppInstance, nil - } return label, nil }