diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 6da6e35a62c5d..9c8042e276317 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -221,7 +221,6 @@ func newFakeControllerWithResync(ctx context.Context, data *fakeData, appResyncP defer cancelApp() clusterCacheMock := &mocks.ClusterCache{} clusterCacheMock.EXPECT().IsNamespaced(mock.Anything).Return(true, nil) - clusterCacheMock.EXPECT().GetOpenAPISchema().Return(nil) clusterCacheMock.EXPECT().GetGVKParser().Return(nil) mockStateCache := &mockstatecache.LiveStateCache{} diff --git a/controller/sync.go b/controller/sync.go index ea17622502fec..fe4ebda52a4e5 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -25,7 +25,6 @@ import ( "k8s.io/apimachinery/pkg/util/managedfields" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" - "k8s.io/kubectl/pkg/util/openapi" "github.com/argoproj/argo-cd/v3/controller/metrics" "github.com/argoproj/argo-cd/v3/controller/syncid" @@ -49,14 +48,6 @@ const ( serviceAccountDisallowedCharSet = "!*[]{}\\/" ) -func (m *appStateManager) getOpenAPISchema(server *v1alpha1.Cluster) (openapi.Resources, error) { - cluster, err := m.liveStateCache.GetClusterCache(server) - if err != nil { - return nil, err - } - return cluster.GetOpenAPISchema(), nil -} - func (m *appStateManager) getGVKParser(server *v1alpha1.Cluster) (*managedfields.GvkParser, error) { cluster, err := m.liveStateCache.GetClusterCache(server) if err != nil { @@ -70,16 +61,11 @@ func (m *appStateManager) getGVKParser(server *v1alpha1.Cluster) (*managedfields // cleanup function that must be called to remove the generated kube config for this // server. func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Cluster) (gitopsDiff.KubeApplier, func(), error) { - clusterCache, err := m.liveStateCache.GetClusterCache(cluster) - if err != nil { - return nil, nil, fmt.Errorf("error getting cluster cache: %w", err) - } - rawConfig, err := cluster.RawRestConfig() if err != nil { return nil, nil, fmt.Errorf("error getting cluster REST config: %w", err) } - ops, cleanup, err := kubeutil.ManageServerSideDiffDryRuns(rawConfig, clusterCache.GetOpenAPISchema(), m.onKubectlRun) + ops, cleanup, err := kubeutil.ManageServerSideDiffDryRuns(rawConfig, m.onKubectlRun) if err != nil { return nil, nil, fmt.Errorf("error creating kubectl ResourceOperations: %w", err) } @@ -249,13 +235,6 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp clientSideApplyManager = managerValue } - openAPISchema, err := m.getOpenAPISchema(destCluster) - if err != nil { - state.Phase = common.OperationError - state.Message = fmt.Sprintf("failed to load openAPISchema: %v", err) - return - } - reconciliationResult := compareResult.reconciliationResult // if RespectIgnoreDifferences is enabled, it should normalize the target @@ -349,7 +328,6 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp rawConfig, m.kubectl, app.Spec.Destination.Namespace, - openAPISchema, opts..., ) if err != nil { diff --git a/gitops-engine/.mockery.yaml b/gitops-engine/.mockery.yaml new file mode 100644 index 0000000000000..b7d34a618afb2 --- /dev/null +++ b/gitops-engine/.mockery.yaml @@ -0,0 +1,11 @@ +dir: '{{.InterfaceDir}}/mocks' +filename: '{{.InterfaceName}}.go' +include-auto-generated: true # Needed since mockery 3.6.1 +pkgname: mocks +structname: '{{.InterfaceName}}' +template-data: + unroll-variadic: true +packages: + github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube: + interfaces: + KubectlOptionsRunner: {} diff --git a/gitops-engine/pkg/engine/engine.go b/gitops-engine/pkg/engine/engine.go index 4196876315b53..1b263a59c434b 100644 --- a/gitops-engine/pkg/engine/engine.go +++ b/gitops-engine/pkg/engine/engine.go @@ -84,7 +84,7 @@ func (e *gitOpsEngine) Sync(ctx context.Context, return nil, fmt.Errorf("failed to diff objects: %w", err) } opts = append(opts, sync.WithSkipHooks(!diffRes.Modified)) - syncCtx, cleanup, err := sync.NewSyncContext(revision, result, e.config, e.config, e.kubectl, namespace, e.cache.GetOpenAPISchema(), opts...) + syncCtx, cleanup, err := sync.NewSyncContext(revision, result, e.config, e.config, e.kubectl, namespace, opts...) if err != nil { return nil, fmt.Errorf("failed to create sync context: %w", err) } diff --git a/gitops-engine/pkg/sync/sync_context.go b/gitops-engine/pkg/sync/sync_context.go index 1a2d1b37dde44..d121a84a651b3 100644 --- a/gitops-engine/pkg/sync/sync_context.go +++ b/gitops-engine/pkg/sync/sync_context.go @@ -27,7 +27,6 @@ import ( "k8s.io/client-go/util/retry" "k8s.io/klog/v2/textlogger" cmdutil "k8s.io/kubectl/pkg/cmd/util" - "k8s.io/kubectl/pkg/util/openapi" "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" "github.com/argoproj/argo-cd/gitops-engine/pkg/health" @@ -236,7 +235,6 @@ func NewSyncContext( rawConfig *rest.Config, kubectl kubeutil.Kubectl, namespace string, - openAPISchema openapi.Resources, opts ...SyncOpt, ) (SyncContext, func(), error) { dynamicIf, err := dynamic.NewForConfig(restConfig) @@ -251,7 +249,7 @@ func NewSyncContext( if err != nil { return nil, nil, fmt.Errorf("failed to create extensions client: %w", err) } - resourceOps, cleanup, err := kubectl.ManageResources(rawConfig, openAPISchema) + resourceOps, cleanup, err := kubectl.ManageResources(rawConfig) if err != nil { return nil, nil, fmt.Errorf("failed to manage resources: %w", err) } diff --git a/gitops-engine/pkg/utils/kube/ctl.go b/gitops-engine/pkg/utils/kube/ctl.go index a8f3e1d5beddf..06c5efbcf2cc7 100644 --- a/gitops-engine/pkg/utils/kube/ctl.go +++ b/gitops-engine/pkg/utils/kube/ctl.go @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/util/version" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/kube-openapi/pkg/util/proto" "k8s.io/kubectl/pkg/util/openapi" @@ -30,8 +31,8 @@ type CleanupFunc func() type OnKubectlRunFunc func(command string) (CleanupFunc, error) type Kubectl interface { - ManageResources(config *rest.Config, openAPISchema openapi.Resources) (ResourceOperations, func(), error) - ManageServerSideDiffDryRuns(config *rest.Config, openAPISchema openapi.Resources) (diff.KubeApplier, func(), error) + ManageResources(config *rest.Config) (ResourceOperations, func(), error) + ManageServerSideDiffDryRuns(config *rest.Config) (diff.KubeApplier, func(), error) LoadOpenAPISchema(config *rest.Config) (openapi.Resources, *managedfields.GvkParser, error) ConvertToVersion(obj *unstructured.Unstructured, group, version string) (*unstructured.Unstructured, error) DeleteResource(ctx context.Context, config *rest.Config, gvk schema.GroupVersionKind, name string, namespace string, deleteOptions metav1.DeleteOptions) error @@ -277,7 +278,7 @@ func (k *KubectlCmd) DeleteResource(ctx context.Context, config *rest.Config, gv return resourceIf.Delete(ctx, name, deleteOptions) } -func (k *KubectlCmd) ManageResources(config *rest.Config, openAPISchema openapi.Resources) (ResourceOperations, func(), error) { +func (k *KubectlCmd) ManageResources(config *rest.Config) (ResourceOperations, func(), error) { f, err := os.CreateTemp(utils.TempDir, "") if err != nil { return nil, nil, fmt.Errorf("failed to generate temp file for kubeconfig: %w", err) @@ -293,17 +294,22 @@ func (k *KubectlCmd) ManageResources(config *rest.Config, openAPISchema openapi. utils.DeleteFile(f.Name()) } return &kubectlResourceOperations{ - config: config, - fact: fact, - openAPISchema: openAPISchema, - tracer: k.Tracer, - log: k.Log, - onKubectlRun: k.OnKubectlRun, + config: config, + getClientFunc: func() (kubernetes.Interface, error) { + return kubernetes.NewForConfig(config) + }, + fact: fact, + tracer: k.Tracer, + log: k.Log, + optionsRunner: &realKubectlOptionsRunner{ + onKubectlRun: k.OnKubectlRun, + }, + outputMode: outputModeLog, }, cleanup, nil } // ManageServerSideDiffDryRuns creates a KubeApplier for server-side diff dry runs -func (k *KubectlCmd) ManageServerSideDiffDryRuns(config *rest.Config, openAPISchema openapi.Resources) (diff.KubeApplier, func(), error) { +func (k *KubectlCmd) ManageServerSideDiffDryRuns(config *rest.Config) (diff.KubeApplier, func(), error) { f, err := os.CreateTemp(utils.TempDir, "") if err != nil { return nil, nil, fmt.Errorf("failed to generate temp file for kubeconfig: %w", err) @@ -318,13 +324,18 @@ func (k *KubectlCmd) ManageServerSideDiffDryRuns(config *rest.Config, openAPISch cleanup := func() { utils.DeleteFile(f.Name()) } - return &kubectlServerSideDiffDryRunApplier{ - config: config, - fact: fact, - openAPISchema: openAPISchema, - tracer: k.Tracer, - log: k.Log, - onKubectlRun: k.OnKubectlRun, + return &kubectlResourceOperations{ + config: config, + getClientFunc: func() (kubernetes.Interface, error) { + return kubernetes.NewForConfig(config) + }, + fact: fact, + tracer: k.Tracer, + log: k.Log, + optionsRunner: &realKubectlOptionsRunner{ + onKubectlRun: k.OnKubectlRun, + }, + outputMode: outputModeJSON, }, cleanup, nil } diff --git a/gitops-engine/pkg/utils/kube/kubetest/mock.go b/gitops-engine/pkg/utils/kube/kubetest/mock.go index 80825202dd2c8..dd3a907458a47 100644 --- a/gitops-engine/pkg/utils/kube/kubetest/mock.go +++ b/gitops-engine/pkg/utils/kube/kubetest/mock.go @@ -35,7 +35,7 @@ type MockKubectlCmd struct { convertToVersionFunc *func(obj *unstructured.Unstructured, group, version string) (*unstructured.Unstructured, error) getResourceFunc *func(ctx context.Context, config *rest.Config, gvk schema.GroupVersionKind, name string, namespace string) (*unstructured.Unstructured, error) loadOpenAPISchemaFunc *func(config *rest.Config) (openapi.Resources, *managedfields.GvkParser, error) - manageServerSideDiffDryRunFunc *func(config *rest.Config, openAPISchema openapi.Resources) (diff.KubeApplier, func(), error) + manageServerSideDiffDryRunFunc *func(config *rest.Config) (diff.KubeApplier, func(), error) } // WithConvertToVersionFunc overrides the default ConvertToVersion behavior. @@ -57,7 +57,7 @@ func (k *MockKubectlCmd) WithLoadOpenAPISchemaFunc(loadOpenAPISchemaFunc func(*r } // WithManageServerSideDiffDryRunFunc overrides the default ManageServerSideDiffDryRuns behavior. -func (k *MockKubectlCmd) WithManageServerSideDiffDryRunFunc(manageServerSideDiffDryRunFunc func(*rest.Config, openapi.Resources) (diff.KubeApplier, func(), error)) *MockKubectlCmd { +func (k *MockKubectlCmd) WithManageServerSideDiffDryRunFunc(manageServerSideDiffDryRunFunc func(*rest.Config) (diff.KubeApplier, func(), error)) *MockKubectlCmd { k.manageServerSideDiffDryRunFunc = &manageServerSideDiffDryRunFunc return k } @@ -117,14 +117,14 @@ func (k *MockKubectlCmd) LoadOpenAPISchema(config *rest.Config) (openapi.Resourc func (k *MockKubectlCmd) SetOnKubectlRun(_ kube.OnKubectlRunFunc) { } -func (k *MockKubectlCmd) ManageResources(_ *rest.Config, _ openapi.Resources) (kube.ResourceOperations, func(), error) { +func (k *MockKubectlCmd) ManageResources(_ *rest.Config) (kube.ResourceOperations, func(), error) { return &MockResourceOps{}, func() { }, nil } -func (k *MockKubectlCmd) ManageServerSideDiffDryRuns(config *rest.Config, openAPISchema openapi.Resources) (diff.KubeApplier, func(), error) { +func (k *MockKubectlCmd) ManageServerSideDiffDryRuns(config *rest.Config) (diff.KubeApplier, func(), error) { if k.manageServerSideDiffDryRunFunc != nil { - return (*k.manageServerSideDiffDryRunFunc)(config, openAPISchema) + return (*k.manageServerSideDiffDryRunFunc)(config) } return &MockKubeApplier{}, func() {}, nil } diff --git a/gitops-engine/pkg/utils/kube/mocks/KubectlOptionsRunner.go b/gitops-engine/pkg/utils/kube/mocks/KubectlOptionsRunner.go new file mode 100644 index 0000000000000..441caa295c59e --- /dev/null +++ b/gitops-engine/pkg/utils/kube/mocks/KubectlOptionsRunner.go @@ -0,0 +1,264 @@ +// Code generated by mockery; DO NOT EDIT. +// github.com/vektra/mockery +// template: testify + +package mocks + +import ( + "github.com/spf13/cobra" + mock "github.com/stretchr/testify/mock" + "k8s.io/kubectl/pkg/cmd/apply" + "k8s.io/kubectl/pkg/cmd/auth" + "k8s.io/kubectl/pkg/cmd/create" + "k8s.io/kubectl/pkg/cmd/replace" + "k8s.io/kubectl/pkg/cmd/util" +) + +// NewKubectlOptionsRunner creates a new instance of KubectlOptionsRunner. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewKubectlOptionsRunner(t interface { + mock.TestingT + Cleanup(func()) +}) *KubectlOptionsRunner { + mock := &KubectlOptionsRunner{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} + +// KubectlOptionsRunner is an autogenerated mock type for the KubectlOptionsRunner type +type KubectlOptionsRunner struct { + mock.Mock +} + +type KubectlOptionsRunner_Expecter struct { + mock *mock.Mock +} + +func (_m *KubectlOptionsRunner) EXPECT() *KubectlOptionsRunner_Expecter { + return &KubectlOptionsRunner_Expecter{mock: &_m.Mock} +} + +// Apply provides a mock function for the type KubectlOptionsRunner +func (_mock *KubectlOptionsRunner) Apply(opts *apply.ApplyOptions) error { + ret := _mock.Called(opts) + + if len(ret) == 0 { + panic("no return value specified for Apply") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(*apply.ApplyOptions) error); ok { + r0 = returnFunc(opts) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// KubectlOptionsRunner_Apply_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Apply' +type KubectlOptionsRunner_Apply_Call struct { + *mock.Call +} + +// Apply is a helper method to define mock.On call +// - opts *apply.ApplyOptions +func (_e *KubectlOptionsRunner_Expecter) Apply(opts interface{}) *KubectlOptionsRunner_Apply_Call { + return &KubectlOptionsRunner_Apply_Call{Call: _e.mock.On("Apply", opts)} +} + +func (_c *KubectlOptionsRunner_Apply_Call) Run(run func(opts *apply.ApplyOptions)) *KubectlOptionsRunner_Apply_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 *apply.ApplyOptions + if args[0] != nil { + arg0 = args[0].(*apply.ApplyOptions) + } + run( + arg0, + ) + }) + return _c +} + +func (_c *KubectlOptionsRunner_Apply_Call) Return(err error) *KubectlOptionsRunner_Apply_Call { + _c.Call.Return(err) + return _c +} + +func (_c *KubectlOptionsRunner_Apply_Call) RunAndReturn(run func(opts *apply.ApplyOptions) error) *KubectlOptionsRunner_Apply_Call { + _c.Call.Return(run) + return _c +} + +// AuthReconcile provides a mock function for the type KubectlOptionsRunner +func (_mock *KubectlOptionsRunner) AuthReconcile(opts *auth.ReconcileOptions) error { + ret := _mock.Called(opts) + + if len(ret) == 0 { + panic("no return value specified for AuthReconcile") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(*auth.ReconcileOptions) error); ok { + r0 = returnFunc(opts) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// KubectlOptionsRunner_AuthReconcile_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AuthReconcile' +type KubectlOptionsRunner_AuthReconcile_Call struct { + *mock.Call +} + +// AuthReconcile is a helper method to define mock.On call +// - opts *auth.ReconcileOptions +func (_e *KubectlOptionsRunner_Expecter) AuthReconcile(opts interface{}) *KubectlOptionsRunner_AuthReconcile_Call { + return &KubectlOptionsRunner_AuthReconcile_Call{Call: _e.mock.On("AuthReconcile", opts)} +} + +func (_c *KubectlOptionsRunner_AuthReconcile_Call) Run(run func(opts *auth.ReconcileOptions)) *KubectlOptionsRunner_AuthReconcile_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 *auth.ReconcileOptions + if args[0] != nil { + arg0 = args[0].(*auth.ReconcileOptions) + } + run( + arg0, + ) + }) + return _c +} + +func (_c *KubectlOptionsRunner_AuthReconcile_Call) Return(err error) *KubectlOptionsRunner_AuthReconcile_Call { + _c.Call.Return(err) + return _c +} + +func (_c *KubectlOptionsRunner_AuthReconcile_Call) RunAndReturn(run func(opts *auth.ReconcileOptions) error) *KubectlOptionsRunner_AuthReconcile_Call { + _c.Call.Return(run) + return _c +} + +// Create provides a mock function for the type KubectlOptionsRunner +func (_mock *KubectlOptionsRunner) Create(opts *create.CreateOptions, fact util.Factory, cmd *cobra.Command) error { + ret := _mock.Called(opts, fact, cmd) + + if len(ret) == 0 { + panic("no return value specified for Create") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(*create.CreateOptions, util.Factory, *cobra.Command) error); ok { + r0 = returnFunc(opts, fact, cmd) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// KubectlOptionsRunner_Create_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Create' +type KubectlOptionsRunner_Create_Call struct { + *mock.Call +} + +// Create is a helper method to define mock.On call +// - opts *create.CreateOptions +// - fact util.Factory +// - cmd *cobra.Command +func (_e *KubectlOptionsRunner_Expecter) Create(opts interface{}, fact interface{}, cmd interface{}) *KubectlOptionsRunner_Create_Call { + return &KubectlOptionsRunner_Create_Call{Call: _e.mock.On("Create", opts, fact, cmd)} +} + +func (_c *KubectlOptionsRunner_Create_Call) Run(run func(opts *create.CreateOptions, fact util.Factory, cmd *cobra.Command)) *KubectlOptionsRunner_Create_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 *create.CreateOptions + if args[0] != nil { + arg0 = args[0].(*create.CreateOptions) + } + var arg1 util.Factory + if args[1] != nil { + arg1 = args[1].(util.Factory) + } + var arg2 *cobra.Command + if args[2] != nil { + arg2 = args[2].(*cobra.Command) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *KubectlOptionsRunner_Create_Call) Return(err error) *KubectlOptionsRunner_Create_Call { + _c.Call.Return(err) + return _c +} + +func (_c *KubectlOptionsRunner_Create_Call) RunAndReturn(run func(opts *create.CreateOptions, fact util.Factory, cmd *cobra.Command) error) *KubectlOptionsRunner_Create_Call { + _c.Call.Return(run) + return _c +} + +// Replace provides a mock function for the type KubectlOptionsRunner +func (_mock *KubectlOptionsRunner) Replace(opts *replace.ReplaceOptions, fact util.Factory) error { + ret := _mock.Called(opts, fact) + + if len(ret) == 0 { + panic("no return value specified for Replace") + } + + var r0 error + if returnFunc, ok := ret.Get(0).(func(*replace.ReplaceOptions, util.Factory) error); ok { + r0 = returnFunc(opts, fact) + } else { + r0 = ret.Error(0) + } + return r0 +} + +// KubectlOptionsRunner_Replace_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Replace' +type KubectlOptionsRunner_Replace_Call struct { + *mock.Call +} + +// Replace is a helper method to define mock.On call +// - opts *replace.ReplaceOptions +// - fact util.Factory +func (_e *KubectlOptionsRunner_Expecter) Replace(opts interface{}, fact interface{}) *KubectlOptionsRunner_Replace_Call { + return &KubectlOptionsRunner_Replace_Call{Call: _e.mock.On("Replace", opts, fact)} +} + +func (_c *KubectlOptionsRunner_Replace_Call) Run(run func(opts *replace.ReplaceOptions, fact util.Factory)) *KubectlOptionsRunner_Replace_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 *replace.ReplaceOptions + if args[0] != nil { + arg0 = args[0].(*replace.ReplaceOptions) + } + var arg1 util.Factory + if args[1] != nil { + arg1 = args[1].(util.Factory) + } + run( + arg0, + arg1, + ) + }) + return _c +} + +func (_c *KubectlOptionsRunner_Replace_Call) Return(err error) *KubectlOptionsRunner_Replace_Call { + _c.Call.Return(err) + return _c +} + +func (_c *KubectlOptionsRunner_Replace_Call) RunAndReturn(run func(opts *replace.ReplaceOptions, fact util.Factory) error) *KubectlOptionsRunner_Replace_Call { + _c.Call.Return(run) + return _c +} diff --git a/gitops-engine/pkg/utils/kube/resource_ops.go b/gitops-engine/pkg/utils/kube/resource_ops.go index a96dd8fa07f85..d9f25bde98e8a 100644 --- a/gitops-engine/pkg/utils/kube/resource_ops.go +++ b/gitops-engine/pkg/utils/kube/resource_ops.go @@ -31,13 +31,19 @@ import ( "k8s.io/kubectl/pkg/cmd/replace" cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/scheme" - "k8s.io/kubectl/pkg/util/openapi" "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/io" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/tracing" ) +type outputMode int + +const ( + outputModeLog outputMode = iota // Return log messages (normal apply) + outputModeJSON // Return JSON object (server-side diff) +) + // ResourceOperations provides methods to manage k8s resources type ResourceOperations interface { ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) @@ -46,24 +52,75 @@ type ResourceOperations interface { UpdateResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy) (*unstructured.Unstructured, error) } -// This is a generic implementation for doing most kubectl operations. Implements the ResourceOperations interface. -type kubectlResourceOperations struct { - config *rest.Config - log logr.Logger - tracer tracing.Tracer - onKubectlRun OnKubectlRunFunc - fact cmdutil.Factory - openAPISchema openapi.Resources +// KubectlOptionsRunner defines the operations to run kubectl commands based on provided options +type KubectlOptionsRunner interface { + Apply(opts *apply.ApplyOptions) error + Create(opts *create.CreateOptions, fact cmdutil.Factory, cmd *cobra.Command) error + Replace(opts *replace.ReplaceOptions, fact cmdutil.Factory) error + AuthReconcile(opts *auth.ReconcileOptions) error +} + +// realKubectlOptionsRunner is the implementation of KubectlOptionsRunner calling underlying kubectl command. +type realKubectlOptionsRunner struct { + onKubectlRun OnKubectlRunFunc +} + +// Apply will perform https://kubernetes.io/docs/reference/kubectl/generated/kubectl_apply/ +func (f *realKubectlOptionsRunner) Apply(opts *apply.ApplyOptions) error { + cleanup, err := f.processKubectlRun("apply") + if err != nil { + return err + } + defer cleanup() + return opts.Run() +} + +// Create will perform https://kubernetes.io/docs/reference/kubectl/generated/kubectl_create/ +func (f *realKubectlOptionsRunner) Create(opts *create.CreateOptions, fact cmdutil.Factory, cmd *cobra.Command) error { + cleanup, err := f.processKubectlRun("create") + if err != nil { + return err + } + defer cleanup() + return opts.RunCreate(fact, cmd) } -// This is an implementation specific for doing server-side diff dry runs. Implements the KubeApplier interface. -type kubectlServerSideDiffDryRunApplier struct { +// Replace will perform https://kubernetes.io/docs/reference/kubectl/generated/kubectl_replace/ +func (f *realKubectlOptionsRunner) Replace(opts *replace.ReplaceOptions, fact cmdutil.Factory) error { + cleanup, err := f.processKubectlRun("replace") + if err != nil { + return err + } + defer cleanup() + return opts.Run(fact) +} + +// AuthReconcile will perform https://kubernetes.io/docs/reference/kubectl/generated/kubectl_auth/kubectl_auth_reconcile/ +func (f *realKubectlOptionsRunner) AuthReconcile(opts *auth.ReconcileOptions) error { + cleanup, err := f.processKubectlRun("auth") + if err != nil { + return err + } + defer cleanup() + return opts.RunReconcile() +} + +func (f *realKubectlOptionsRunner) processKubectlRun(cmd string) (CleanupFunc, error) { + if f.onKubectlRun != nil { + return f.onKubectlRun(cmd) + } + return func() {}, nil +} + +// This is a generic implementation for doing most kubectl operations. Implements the ResourceOperations interface. +type kubectlResourceOperations struct { config *rest.Config + getClientFunc func() (kubernetes.Interface, error) log logr.Logger tracer tracing.Tracer - onKubectlRun OnKubectlRunFunc fact cmdutil.Factory - openAPISchema openapi.Resources + optionsRunner KubectlOptionsRunner + outputMode outputMode } type commandExecutor func(ioStreams genericiooptions.IOStreams, fileName string) error @@ -112,45 +169,30 @@ func createManifestFile(obj *unstructured.Unstructured, log logr.Logger) (*os.Fi return manifestFile, nil } -func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, reconcileRBAC bool, executor commandExecutor) (string, error) { - manifestFile, err := createManifestFile(obj, k.log) - if err != nil { +func (k *kubectlResourceOperations) handleJSONOutput(stdout, stderr string) (string, error) { + if stderr != "" && stdout == "" { + err := fmt.Errorf("command output had non-empty stderr: %s", stderr) + k.log.Error(err, "error running command") return "", err } - defer io.DeleteFile(manifestFile.Name()) - - var out []string - // rbac resources are first applied with auth reconcile kubectl feature. - if reconcileRBAC && obj.GetAPIVersion() == "rbac.authorization.k8s.io/v1" { - outReconcile, err := k.rbacReconcile(ctx, obj, manifestFile.Name(), dryRunStrategy) - if err != nil { - return "", fmt.Errorf("error running rbacReconcile: %w", err) - } - out = append(out, outReconcile) - // We still want to fallthrough and run `kubectl apply` in order set the - // last-applied-configuration annotation in the object. + if stderr != "" { + k.log.Info("Warning: Command output had non-empty stderr: %s", stderr) } + return stdout, nil +} - // Run kubectl apply - ioStreams := genericiooptions.IOStreams{ - In: &bytes.Buffer{}, - Out: &bytes.Buffer{}, - ErrOut: &bytes.Buffer{}, - } - err = executor(ioStreams, manifestFile.Name()) - if err != nil { - return "", errors.New(cleanKubectlOutput(err.Error())) - } - if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); len(buf) > 0 { +func (k *kubectlResourceOperations) handleLogOutput(stdout, stderr string) (string, error) { + var out []string + if buf := strings.TrimSpace(stdout); buf != "" { out = append(out, buf) } - if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); len(buf) > 0 { + if buf := strings.TrimSpace(stderr); buf != "" { out = append(out, buf) } return strings.Join(out, ". "), nil } -func (k *kubectlServerSideDiffDryRunApplier) runResourceCommand(obj *unstructured.Unstructured, executor commandExecutor) (string, error) { +func (k *kubectlResourceOperations) runResourceCommand(_ context.Context, obj *unstructured.Unstructured, executor commandExecutor) (string, error) { manifestFile, err := createManifestFile(obj, k.log) if err != nil { return "", err @@ -160,7 +202,7 @@ func (k *kubectlServerSideDiffDryRunApplier) runResourceCommand(obj *unstructure stdoutBuf := &bytes.Buffer{} stderrBuf := &bytes.Buffer{} - // Run kubectl apply + // Run kubectl command ioStreams := genericiooptions.IOStreams{ In: &bytes.Buffer{}, Out: stdoutBuf, @@ -170,18 +212,15 @@ func (k *kubectlServerSideDiffDryRunApplier) runResourceCommand(obj *unstructure if err != nil { return "", errors.New(cleanKubectlOutput(err.Error())) } + stdout := stdoutBuf.String() stderr := stderrBuf.String() - if stderr != "" && stdout == "" { - err := fmt.Errorf("server-side dry run apply had non-empty stderr: %s", stderr) - k.log.Error(err, "server-side diff") - return "", err - } - if stderr != "" { - k.log.Info("Warning: Server-side dry run apply had non-empty stderr: %s", stderr) + // Delegate to appropriate handler based on output mode + if k.outputMode == outputModeJSON { + return k.handleJSONOutput(stdout, stderr) } - return stdout, nil + return k.handleLogOutput(stdout, stderr) } // rbacReconcile will perform reconciliation for RBAC resources. It will run @@ -193,17 +232,47 @@ func (k *kubectlServerSideDiffDryRunApplier) runResourceCommand(obj *unstructure // 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 := processKubectlRun(k.onKubectlRun, "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 (k *kubectlResourceOperations) rbacReconcile(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy) (string, error) { + gvk := obj.GroupVersionKind() + span := k.tracer.StartSpan("AuthReconcile") + span.SetBaggageItem("kind", gvk.Kind) + span.SetBaggageItem("name", obj.GetName()) + defer span.Finish() + return k.runResourceCommand(ctx, obj, func(ioStreams genericiooptions.IOStreams, fileName string) error { + kubeClient, err := k.getClientFunc() + if err != nil { + return fmt.Errorf("error creating kube client: %w", err) + } + + // `kubectl auth reconcile` has a side effect of auto-creating namespaces if it doesn't exist. + // See: https://github.com/kubernetes/kubernetes/issues/71185. This is behavior which we do + // not want. We need to check if the namespace exists, before know if it is safe to run this + // command. Skip this for dryRuns. + + // When an Argo CD Application specifies destination.namespace, that namespace + // may be propagated even for cluster-scoped resources. Passing a namespace in + // this case causes `kubectl auth reconcile` to fail with: + // "namespaces not found" + // or may trigger unintended namespace handling behavior. + // Therefore, we skip namespace existence checks for cluster-scoped RBAC + // resources and allow reconcile to run without a namespace. + // + // https://github.com/argoproj/argo-cd/issues/24833 + clusterScoped := obj.GetKind() == "ClusterRole" || obj.GetKind() == "ClusterRoleBinding" + if dryRunStrategy == cmdutil.DryRunNone && obj.GetNamespace() != "" && !clusterScoped { + _, err = kubeClient.CoreV1().Namespaces().Get(ctx, obj.GetNamespace(), metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("error getting namespace %s: %w", obj.GetNamespace(), err) + } + } + + authReconcileOptions, err := newReconcileOptions(k.fact, kubeClient, fileName, ioStreams, obj.GetNamespace(), dryRunStrategy) + if err != nil { + return err + } + + return k.optionsRunner.AuthReconcile(authReconcileOptions) + }) } func kubeCmdFactory(kubeconfig, ns string, config *rest.Config) cmdutil.Factory { @@ -227,19 +296,13 @@ 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, false, func(ioStreams genericiooptions.IOStreams, fileName string) error { - cleanup, err := processKubectlRun(k.onKubectlRun, "replace") - if err != nil { - return err - } - defer cleanup() - + return k.runResourceCommand(ctx, obj, func(ioStreams genericiooptions.IOStreams, fileName string) error { replaceOptions, err := k.newReplaceOptions(k.config, k.fact, ioStreams, fileName, obj.GetNamespace(), force, dryRunStrategy) if err != nil { return err } - return replaceOptions.Run(k.fact) + return k.optionsRunner.Replace(replaceOptions, k.fact) }) } @@ -249,13 +312,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, false, func(ioStreams genericiooptions.IOStreams, fileName string) error { - cleanup, err := processKubectlRun(k.onKubectlRun, "create") - if err != nil { - return err - } - defer cleanup() - + return k.runResourceCommand(ctx, obj, func(ioStreams genericiooptions.IOStreams, fileName string) error { createOptions, err := k.newCreateOptions(ioStreams, fileName, dryRunStrategy) if err != nil { return err @@ -269,7 +326,7 @@ func (k *kubectlResourceOperations) CreateResource(ctx context.Context, obj *uns _ = command.Flags().Set("validate", "true") } - return createOptions.RunCreate(k.fact, command) + return k.optionsRunner.Create(createOptions, k.fact, command) }) } @@ -303,38 +360,13 @@ func (k *kubectlResourceOperations) UpdateResource(ctx context.Context, obj *uns return resourceIf.Update(ctx, obj, updateOptions) } -// ApplyResource performs an apply of a unstructured resource -func (k *kubectlServerSideDiffDryRunApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { - span := k.tracer.StartSpan("ApplyResource") - span.SetBaggageItem("kind", obj.GetKind()) - span.SetBaggageItem("name", obj.GetName()) - defer span.Finish() - k.log.V(1).WithValues( - "dry-run", [...]string{"none", "client", "server"}[dryRunStrategy], - "manager", manager, - "serverSideApply", serverSideApply).Info(fmt.Sprintf("Running server-side diff. Dry run applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace())) - - return k.runResourceCommand(obj, func(ioStreams genericiooptions.IOStreams, fileName string) error { - cleanup, err := processKubectlRun(k.onKubectlRun, "apply") - if err != nil { - return err - } - defer cleanup() - - applyOpts, err := k.newApplyOptions(ioStreams, obj, fileName, validate, force, serverSideApply, dryRunStrategy, manager) - if err != nil { - return err - } - return applyOpts.Run() - }) -} - // ApplyResource performs an apply of a unstructured resource func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { span := k.tracer.StartSpan("ApplyResource") span.SetBaggageItem("kind", obj.GetKind()) span.SetBaggageItem("name", obj.GetName()) defer span.Finish() + logWithLevel := k.log.V(0) if dryRunStrategy != cmdutil.DryRunNone { logWithLevel = logWithLevel.V(1) @@ -342,25 +374,43 @@ func (k *kubectlResourceOperations) ApplyResource(ctx context.Context, obj *unst logWithLevel.WithValues( "dry-run", [...]string{"none", "client", "server"}[dryRunStrategy], "manager", manager, - "serverSideApply", serverSideApply, - "serverSideDiff", true).Info(fmt.Sprintf("Applying resource %s/%s in cluster: %s, namespace: %s", obj.GetKind(), obj.GetName(), k.config.Host, obj.GetNamespace())) + "serverSideApply", serverSideApply).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, true, func(ioStreams genericiooptions.IOStreams, fileName string) error { - cleanup, err := processKubectlRun(k.onKubectlRun, "apply") + // rbac resources are first applied with auth reconcile kubectl feature. + // This is not supported with server-side apply/diff. + // Server-side apply correctly handles the RBAC resources. + var outReconcile string + if !serverSideApply && obj.GetAPIVersion() == "rbac.authorization.k8s.io/v1" { + out, err := k.rbacReconcile(ctx, obj, dryRunStrategy) if err != nil { - return err + return "", fmt.Errorf("error running kubectl auth reconcile: %w", err) } - defer cleanup() + outReconcile = out + } + return k.runResourceCommand(ctx, obj, func(ioStreams genericiooptions.IOStreams, fileName string) error { applyOpts, err := k.newApplyOptions(ioStreams, obj, fileName, validate, force, serverSideApply, dryRunStrategy, manager) if err != nil { return err } - return applyOpts.Run() + _, err = ioStreams.Out.Write([]byte(outReconcile)) + if err != nil { + return fmt.Errorf("error writing reconcile output to stdout: %w", err) + } + return k.optionsRunner.Apply(applyOpts) }) } -func newApplyOptionsCommon(config *rest.Config, fact cmdutil.Factory, ioStreams genericiooptions.IOStreams, obj *unstructured.Unstructured, fileName string, validate bool, force, serverSideApply bool, dryRunStrategy cmdutil.DryRunStrategy, manager string) (*apply.ApplyOptions, error) { +func (k *kubectlResourceOperations) newApplyOptions(ioStreams genericiooptions.IOStreams, obj *unstructured.Unstructured, fileName string, validate bool, force, serverSideApply bool, dryRunStrategy cmdutil.DryRunStrategy, manager string) (*apply.ApplyOptions, error) { + if k.outputMode == outputModeJSON { + if dryRunStrategy != cmdutil.DryRunServer { + return nil, fmt.Errorf("invalid dry run strategy used with JSON output. : %d, expected %d", dryRunStrategy, cmdutil.DryRunServer) + } + if !serverSideApply { + return nil, errors.New("invalid Apply strategy used with JSON output. Must use server-side apply") + } + } + flags := apply.NewApplyFlags(ioStreams) o := &apply.ApplyOptions{ IOStreams: ioStreams, @@ -372,7 +422,7 @@ func newApplyOptionsCommon(config *rest.Config, fact cmdutil.Factory, ioStreams OpenAPIPatch: true, ServerSideApply: serverSideApply, } - dynamicClient, err := dynamic.NewForConfig(config) + dynamicClient, err := dynamic.NewForConfig(k.config) if err != nil { return nil, fmt.Errorf("failed to create dynamic client: %w", err) } @@ -381,70 +431,44 @@ func newApplyOptionsCommon(config *rest.Config, fact cmdutil.Factory, ioStreams if err != nil { return nil, fmt.Errorf("failed to create delete flags: %w", err) } - o.OpenAPIGetter = fact + o.OpenAPIGetter = k.fact o.DryRunStrategy = dryRunStrategy o.FieldManager = manager validateDirective := metav1.FieldValidationIgnore if validate { validateDirective = metav1.FieldValidationStrict } - o.Validator, err = fact.Validator(validateDirective) + o.Validator, err = k.fact.Validator(validateDirective) if err != nil { return nil, fmt.Errorf("failed to create validator: %w", err) } - o.Builder = fact.NewBuilder() - o.Mapper, err = fact.ToRESTMapper() + o.Builder = k.fact.NewBuilder() + o.Mapper, err = k.fact.ToRESTMapper() if err != nil { return nil, fmt.Errorf("failed to create restmapper: %w", err) } - o.DeleteOptions.Filenames = []string{fileName} o.Namespace = obj.GetNamespace() + o.DeleteOptions.Filenames = []string{fileName} o.DeleteOptions.ForceDeletion = force - o.DryRunStrategy = dryRunStrategy - if manager != "" { - o.FieldManager = manager - } - return o, nil -} - -func (k *kubectlServerSideDiffDryRunApplier) newApplyOptions(ioStreams genericiooptions.IOStreams, obj *unstructured.Unstructured, fileName string, validate bool, force, serverSideApply bool, dryRunStrategy cmdutil.DryRunStrategy, manager string) (*apply.ApplyOptions, error) { - o, err := newApplyOptionsCommon(k.config, k.fact, ioStreams, obj, fileName, validate, force, serverSideApply, dryRunStrategy, manager) - if err != nil { - return nil, err - } + o.ForceConflicts = serverSideApply o.ToPrinter = func(operation string) (printers.ResourcePrinter, error) { o.PrintFlags.NamePrintFlags.Operation = operation - if o.DryRunStrategy != cmdutil.DryRunServer { - return nil, fmt.Errorf("invalid dry run strategy passed to server-side diff dry run applier: %d, expected %d", o.DryRunStrategy, cmdutil.DryRunServer) - } - // managedFields are required by server-side diff to identify - // changes made by mutation webhooks. - o.PrintFlags.JSONYamlPrintFlags.ShowManagedFields = true - p, err := o.PrintFlags.JSONYamlPrintFlags.ToPrinter("json") - if err != nil { - return nil, fmt.Errorf("error configuring server-side diff printer: %w", err) - } - return p, nil - } - - o.ForceConflicts = true - if err := o.Validate(); err != nil { - return nil, fmt.Errorf("error validating options: %w", err) - } - return o, nil -} - -func (k *kubectlResourceOperations) newApplyOptions(ioStreams genericiooptions.IOStreams, obj *unstructured.Unstructured, fileName string, validate bool, force, serverSideApply bool, dryRunStrategy cmdutil.DryRunStrategy, manager string) (*apply.ApplyOptions, error) { - o, err := newApplyOptionsCommon(k.config, k.fact, ioStreams, obj, fileName, validate, force, serverSideApply, dryRunStrategy, manager) - if err != nil { - return nil, err - } + // For server-side diff (outputModeJSON), use JSON printer + if k.outputMode == outputModeJSON { + // managedFields are required by server-side diff to identify + // changes made by mutation webhooks. + o.PrintFlags.JSONYamlPrintFlags.ShowManagedFields = true + p, err := o.PrintFlags.JSONYamlPrintFlags.ToPrinter("json") + if err != nil { + return nil, fmt.Errorf("error configuring server-side diff printer: %w", err) + } + return p, nil + } - o.ToPrinter = func(operation string) (printers.ResourcePrinter, error) { - o.PrintFlags.NamePrintFlags.Operation = operation + // For normal output mode (outputModeLog), use name printer switch o.DryRunStrategy { case cmdutil.DryRunClient: err = o.PrintFlags.Complete("%s (dry run)") @@ -460,10 +484,6 @@ func (k *kubectlResourceOperations) newApplyOptions(ioStreams genericiooptions.I return o.PrintFlags.ToPrinter() } - if serverSideApply { - o.ForceConflicts = true - } - if err := o.Validate(); err != nil { return nil, fmt.Errorf("error validating options: %w", err) } @@ -471,6 +491,11 @@ func (k *kubectlResourceOperations) newApplyOptions(ioStreams genericiooptions.I } func (k *kubectlResourceOperations) newCreateOptions(ioStreams genericiooptions.IOStreams, fileName string, dryRunStrategy cmdutil.DryRunStrategy) (*create.CreateOptions, error) { + // JSON output mode is only supported for Apply operations + if k.outputMode == outputModeJSON { + return nil, errors.New("invalid output mode: CreateResource is not supported with JSON output mode") + } + o := create.NewCreateOptions(ioStreams) recorder, err := o.RecordFlags.ToRecorder() @@ -509,6 +534,11 @@ func (k *kubectlResourceOperations) newCreateOptions(ioStreams genericiooptions. } func (k *kubectlResourceOperations) newReplaceOptions(config *rest.Config, f cmdutil.Factory, ioStreams genericiooptions.IOStreams, fileName string, namespace string, force bool, dryRunStrategy cmdutil.DryRunStrategy) (*replace.ReplaceOptions, error) { + // JSON output mode is only supported for Apply operations + if k.outputMode == outputModeJSON { + return nil, errors.New("invalid output mode: ReplaceResource is not supported with JSON output mode") + } + o := replace.NewReplaceOptions(ioStreams) recorder, err := o.RecordFlags.ToRecorder() @@ -566,7 +596,7 @@ func (k *kubectlResourceOperations) newReplaceOptions(config *rest.Config, f cmd return o, nil } -func newReconcileOptions(f cmdutil.Factory, kubeClient *kubernetes.Clientset, fileName string, ioStreams genericiooptions.IOStreams, namespace string, dryRunStrategy cmdutil.DryRunStrategy) (*auth.ReconcileOptions, error) { +func newReconcileOptions(f cmdutil.Factory, kubeClient kubernetes.Interface, fileName string, ioStreams genericiooptions.IOStreams, namespace string, dryRunStrategy cmdutil.DryRunStrategy) (*auth.ReconcileOptions, error) { o := auth.NewReconcileOptions(ioStreams) o.RBACClient = kubeClient.RbacV1() o.NamespaceClient = kubeClient.CoreV1() @@ -598,66 +628,3 @@ func newReconcileOptions(f cmdutil.Factory, kubeClient *kubernetes.Clientset, fi } return o, nil } - -func (k *kubectlResourceOperations) authReconcile(ctx context.Context, obj *unstructured.Unstructured, manifestFile string, dryRunStrategy cmdutil.DryRunStrategy) (string, error) { - kubeClient, err := kubernetes.NewForConfig(k.config) - if err != nil { - return "", fmt.Errorf("error creating kube client: %w", err) - } - - clusterScoped := obj.GetKind() == "ClusterRole" || obj.GetKind() == "ClusterRoleBinding" - - // `kubectl auth reconcile` has a side effect of auto-creating namespaces if it doesn't exist. - // See: https://github.com/kubernetes/kubernetes/issues/71185. This is behavior which we do - // not want. We need to check if the namespace exists, before know if it is safe to run this - // command. Skip this for dryRuns. - - // When an Argo CD Application specifies destination.namespace, that namespace - // may be propagated even for cluster-scoped resources. Passing a namespace in - // this case causes `kubectl auth reconcile` to fail with: - // "namespaces not found" - // or may trigger unintended namespace handling behavior. - // Therefore, we skip namespace existence checks for cluster-scoped RBAC - // resources and allow reconcile to run without a namespace. - // - // https://github.com/argoproj/argo-cd/issues/24833 - if dryRunStrategy == cmdutil.DryRunNone && obj.GetNamespace() != "" && !clusterScoped { - _, err = kubeClient.CoreV1().Namespaces().Get(ctx, obj.GetNamespace(), metav1.GetOptions{}) - if err != nil { - return "", fmt.Errorf("error getting namespace %s: %w", obj.GetNamespace(), err) - } - } - ioStreams := genericiooptions.IOStreams{ - In: &bytes.Buffer{}, - Out: &bytes.Buffer{}, - ErrOut: &bytes.Buffer{}, - } - reconcileOpts, err := newReconcileOptions(k.fact, kubeClient, manifestFile, ioStreams, obj.GetNamespace(), dryRunStrategy) - if err != nil { - return "", fmt.Errorf("error calling newReconcileOptions: %w", err) - } - err = reconcileOpts.Validate() - if err != nil { - return "", errors.New(cleanKubectlOutput(err.Error())) - } - err = reconcileOpts.RunReconcile() - if err != nil { - return "", errors.New(cleanKubectlOutput(err.Error())) - } - - var out []string - if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); len(buf) > 0 { - out = append(out, buf) - } - if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); len(buf) > 0 { - out = append(out, buf) - } - return strings.Join(out, ". "), nil -} - -func processKubectlRun(onKubectlRun OnKubectlRunFunc, cmd string) (CleanupFunc, error) { - if onKubectlRun != nil { - return onKubectlRun(cmd) - } - return func() {}, nil -} diff --git a/gitops-engine/pkg/utils/kube/resource_ops_test.go b/gitops-engine/pkg/utils/kube/resource_ops_test.go index 3e8ca1d1eadf5..e0b1985938bd1 100644 --- a/gitops-engine/pkg/utils/kube/resource_ops_test.go +++ b/gitops-engine/pkg/utils/kube/resource_ops_test.go @@ -1,171 +1,506 @@ package kube import ( - "bytes" "context" "encoding/json" - "fmt" - "net/http" - "net/http/httptest" "testing" + "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube/mocks" testingutils "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/testing" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/tracing" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/cli-runtime/pkg/genericclioptions" - "k8s.io/cli-runtime/pkg/genericiooptions" + "k8s.io/cli-runtime/pkg/printers" + "k8s.io/client-go/kubernetes" + kubefake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" + "k8s.io/kubectl/pkg/cmd/apply" + "k8s.io/kubectl/pkg/cmd/create" + "k8s.io/kubectl/pkg/cmd/replace" cmdutil "k8s.io/kubectl/pkg/cmd/util" ) -func TestAuthReconcileWithMissingNamespace(t *testing.T) { - namespace := "test-ns" - fakeBearer := "fake-bearer" - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - status := &metav1.Status{ - Status: "Failure", - Message: fmt.Sprintf("namespace \"%s\" not found", namespace), - Reason: metav1.StatusReasonNotFound, - Code: http.StatusNotFound, - } - w.WriteHeader(http.StatusNotFound) - json.NewEncoder(w).Encode(status) - })) - defer server.Close() - - kubeConfigFlags := genericclioptions.NewConfigFlags(true) - kubeConfigFlags.Namespace = &namespace - kubeConfigFlags.APIServer = &server.URL - kubeConfigFlags.BearerToken = &fakeBearer - matchFlags := cmdutil.NewMatchVersionFlags(kubeConfigFlags) - fact := cmdutil.NewFactory(matchFlags) - - config := &rest.Config{Host: server.URL} +func newTestKubectlResourceOperations(t *testing.T) (*kubectlResourceOperations, *mocks.KubectlOptionsRunner) { + t.Helper() + + cmdMocks := mocks.NewKubectlOptionsRunner(t) + k := &kubectlResourceOperations{ - config: config, - fact: fact, + config: &rest.Config{}, + log: logr.Discard(), + tracer: &tracing.NopTracer{}, + fact: cmdutil.NewFactory(cmdutil.NewMatchVersionFlags(genericclioptions.NewConfigFlags(true))), + optionsRunner: cmdMocks, + getClientFunc: func() (kubernetes.Interface, error) { + return kubefake.NewSimpleClientset(), nil + }, + outputMode: outputModeLog, } + return k, cmdMocks +} + +func TestAuthReconcileWithMissingNamespace(t *testing.T) { + namespace := "test-ns" + + t.Run("Namespaced resources", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + + role := testingutils.NewRole() + role.SetNamespace(namespace) + + _, err := k.rbacReconcile(context.Background(), role, cmdutil.DryRunNone) + require.Error(t, err) + assert.Contains(t, err.Error(), `namespaces "test-ns" not found`) + + roleBinding := testingutils.NewRoleBinding() + roleBinding.SetNamespace(namespace) + + _, err = k.rbacReconcile(context.Background(), roleBinding, cmdutil.DryRunNone) + require.Error(t, err) + assert.Contains(t, err.Error(), `namespaces "test-ns" not found`) + }) + + t.Run("Cluster-scoped resources", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("AuthReconcile", mock.Anything).Return(nil).Twice() + + clusterRole := testingutils.NewClusterRole() + clusterRole.SetNamespace(namespace) + + _, err := k.rbacReconcile(context.Background(), clusterRole, cmdutil.DryRunNone) + require.NoError(t, err) + + clusterRoleBinding := testingutils.NewClusterRoleBinding() + clusterRoleBinding.SetNamespace(namespace) + + _, err = k.rbacReconcile(context.Background(), clusterRoleBinding, cmdutil.DryRunNone) + require.NoError(t, err) + }) +} + +func TestAuthReconcileUsage(t *testing.T) { + // This test verifies that the rbacReconcile logic is correctly applied based on the operation type + // and server-side apply setting. role := testingutils.NewRole() - role.SetNamespace(namespace) - _, err := k.authReconcile(context.Background(), role, "/dev/null", cmdutil.DryRunNone) - assert.Error(t, err) - assert.True(t, errors.IsNotFound(err), "returned error wasn't not found") + t.Run("CreateResource should not call auth reconcile", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Create", mock.Anything, mock.Anything, mock.Anything).Return(nil) - roleBinding := testingutils.NewRoleBinding() - roleBinding.SetNamespace(namespace) + _, err := k.CreateResource(t.Context(), role, cmdutil.DryRunNone, false) + require.NoError(t, err) + cmdMocks.AssertNotCalled(t, "AuthReconcile") + }) - _, err = k.authReconcile(context.Background(), roleBinding, "/dev/null", cmdutil.DryRunNone) - assert.Error(t, err) - assert.True(t, errors.IsNotFound(err), "returned error wasn't not found") + t.Run("ReplaceResource should not call auth reconcile", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Replace", mock.Anything, mock.Anything).Return(nil) - clusterRole := testingutils.NewClusterRole() - clusterRole.SetNamespace(namespace) + _, err := k.ReplaceResource(t.Context(), role, cmdutil.DryRunNone, false) + require.NoError(t, err) + cmdMocks.AssertNotCalled(t, "AuthReconcile") + }) + + t.Run("ApplyResource should not call auth reconcile on server-side apply", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Apply", mock.Anything).Return(nil) - _, err = k.authReconcile(context.Background(), clusterRole, "/dev/null", cmdutil.DryRunNone) - assert.NoError(t, err) + ssa := true + _, err := k.ApplyResource(t.Context(), role, cmdutil.DryRunNone, false, false, ssa, "") + require.NoError(t, err) + cmdMocks.AssertNotCalled(t, "AuthReconcile") + }) - clusterRoleBinding := testingutils.NewClusterRoleBinding() - clusterRoleBinding.SetNamespace(namespace) + t.Run("ApplyResource should call auth reconcile on client-side apply", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Apply", mock.Anything).Return(nil) + cmdMocks.On("AuthReconcile", mock.Anything).Return(nil) - _, err = k.authReconcile(context.Background(), clusterRoleBinding, "/dev/null", cmdutil.DryRunNone) - assert.NoError(t, err) + ssa := false + _, err := k.ApplyResource(t.Context(), role, cmdutil.DryRunNone, false, false, ssa, "") + require.NoError(t, err) + }) } -func TestRBACReconcileUsage(t *testing.T) { - var executedCommands []string - onKubectlRun := func(cmd string) (CleanupFunc, error) { - executedCommands = append(executedCommands, cmd) - return func() {}, nil - } +func TestOutputModeLog(t *testing.T) { + // Test normal flow operations with outputModeLog - k := &kubectlResourceOperations{ - onKubectlRun: onKubectlRun, - tracer: &tracing.NopTracer{}, - } + t.Run("CreateResource with outputModeLog", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Create", mock.Anything, mock.Anything, mock.Anything).Return(nil) - role := testingutils.NewRole() + obj := testingutils.NewPod() + _, err := k.CreateResource(t.Context(), obj, cmdutil.DryRunNone, false) + require.NoError(t, err) + }) - t.Run("CreateResource should NOT call rbacReconcile", func(t *testing.T) { - executedCommands = nil - _, err := k.runResourceCommand(context.Background(), role, cmdutil.DryRunClient, false, func(ioStreams genericiooptions.IOStreams, fileName string) error { - return nil - }) - assert.NoError(t, err) + t.Run("ReplaceResource with outputModeLog", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Replace", mock.Anything, mock.Anything).Return(nil) - for _, cmd := range executedCommands { - assert.NotEqual(t, "auth", cmd, "auth reconcile should NOT be called") - } + obj := testingutils.NewPod() + _, err := k.ReplaceResource(t.Context(), obj, cmdutil.DryRunNone, false) + require.NoError(t, err) + }) + + t.Run("ApplyResource with outputModeLog and client-side apply", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Apply", mock.Anything).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunNone, false, false, false, "test-manager") + require.NoError(t, err) + }) + + t.Run("ApplyResource with outputModeLog and server-side apply", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + cmdMocks.On("Apply", mock.Anything).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunNone, false, false, true, "test-manager") + require.NoError(t, err) + }) +} + +func TestOutputModeJSON(t *testing.T) { + // Test JSON output mode operations + + t.Run("CreateResource with outputModeJSON should fail", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + obj := testingutils.NewPod() + _, err := k.CreateResource(t.Context(), obj, cmdutil.DryRunServer, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "CreateResource is not supported with JSON output mode") + }) + + t.Run("ReplaceResource with outputModeJSON should fail", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + obj := testingutils.NewPod() + _, err := k.ReplaceResource(t.Context(), obj, cmdutil.DryRunServer, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "ReplaceResource is not supported with JSON output mode") + }) + + t.Run("ApplyResource with outputModeJSON without Dry run", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunNone, false, false, true, "test-manager") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid dry run strategy used with JSON output") + }) + + t.Run("ApplyResource with outputModeJSON requires DryRunServer", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunClient, false, false, true, "test-manager") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid dry run strategy used with JSON output") + }) + + t.Run("ApplyResource with outputModeJSON requires server-side apply", func(t *testing.T) { + k, _ := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunServer, false, false, false, "test-manager") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid Apply strategy used with JSON output") + }) + + t.Run("ApplyResource with outputModeJSON return object", func(t *testing.T) { + obj := testingutils.NewPod() + jsonObj, err := json.Marshal(obj) + require.NoError(t, err) + + k, cmdMocks := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + applyOpts := args[0].(*apply.ApplyOptions) + _, err := applyOpts.Out.Write(jsonObj) + require.NoError(t, err) + }).Return(nil) + + result, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunServer, false, false, true, "test-manager") + require.NoError(t, err) + assert.JSONEq(t, string(jsonObj), result) }) - t.Run("ReplaceResource should NOT call rbacReconcile", func(t *testing.T) { - executedCommands = nil - _, err := k.runResourceCommand(context.Background(), role, cmdutil.DryRunClient, false, func(ioStreams genericiooptions.IOStreams, fileName string) error { - return nil - }) - assert.NoError(t, err) + t.Run("ApplyResource with outputModeJSON with object and stderr returns object", func(t *testing.T) { + obj := testingutils.NewPod() + jsonObj, err := json.Marshal(obj) + require.NoError(t, err) - for _, cmd := range executedCommands { - assert.NotEqual(t, "auth", cmd, "auth reconcile should NOT be called") + k, cmdMocks := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + applyOpts := args[0].(*apply.ApplyOptions) + _, err := applyOpts.Out.Write(jsonObj) + require.NoError(t, err) + + // add an stderr message that should not be returned in the result + _, err = applyOpts.ErrOut.Write([]byte("error message")) + require.NoError(t, err) + }).Return(nil) + + result, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunServer, false, false, true, "test-manager") + require.NoError(t, err) + assert.JSONEq(t, string(jsonObj), result) + }) + + t.Run("ApplyResource with outputModeJSON without object with a stderr returns error", func(t *testing.T) { + obj := testingutils.NewPod() + + k, cmdMocks := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + applyOpts := args[0].(*apply.ApplyOptions) + + _, err := applyOpts.ErrOut.Write([]byte("error message")) + require.NoError(t, err) + }).Return(nil) + + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunServer, false, false, true, "test-manager") + require.Error(t, err) + assert.Contains(t, err.Error(), "error message") + }) +} + +func TestApplyOptionsConfiguration(t *testing.T) { + // Test that newApplyOptions correctly configures all ApplyOptions fields + t.Run("general options are correctly set", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + }{ + {"DryRunNone", cmdutil.DryRunNone}, + {"DryRunClient", cmdutil.DryRunClient}, + {"DryRunServer", cmdutil.DryRunServer}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *apply.ApplyOptions + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*apply.ApplyOptions) + }).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, tc.strategy, false, false, false, "test-manager") + require.NoError(t, err) + + assert.Equal(t, tc.strategy, capturedOpts.DryRunStrategy) + assert.Equal(t, "test-manager", capturedOpts.FieldManager) + assert.True(t, capturedOpts.Overwrite) + assert.True(t, capturedOpts.OpenAPIPatch) + assert.False(t, capturedOpts.ServerSideApply) + assert.False(t, capturedOpts.ForceConflicts) + }) } }) - t.Run("Simulation of original issue: when reconcileRBAC is TRUE, it should fail if resource is created by reconcile first", func(t *testing.T) { - // This test simulates the BUGGY behavior (passing reconcileRBAC=true to runResourceCommand) - // and shows why it fails with "already exists". - var executedCommands []string - authCalled := false + t.Run("serverSideApply=true sets ServerSideApply=true and ForceConflicts=true", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + expectedError string + }{ + {"DryRunNone", cmdutil.DryRunNone, ""}, + {"DryRunClient", cmdutil.DryRunClient, "error validating options: --dry-run=client doesn't work with --server-side"}, + {"DryRunServer", cmdutil.DryRunServer, ""}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *apply.ApplyOptions + if tc.expectedError == "" { + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*apply.ApplyOptions) + }).Return(nil) + } - kWithAuth := &kubectlResourceOperations{ - onKubectlRun: func(cmd string) (CleanupFunc, error) { - executedCommands = append(executedCommands, cmd) - if cmd == "auth" { - authCalled = true + ssa := true + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, tc.strategy, false, false, ssa, "test-manager") + + if tc.expectedError == "" { + require.NoError(t, err) + assert.True(t, capturedOpts.ServerSideApply) + assert.True(t, capturedOpts.ForceConflicts) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) } - return func() {}, nil - }, - tracer: &tracing.NopTracer{}, + }) + } + }) + + t.Run("force=true sets DeleteOptions.ForceDeletion", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + }{ + {"DryRunNone", cmdutil.DryRunNone}, + {"DryRunClient", cmdutil.DryRunClient}, + {"DryRunServer", cmdutil.DryRunServer}, } - // Mock runResourceCommand behavior manually to avoid calling real authReconcile which panics due to nil fact/config - runResourceCommandMock := func(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, reconcileRBAC bool, executor commandExecutor) (string, error) { - if reconcileRBAC && obj.GetAPIVersion() == "rbac.authorization.k8s.io/v1" { - _, err := kWithAuth.onKubectlRun("auth") + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *apply.ApplyOptions + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*apply.ApplyOptions) + }).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunNone, true, false, false, "") require.NoError(t, err) - } - ioStreams := genericiooptions.IOStreams{ - In: &bytes.Buffer{}, - Out: &bytes.Buffer{}, - ErrOut: &bytes.Buffer{}, - } - return "", executor(ioStreams, "") + + assert.True(t, capturedOpts.DeleteOptions.ForceDeletion) + }) } + }) + + t.Run("outputModeJSON returns JSONPrinter", func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + k.outputMode = outputModeJSON + + var capturedOpts *apply.ApplyOptions + cmdMocks.On("Apply", mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*apply.ApplyOptions) + }).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ApplyResource(t.Context(), obj, cmdutil.DryRunServer, false, false, true, "test-manager") + require.NoError(t, err) - executor := func(ioStreams genericiooptions.IOStreams, fileName string) error { - if authCalled { - // Simulate the "already exists" error that happens when kubectl create/replace - // is called after kubectl auth reconcile has already created the resource. - return fmt.Errorf("roles.rbac.authorization.k8s.io \"mytestrole\" already exists") - } - return nil + // Call ToPrinter and verify it returns a JSON printer + printer, err := capturedOpts.ToPrinter("configured") + require.NoError(t, err) + assert.NotNil(t, printer) + + // Verify it's a JSONPrinter by checking the type + _, isJSONPrinter := printer.(*printers.JSONPrinter) + assert.True(t, isJSONPrinter, "Expected printer to be of type *printers.JSONPrinter") + + // Verify ShowManagedFields is set to true for JSON output + assert.True(t, capturedOpts.PrintFlags.JSONYamlPrintFlags.ShowManagedFields) + }) +} + +func TestCreateOptionsConfiguration(t *testing.T) { + // Test that newCreateOptions correctly configures all CreateOptions fields + + t.Run("general options are correctly set", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + }{ + {"DryRunNone", cmdutil.DryRunNone}, + {"DryRunClient", cmdutil.DryRunClient}, + {"DryRunServer", cmdutil.DryRunServer}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *create.CreateOptions + cmdMocks.On("Create", mock.Anything, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*create.CreateOptions) + }).Return(nil) + + obj := testingutils.NewPod() + _, err := k.CreateResource(t.Context(), obj, tc.strategy, false) + require.NoError(t, err) + + assert.Equal(t, tc.strategy, capturedOpts.DryRunStrategy) + assert.NotEmpty(t, capturedOpts.FilenameOptions.Filenames) + assert.NotNil(t, capturedOpts.PrintObj) + }) + } + }) +} + +func TestReplaceOptionsConfiguration(t *testing.T) { + // Test that newReplaceOptions correctly configures all ReplaceOptions fields + + t.Run("general options are correctly set", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + }{ + {"DryRunNone", cmdutil.DryRunNone}, + {"DryRunClient", cmdutil.DryRunClient}, + {"DryRunServer", cmdutil.DryRunServer}, } - // If we call it with reconcileRBAC = true (the bug), it should fail - _, err := runResourceCommandMock(context.Background(), role, cmdutil.DryRunClient, true, executor) - assert.Error(t, err) - assert.Contains(t, err.Error(), "already exists") + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *replace.ReplaceOptions + cmdMocks.On("Replace", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*replace.ReplaceOptions) + }).Return(nil) + + obj := testingutils.NewPod() + obj.SetNamespace("test-namespace") + _, err := k.ReplaceResource(t.Context(), obj, tc.strategy, false) + require.NoError(t, err) + + assert.Equal(t, tc.strategy, capturedOpts.DryRunStrategy) + assert.False(t, capturedOpts.DeleteOptions.ForceDeletion) + assert.NotEmpty(t, capturedOpts.DeleteOptions.Filenames) + assert.Equal(t, "test-namespace", capturedOpts.Namespace) + assert.NotNil(t, capturedOpts.PrintObj) + }) + } + }) - // If we call it with reconcileRBAC = false (the fix), it should succeed - authCalled = false - executedCommands = nil - _, err = runResourceCommandMock(context.Background(), role, cmdutil.DryRunClient, false, executor) - assert.NoError(t, err) + t.Run("force=true sets DeleteOptions.ForceDeletion correctly", func(t *testing.T) { + testCases := []struct { + name string + strategy cmdutil.DryRunStrategy + expected bool + }{ + {"DryRunNone", cmdutil.DryRunNone, true}, + {"DryRunClient", cmdutil.DryRunClient, false}, + {"DryRunServer", cmdutil.DryRunServer, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + k, cmdMocks := newTestKubectlResourceOperations(t) + + var capturedOpts *replace.ReplaceOptions + cmdMocks.On("Replace", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + capturedOpts = args[0].(*replace.ReplaceOptions) + }).Return(nil) + + obj := testingutils.NewPod() + _, err := k.ReplaceResource(t.Context(), obj, tc.strategy, true) + require.NoError(t, err) + + assert.Equal(t, tc.expected, capturedOpts.DeleteOptions.ForceDeletion) + }) + } }) } diff --git a/hack/generate-mock.sh b/hack/generate-mock.sh index 8a89b87738c9e..6c97496b8ed08 100755 --- a/hack/generate-mock.sh +++ b/hack/generate-mock.sh @@ -16,3 +16,7 @@ PATH="${PROJECT_ROOT}/dist:${PATH}" mockery version mockery --config "${PROJECT_ROOT}"/.mockery.yaml + +# Generate mocks for gitops-engine +cd "${PROJECT_ROOT}"/gitops-engine +mockery --config .mockery.yaml diff --git a/server/application/application.go b/server/application/application.go index c87f06629d0b5..dffdef9761e73 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -2949,12 +2949,12 @@ func (s *Server) ServerSideDiff(ctx context.Context, q *application.ApplicationS } // Create server-side diff dry run applier - openAPISchema, gvkParser, err := s.kubectl.LoadOpenAPISchema(clusterConfig) + _, gvkParser, err := s.kubectl.LoadOpenAPISchema(clusterConfig) if err != nil { return nil, fmt.Errorf("failed to get OpenAPI schema: %w", err) } - applier, cleanup, err := kubeutil.ManageServerSideDiffDryRuns(clusterConfig, openAPISchema, func(_ string) (kube.CleanupFunc, error) { + applier, cleanup, err := kubeutil.ManageServerSideDiffDryRuns(clusterConfig, func(_ string) (kube.CleanupFunc, error) { return func() {}, nil }) if err != nil { diff --git a/server/application/application_test.go b/server/application/application_test.go index 59b600195646a..d77704854b7a4 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -15,7 +15,6 @@ import ( "k8s.io/apimachinery/pkg/labels" cmdutil "k8s.io/kubectl/pkg/cmd/util" - "k8s.io/kubectl/pkg/util/openapi" "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" "github.com/argoproj/argo-cd/gitops-engine/pkg/health" @@ -4753,7 +4752,7 @@ func TestServerSideDiff(t *testing.T) { // Create mock kubectl that returns our custom applier mockKubectl := &kubetest.MockKubectlCmd{} - mockKubectl.WithManageServerSideDiffDryRunFunc(func(_ *rest.Config, _ openapi.Resources) (diff.KubeApplier, func(), error) { + mockKubectl.WithManageServerSideDiffDryRunFunc(func(_ *rest.Config) (diff.KubeApplier, func(), error) { return mockApplier, func() {}, nil }) diff --git a/util/kube/kubectl.go b/util/kube/kubectl.go index 8147df306a00f..f332f2fafaed9 100644 --- a/util/kube/kubectl.go +++ b/util/kube/kubectl.go @@ -5,7 +5,6 @@ import ( "github.com/go-logr/logr" "k8s.io/client-go/rest" - "k8s.io/kubectl/pkg/util/openapi" "github.com/argoproj/argo-cd/v3/util/log" @@ -29,11 +28,11 @@ func NewKubectl() kube.Kubectl { return &kube.KubectlCmd{Tracer: tracer, Log: logger} } -func ManageServerSideDiffDryRuns(config *rest.Config, openAPISchema openapi.Resources, onKubectlRun kube.OnKubectlRunFunc) (diff.KubeApplier, func(), error) { +func ManageServerSideDiffDryRuns(config *rest.Config, onKubectlRun kube.OnKubectlRunFunc) (diff.KubeApplier, func(), error) { k := &kube.KubectlCmd{ Log: logger, Tracer: tracer, OnKubectlRun: onKubectlRun, } - return k.ManageServerSideDiffDryRuns(config, openAPISchema) + return k.ManageServerSideDiffDryRuns(config) }