From 870d3ba92372c2edbf65936fa2f8b62289e4dbef Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Fri, 19 Jan 2024 13:11:57 +0530
Subject: [PATCH 1/7] Revert "feat: retry with client side dry run if server
one was failed (#548)"
This reverts commit c0c2dd1f6f487c46a25d3d3caaad7fec06667728.
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/sync/sync_context.go | 50 ++++++++++++++++------------------------
1 file changed, 20 insertions(+), 30 deletions(-)
diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go
index c2ed67691..fd03d11b3 100644
--- a/pkg/sync/sync_context.go
+++ b/pkg/sync/sync_context.go
@@ -921,39 +921,29 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, force, validate bool) (c
var err error
var message string
shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace)
- applyFn := func(dryRunStrategy cmdutil.DryRunStrategy) (string, error) {
- if !shouldReplace {
- return sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager, false)
- }
- if t.liveObj == nil {
- return sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate)
- }
- // Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
- // The same thing applies for namespaces, which would delete the namespace as well as everything within it,
- // so we want to avoid using `kubectl replace` in that case as well.
- if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind {
- update := t.targetObj.DeepCopy()
- update.SetResourceVersion(t.liveObj.GetResourceVersion())
- _, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy)
- if err != nil {
- return fmt.Sprintf("error when updating: %v", err.Error()), err
+ if shouldReplace {
+ if t.liveObj != nil {
+ // Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
+ // The same thing applies for namespaces, which would delete the namespace as well as everything within it,
+ // so we want to avoid using `kubectl replace` in that case as well.
+ if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind {
+ update := t.targetObj.DeepCopy()
+ update.SetResourceVersion(t.liveObj.GetResourceVersion())
+ _, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy)
+ if err == nil {
+ message = fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName())
+ } else {
+ message = fmt.Sprintf("error when updating: %v", err.Error())
+ }
+ } else {
+ message, err = sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force)
}
- return fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName()), nil
-
+ } else {
+ message, err = sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate)
}
- return sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force)
- }
-
- message, err = applyFn(dryRunStrategy)
-
- // DryRunServer fails with "Kind does not support fieldValidation" error for kubernetes server < 1.25
- // it fails inside apply.go , o.DryRunVerifier.HasSupport(info.Mapping.GroupVersionKind) line
- // so we retry with DryRunClient that works for all cases, but cause issues with hooks
- // Details: https://github.com/argoproj/argo-cd/issues/16177
- if dryRunStrategy == cmdutil.DryRunServer && err != nil {
- message, err = applyFn(cmdutil.DryRunClient)
+ } else {
+ message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager, false)
}
-
if err != nil {
return common.ResultCodeSyncFailed, err.Error()
}
From e7dc823408ad5fa1a7b7baf2250a044ce3db90c1 Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Fri, 19 Jan 2024 13:12:15 +0530
Subject: [PATCH 2/7] Revert "fix(server): use server side dry run in case if
it is server side apply (#546)"
This reverts commit 4a5648ee411b42f6601782ae6c1c50e0f724c9c5.
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/sync/sync_context.go | 18 +++++-------------
pkg/sync/sync_context_test.go | 20 --------------------
2 files changed, 5 insertions(+), 33 deletions(-)
diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go
index fd03d11b3..0262b512f 100644
--- a/pkg/sync/sync_context.go
+++ b/pkg/sync/sync_context.go
@@ -903,24 +903,16 @@ func (sc *syncContext) ensureCRDReady(name string) error {
})
}
-func getDryRunStrategy(serverSideApply, dryRun bool) cmdutil.DryRunStrategy {
- if !dryRun {
- return cmdutil.DryRunNone
- }
- if serverSideApply {
- return cmdutil.DryRunServer
- }
- return cmdutil.DryRunClient
-}
-
func (sc *syncContext) applyObject(t *syncTask, dryRun, force, validate bool) (common.ResultCode, string) {
- serverSideApply := sc.serverSideApply || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply)
-
- dryRunStrategy := getDryRunStrategy(serverSideApply, dryRun)
+ dryRunStrategy := cmdutil.DryRunNone
+ if dryRun {
+ dryRunStrategy = cmdutil.DryRunClient
+ }
var err error
var message string
shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace)
+ serverSideApply := sc.serverSideApply || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply)
if shouldReplace {
if t.liveObj != nil {
// Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go
index 984787014..0c2e1e14c 100644
--- a/pkg/sync/sync_context_test.go
+++ b/pkg/sync/sync_context_test.go
@@ -5,7 +5,6 @@ import (
"encoding/json"
"errors"
"fmt"
- "k8s.io/kubectl/pkg/cmd/util"
"net/http"
"net/http/httptest"
"reflect"
@@ -540,25 +539,6 @@ func (s *APIServerMock) newHttpServer(t *testing.T, apiFailuresCount int) *httpt
return server
}
-func TestGetDryRunStrategy(t *testing.T) {
- t.Run("no dry run", func(t *testing.T) {
- strategy := getDryRunStrategy(false, false)
- assert.Equal(t, util.DryRunNone, strategy)
- })
- t.Run("no dry run with server side apply", func(t *testing.T) {
- strategy := getDryRunStrategy(true, false)
- assert.Equal(t, util.DryRunNone, strategy)
- })
- t.Run("dry run with server side apply", func(t *testing.T) {
- strategy := getDryRunStrategy(true, true)
- assert.Equal(t, util.DryRunServer, strategy)
- })
- t.Run("dry run with client side apply", func(t *testing.T) {
- strategy := getDryRunStrategy(false, true)
- assert.Equal(t, util.DryRunClient, strategy)
- })
-}
-
func TestServerResourcesRetry(t *testing.T) {
type fixture struct {
apiServerMock *APIServerMock
From 208a954e637fa0c24d0935b6e8ead876d71668b3 Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Fri, 19 Jan 2024 19:31:53 +0530
Subject: [PATCH 3/7] Fixed the logic to disable server side apply if it is a
dry run
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/sync/sync_context.go | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go
index 0262b512f..26d09efcb 100644
--- a/pkg/sync/sync_context.go
+++ b/pkg/sync/sync_context.go
@@ -906,13 +906,18 @@ func (sc *syncContext) ensureCRDReady(name string) error {
func (sc *syncContext) applyObject(t *syncTask, dryRun, force, validate bool) (common.ResultCode, string) {
dryRunStrategy := cmdutil.DryRunNone
if dryRun {
+ // irrespective of the dry run mode set in the sync context, always run
+ // in client dry run mode as the goal is to validate only the
+ // yaml correctness of the rendered manifests.
dryRunStrategy = cmdutil.DryRunClient
}
var err error
var message string
shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace)
- serverSideApply := sc.serverSideApply || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply)
+ // if it is a dry run, disable server side apply, as the goal is to validate only the
+ // yaml correctness of the rendered manifests.
+ serverSideApply := !dryRun && (sc.serverSideApply || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply))
if shouldReplace {
if t.liveObj != nil {
// Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
From 209eee8427ee9fce1ad30117642c563fe5e6e5f0 Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Fri, 19 Jan 2024 22:41:11 +0530
Subject: [PATCH 4/7] Added more values in the log message for better debugging
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/utils/kube/resource_ops.go | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/pkg/utils/kube/resource_ops.go b/pkg/utils/kube/resource_ops.go
index 7faa037b2..5fd23930f 100644
--- a/pkg/utils/kube/resource_ops.go
+++ b/pkg/utils/kube/resource_ops.go
@@ -229,7 +229,12 @@ func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unst
span.SetBaggageItem("kind", obj.GetKind())
span.SetBaggageItem("name", obj.GetName())
defer span.Finish()
- k.log.Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace()))
+ k.log.WithValues(
+ "dry-run", dryRunStrategy,
+ "manager", manager,
+ "serverSideApply", serverSideApply,
+ "serverSideDiff", serverSideDiff
+ ).Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace()))
return k.runResourceCommand(ctx, obj, dryRunStrategy, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
cleanup, err := k.processKubectlRun("apply")
if err != nil {
From fa36dc3d4d960dae5218a3935af6afd6c1a101f6 Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Fri, 19 Jan 2024 22:50:02 +0530
Subject: [PATCH 5/7] Fixed compilation error
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/utils/kube/resource_ops.go | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/pkg/utils/kube/resource_ops.go b/pkg/utils/kube/resource_ops.go
index 5fd23930f..df028f5dd 100644
--- a/pkg/utils/kube/resource_ops.go
+++ b/pkg/utils/kube/resource_ops.go
@@ -233,8 +233,7 @@ func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unst
"dry-run", dryRunStrategy,
"manager", manager,
"serverSideApply", serverSideApply,
- "serverSideDiff", serverSideDiff
- ).Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace()))
+ "serverSideDiff", serverSideDiff).Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace()))
return k.runResourceCommand(ctx, obj, dryRunStrategy, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
cleanup, err := k.processKubectlRun("apply")
if err != nil {
From 2a503268f83c55fa2ab8568c9dfda8f94bb6b2be Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Sat, 20 Jan 2024 00:00:51 +0530
Subject: [PATCH 6/7] Written an inline fn to get string value of dry-run
strategy
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/utils/kube/resource_ops.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkg/utils/kube/resource_ops.go b/pkg/utils/kube/resource_ops.go
index df028f5dd..6867ae876 100644
--- a/pkg/utils/kube/resource_ops.go
+++ b/pkg/utils/kube/resource_ops.go
@@ -230,7 +230,7 @@ func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unst
span.SetBaggageItem("name", obj.GetName())
defer span.Finish()
k.log.WithValues(
- "dry-run", dryRunStrategy,
+ "dry-run", [...]string{"none", "client", "server"}[dryRunStrategy],
"manager", manager,
"serverSideApply", serverSideApply,
"serverSideDiff", serverSideDiff).Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace()))
From b77faaaa04a3da4567974c37358b3bdb8e42369d Mon Sep 17 00:00:00 2001
From: Anand Francis Joseph <anjoseph@redhat.com>
Date: Sat, 20 Jan 2024 00:20:53 +0530
Subject: [PATCH 7/7] Added comment as requested with reference to the issue
number
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
---
pkg/sync/sync_context.go | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go
index 26d09efcb..c11ff7e0c 100644
--- a/pkg/sync/sync_context.go
+++ b/pkg/sync/sync_context.go
@@ -909,6 +909,8 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, force, validate bool) (c
// irrespective of the dry run mode set in the sync context, always run
// in client dry run mode as the goal is to validate only the
// yaml correctness of the rendered manifests.
+ // running dry-run in server mode breaks the auto create namespace feature
+ // https://github.com/argoproj/argo-cd/issues/13874
dryRunStrategy = cmdutil.DryRunClient
}
@@ -917,6 +919,8 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, force, validate bool) (c
shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace)
// if it is a dry run, disable server side apply, as the goal is to validate only the
// yaml correctness of the rendered manifests.
+ // running dry-run in server mode breaks the auto create namespace feature
+ // https://github.com/argoproj/argo-cd/issues/13874
serverSideApply := !dryRun && (sc.serverSideApply || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply))
if shouldReplace {
if t.liveObj != nil {