fix: address kubectl auth reconcile during server-side diff (#562)

* fix: address kubectl auth reconcile during server-side diff Signed-off-by: Leonardo Luz Almeida <[email protected]> * server-side diff force conflict Signed-off-by: Leonardo Luz Almeida <[email protected]> * do not ssa when ssd rbac Signed-off-by: Leonardo Luz Almeida <[email protected]> * debug Signed-off-by: Leonardo Luz Almeida <[email protected]> * better logs Signed-off-by: Leonardo Luz Almeida <[email protected]> * remove debug Signed-off-by: Leonardo Luz Almeida <[email protected]> * add comments Signed-off-by: Leonardo Luz Almeida <[email protected]> * better comments Signed-off-by: Leonardo Luz Almeida <[email protected]> * refactoring on rbacReconcile Signed-off-by: Leonardo Luz Almeida <[email protected]> --------- Signed-off-by: Leonardo Luz Almeida <[email protected]>
argoproj · Jan 22, 2024 · c1e2359 · c1e2359
1 parent aba3819
commit c1e2359
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 26 deletions.
diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go
@@ -165,17 +165,17 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D
 	}
 	predictedLiveStr, err := o.serverSideDryRunner.Run(context.Background(), config, o.manager)
 	if err != nil {
-		return nil, fmt.Errorf("error running server side apply in dryrun mode: %w", err)
+		return nil, fmt.Errorf("error running server side apply in dryrun mode for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 	}
 	predictedLive, err := jsonStrToUnstructured(predictedLiveStr)
 	if err != nil {
-		return nil, fmt.Errorf("error converting json string to unstructured: %w", err)
+		return nil, fmt.Errorf("error converting json string to unstructured for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 	}
 
 	if o.ignoreMutationWebhook {
 		predictedLive, err = removeWebhookMutation(predictedLive, live, o.gvkParser, o.manager)
 		if err != nil {
-			return nil, fmt.Errorf("error removing non config mutations: %w", err)
+			return nil, fmt.Errorf("error removing non config mutations for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 		}
 	}
 
@@ -184,13 +184,13 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D
 
 	predictedLiveBytes, err := json.Marshal(predictedLive)
 	if err != nil {
-		return nil, fmt.Errorf("error marshaling predicted live resource: %w", err)
+		return nil, fmt.Errorf("error marshaling predicted live for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 	}
 
 	unstructured.RemoveNestedField(live.Object, "metadata", "managedFields")
 	liveBytes, err := json.Marshal(live)
 	if err != nil {
-		return nil, fmt.Errorf("error marshaling live resource: %w", err)
+		return nil, fmt.Errorf("error marshaling live resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 	}
 	return buildDiffResult(predictedLiveBytes, liveBytes), nil
 }

diff --git a/pkg/utils/kube/resource_ops.go b/pkg/utils/kube/resource_ops.go
@@ -55,7 +55,7 @@ type kubectlResourceOperations struct {
 
 type commandExecutor func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error
 
-func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, executor commandExecutor) (string, error) {
+func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, serverSideDiff bool, executor commandExecutor) (string, error) {
 	manifestBytes, err := json.Marshal(obj)
 	if err != nil {
 		return "", err
@@ -91,21 +91,14 @@ func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj
 	}
 
 	var out []string
-	if obj.GetAPIVersion() == "rbac.authorization.k8s.io/v1" {
-		// If it is an RBAC resource, run `kubectl auth reconcile`. This is preferred over
-		// `kubectl apply`, which cannot tolerate changes in roleRef, which is an immutable field.
-		// See: https://github.com/kubernetes/kubernetes/issues/66353
-		// `auth reconcile` will delete and recreate the resource if necessary
-		outReconcile, err := func() (string, error) {
-			cleanup, err := k.processKubectlRun("auth")
-			if err != nil {
-				return "", err
-			}
-			defer cleanup()
-			return k.authReconcile(ctx, obj, manifestFile.Name(), dryRunStrategy)
-		}()
+	// rbac resouces are first applied with auth reconcile kubectl feature.
+	// serverSideDiff should avoid this step as the resources are not being actually
+	// applied but just running in dryrun mode. Also, kubectl auth reconcile doesn't
+	// currently support running dryrun in server mode.
+	if obj.GetAPIVersion() == "rbac.authorization.k8s.io/v1" && !serverSideDiff {
+		outReconcile, err := k.rbacReconcile(ctx, obj, manifestFile.Name(), dryRunStrategy)
 		if err != nil {
-			return "", err
+			return "", fmt.Errorf("error running rbacReconcile: %s", err)
 		}
 		out = append(out, outReconcile)
 		// We still want to fallthrough and run `kubectl apply` in order set the
@@ -131,6 +124,28 @@ func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj
 	return strings.Join(out, ". "), nil
 }
 
+// rbacReconcile will perform reconciliation for RBAC resources. It will run
+// the following command:
+//
+//	kubectl auth reconcile
+//
+// This is preferred over `kubectl apply`, which cannot tolerate changes in
+// roleRef, which is an immutable field.
+// See: https://github.com/kubernetes/kubernetes/issues/66353
+// `auth reconcile` will delete and recreate the resource if necessary
+func (k *kubectlResourceOperations) rbacReconcile(ctx context.Context, obj *unstructured.Unstructured, fileName string, dryRunStrategy cmdutil.DryRunStrategy) (string, error) {
+	cleanup, err := k.processKubectlRun("auth")
+	if err != nil {
+		return "", fmt.Errorf("error processing kubectl run auth: %w", err)
+	}
+	defer cleanup()
+	outReconcile, err := k.authReconcile(ctx, obj, fileName, dryRunStrategy)
+	if err != nil {
+		return "", fmt.Errorf("error running kubectl auth reconcile: %w", err)
+	}
+	return outReconcile, nil
+}
+
 func kubeCmdFactory(kubeconfig, ns string, config *rest.Config) cmdutil.Factory {
 	kubeConfigFlags := genericclioptions.NewConfigFlags(true)
 	if ns != "" {
@@ -149,7 +164,7 @@ func (k *kubectlResourceOperations) ReplaceResource(ctx context.Context, obj *un
 	span.SetBaggageItem("name", obj.GetName())
 	defer span.Finish()
 	k.log.Info(fmt.Sprintf("Replacing 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 {
+	return k.runResourceCommand(ctx, obj, dryRunStrategy, false, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
 		cleanup, err := k.processKubectlRun("replace")
 		if err != nil {
 			return err
@@ -170,7 +185,7 @@ func (k *kubectlResourceOperations) CreateResource(ctx context.Context, obj *uns
 	span.SetBaggageItem("kind", gvk.Kind)
 	span.SetBaggageItem("name", obj.GetName())
 	defer span.Finish()
-	return k.runResourceCommand(ctx, obj, dryRunStrategy, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
+	return k.runResourceCommand(ctx, obj, dryRunStrategy, false, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
 		cleanup, err := k.processKubectlRun("create")
 		if err != nil {
 			return err
@@ -230,7 +245,7 @@ func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unst
 	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()))
-	return k.runResourceCommand(ctx, obj, dryRunStrategy, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
+	return k.runResourceCommand(ctx, obj, dryRunStrategy, serverSideDiff, func(f cmdutil.Factory, ioStreams genericclioptions.IOStreams, fileName string) error {
 		cleanup, err := k.processKubectlRun("apply")
 		if err != nil {
 			return err
@@ -317,7 +332,7 @@ func (k *kubectlResourceOperations) newApplyOptions(ioStreams genericclioptions.
 	if manager != "" {
 		o.FieldManager = manager
 	}
-	if serverSideApply {
+	if serverSideApply || serverSideDiff {
 		o.ForceConflicts = true
 	}
 	return o, nil
@@ -469,9 +484,8 @@ func (k *kubectlResourceOperations) authReconcile(ctx context.Context, obj *unst
 	}
 	reconcileOpts, err := newReconcileOptions(k.fact, kubeClient, manifestFile, ioStreams, obj.GetNamespace(), dryRunStrategy)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error calling newReconcileOptions: %w", err)
 	}
-
 	err = reconcileOpts.Validate()
 	if err != nil {
 		return "", errors.New(cleanKubectlOutput(err.Error()))