Repository: istio/istio (go)

Issue:
Install regression, cannot install using istioctl at all
#25384 broke this

```
$ go run ./istioctl/cmd/istioctl install -d manifests
This will install the default Istio profile into the cluster. Proceed? (y/N) y
Error: failed to apply manifests: open installPackagePath=manifests: no such file or directory
exit status 1
```

It seems the change also fundamentally drops out test coverage as this was not caught

Pull request:
Fix breakage in istioctl manifest apply
This was introduced by #25384. The issue was that the parameter order was swapped in order to be consistent with GenerateConfig (and a few other places). There was apparently no test coverage for this beyond the integration test framework's usage when installing istio. Unfortunately, we don't want to use the commands directly there because the use of flags interferes with the ability to install multiple control planes in parallel.

Fixing the issue by swapping to a structure for the parameter to avoid similar breakage in the future.

Fixes #25447



[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Patch:
diff --git a/operator/cmd/mesh/manifest-apply.go b/operator/cmd/mesh/manifest-apply.go
index 1dfbf149b2..6934715292 100644
--- a/operator/cmd/mesh/manifest-apply.go
+++ b/operator/cmd/mesh/manifest-apply.go
@@ -64,6 +64,21 @@ type manifestApplyArgs struct {
 	revision string
 }
 
+func (args manifestApplyArgs) toOptions(rootArgs *rootArgs) (ApplyManifestsOptions, error) {
+	client, err := k8sClient(args.kubeConfigPath, args.context)
+	if err != nil {
+		return ApplyManifestsOptions{}, err
+	}
+	return ApplyManifestsOptions{
+		Client:      client,
+		InFilenames: args.inFilenames,
+		SetOverlay:  applyFlagAliases(args.set, args.manifestsPath, args.revision),
+		Force:       args.force,
+		DryRun:      rootArgs.dryRun,
+		WaitTimeout: args.readinessTimeout,
+	}, nil
+}
+
 func addManifestApplyFlags(cmd *cobra.Command, args *manifestApplyArgs) {
 	cmd.PersistentFlags().StringSliceVarP(&args.inFilenames, "filename", "f", nil, filenameFlagHelpStr)
 	cmd.PersistentFlags().StringVarP(&args.kubeConfigPath, "kubeconfig", "c", "", "Path to kube config.")
@@ -146,26 +161,40 @@ func runApplyCmd(cmd *cobra.Command, rootArgs *rootArgs, maArgs *manifestApplyAr
 	if err := configLogs(logOpts); err != nil {
 		return fmt.Errorf("could not configure logs: %s", err)
 	}
-	client, err := k8sClient(maArgs.kubeConfigPath, maArgs.context)
+
+	opts, err := maArgs.toOptions(rootArgs)
 	if err != nil {
 		return err
 	}
-	if err := ApplyManifests(applyFlagAliases(maArgs.set, maArgs.manifestsPath, maArgs.revision), maArgs.inFilenames,
-		maArgs.force, rootArgs.dryRun, client, maArgs.readinessTimeout, l); err != nil {
+
+	if err := ApplyManifests(opts, l); err != nil {
 		return fmt.Errorf("failed to apply manifests: %v", err)
 	}
 
 	return nil
 }
 
+// ApplyManifestOptions options for the apply manifests command.
+type ApplyManifestsOptions struct {
+	Client      kube.Client
+	InFilenames []string
+	SetOverlay  []string
+	Force       bool
+	DryRun      bool
+	WaitTimeout time.Duration
+}
+
 // ApplyManifests generates manifests from the given input files and --set flag overlays and applies them to the
 // cluster. See GenManifests for more description of the manifest generation process.
 //  force   validation warnings are written to logger but command is not aborted
 //  dryRun  all operations are done but nothing is written
-func ApplyManifests(inFilenames, setOverlay []string, force bool, dryRun bool,
-	client kube.Client, waitTimeout time.Duration, l clog.Logger) error {
-
-	_, iops, err := GenerateConfig(inFilenames, setOverlay, force, client.RESTConfig(), l)
+func ApplyManifests(opts ApplyManifestsOptions, l clog.Logger) error {
+	_, iops, err := GenerateConfig(GenerateConfigOptions{
+		InFilenames: opts.InFilenames,
+		SetOverlay:  opts.SetOverlay,
+		Force:       opts.Force,
+		KubeConfig:  opts.Client.RESTConfig(),
+	}, l)
 	if err != nil {
 		return err
 	}
@@ -179,15 +208,20 @@ func ApplyManifests(inFilenames, setOverlay []string, force bool, dryRun bool,
 		return err
 	}
 
-	if err := createNamespace(client.Kube(), iop.Namespace); err != nil {
+	if err := createNamespace(opts.Client.Kube(), iop.Namespace); err != nil {
 		return err
 	}
 
 	// Needed in case we are running a test through this path that doesn't start a new process.
 	cache.FlushObjectCaches()
-	opts := &helmreconciler.Options{DryRun: dryRun, Log: l, WaitTimeout: waitTimeout, ProgressLog: progress.NewLog(),
-		Force: force}
-	reconciler, err := helmreconciler.NewHelmReconciler(client.Controller(), client.RESTConfig(), iop, opts)
+	helmOpts := &helmreconciler.Options{
+		DryRun:      opts.DryRun,
+		Log:         l,
+		WaitTimeout: opts.WaitTimeout,
+		ProgressLog: progress.NewLog(),
+		Force:       opts.Force,
+	}
+	reconciler, err := helmreconciler.NewHelmReconciler(opts.Client.Controller(), opts.Client.RESTConfig(), iop, helmOpts)
 	if err != nil {
 		return err
 	}
@@ -199,7 +233,7 @@ func ApplyManifests(inFilenames, setOverlay []string, force bool, dryRun bool,
 		return fmt.Errorf("errors occurred during operation")
 	}
 
-	opts.ProgressLog.SetState(progress.StateComplete)
+	helmOpts.ProgressLog.SetState(progress.StateComplete)
 
 	// Save state to cluster in IstioOperator CR.
 	iopStr, err := translate.IOPStoIOPstr(iops, crName, iopv1alpha1.Namespace(iops))
diff --git a/operator/cmd/mesh/manifest-generate.go b/operator/cmd/mesh/manifest-generate.go
index 575998dcdc..b44d00606f 100644
--- a/operator/cmd/mesh/manifest-generate.go
+++ b/operator/cmd/mesh/manifest-generate.go
@@ -129,7 +129,12 @@ func manifestGenerate(args *rootArgs, mgArgs *manifestGenerateArgs, logopts *log
 // supplied logger.
 func GenManifests(inFilename []string, setOverlay []string, force bool,
 	kubeConfig *rest.Config, l clog.Logger) (name.ManifestMap, *v1alpha1.IstioOperatorSpec, error) {
-	mergedYAML, _, err := GenerateConfig(inFilename, setOverlay, force, kubeConfig, l)
+	mergedYAML, _, err := GenerateConfig(GenerateConfigOptions{
+		InFilenames: inFilename,
+		SetOverlay:  setOverlay,
+		Force:       force,
+		KubeConfig:  kubeConfig,
+	}, l)
 	if err != nil {
 		return nil, nil, err
 	}
diff --git a/operator/cmd/mesh/profile-common.go b/operator/cmd/mesh/profile-common.go
index fab8a1c356..33580da3e2 100644
--- a/operator/cmd/mesh/profile-common.go
+++ b/operator/cmd/mesh/profile-common.go
@@ -36,6 +36,14 @@ import (
 	pkgversion "istio.io/pkg/version"
 )
 
+// GenerateConfigOptions provides options for the generate command.
+type GenerateConfigOptions struct {
+	InFilenames []string
+	SetOverlay  []string
+	Force       bool
+	KubeConfig  *rest.Config
+}
+
 // GenerateConfig creates an IstioOperatorSpec from the following sources, overlaid sequentially:
 // 1. Compiled in base, or optionally base from paths pointing to one or multiple ICP/IOP files at inFilenames.
 // 2. Profile overlay, if non-default overlay is selected. This also comes either from compiled in or path specified in IOP contained in inFilenames.
@@ -47,18 +55,17 @@ import (
 // Otherwise it will be the compiled in profile YAMLs.
 // In step 3, the remaining fields in the same user overlay are applied on the resulting profile base.
 // The force flag causes validation errors not to abort but only emit log/console warnings.
-func GenerateConfig(inFilenames, setOverlay []string, force bool, kubeConfig *rest.Config,
-	l clog.Logger) (string, *v1alpha1.IstioOperatorSpec, error) {
-	if err := validateSetFlags(setOverlay); err != nil {
+func GenerateConfig(opts GenerateConfigOptions, l clog.Logger) (string, *v1alpha1.IstioOperatorSpec, error) {
+	if err := validateSetFlags(opts.SetOverlay); err != nil {
 		return "", nil, err
 	}
 
-	fy, profile, err := readYamlProfile(inFilenames, setOverlay, force, l)
+	fy, profile, err := readYamlProfile(opts.InFilenames, opts.SetOverlay, opts.Force, l)
 	if err != nil {
 		return "", nil, err
 	}
 
-	iopsString, iops, err := genIOPSFromProfile(profile, fy, setOverlay, force, kubeConfig, l)
+	iopsString, iops, err := genIOPSFromProfile(profile, fy, opts.SetOverlay, opts.Force, opts.KubeConfig, l)
 	if err != nil {
 		return "", nil, err
 	}
diff --git a/operator/cmd/mesh/profile-dump.go b/operator/cmd/mesh/profile-dump.go
index 0f3ca618ae..d1575c62f9 100644
--- a/operator/cmd/mesh/profile-dump.go
+++ b/operator/cmd/mesh/profile-dump.go
@@ -129,8 +129,11 @@ func profileDump(args []string, rootArgs *rootArgs, pdArgs *profileDumpArgs, l c
 	if len(args) == 1 {
 		setFlags = append(setFlags, "profile="+args[0])
 	}
-
-	y, _, err := GenerateConfig(pdArgs.inFilenames, setFlags, true, nil, l)
+	y, _, err := GenerateConfig(GenerateConfigOptions{
+		InFilenames: pdArgs.inFilenames,
+		SetOverlay:  setFlags,
+		Force:       true,
+	}, l)
 	if err != nil {
 		return err
 	}
diff --git a/operator/cmd/mesh/shared.go b/operator/cmd/mesh/shared.go
index 7f3eec918e..d405c36a8b 100644
--- a/operator/cmd/mesh/shared.go
+++ b/operator/cmd/mesh/shared.go
@@ -161,7 +161,9 @@ func getCRAndNamespaceFromFile(filePath string, l clog.Logger) (customResource s
 		return "", "", nil
 	}
 
-	_, mergedIOPS, err := GenerateConfig([]string{filePath}, nil, false, nil, l)
+	_, mergedIOPS, err := GenerateConfig(GenerateConfigOptions{
+		InFilenames: []string{filePath},
+	}, l)
 	if err != nil {
 		return "", "", err
 	}
diff --git a/operator/cmd/mesh/upgrade.go b/operator/cmd/mesh/upgrade.go
index 064d9d42b3..a79bc3bbd0 100644
--- a/operator/cmd/mesh/upgrade.go
+++ b/operator/cmd/mesh/upgrade.go
@@ -136,7 +136,11 @@ func upgrade(rootArgs *rootArgs, args *upgradeArgs, l clog.Logger) (err error) {
 	}
 	setFlags := applyFlagAliases(args.set, args.manifestsPath, "")
 	// Generate IOPS parseObjectSetFromManifest
-	targetIOPSYaml, targetIOPS, err := GenerateConfig(args.inFilenames, setFlags, args.force, nil, l)
+	targetIOPSYaml, targetIOPS, err := GenerateConfig(GenerateConfigOptions{
+		InFilenames: args.inFilenames,
+		SetOverlay:  setFlags,
+		Force:       args.force,
+	}, l)
 	if err != nil {
 		return fmt.Errorf("failed to generate Istio configs from file %s, error: %s", args.inFilenames, err)
 	}
@@ -206,8 +210,14 @@ func upgrade(rootArgs *rootArgs, args *upgradeArgs, l clog.Logger) (err error) {
 	waitForConfirmation(args.skipConfirmation, l)
 
 	// Apply the Istio Control Plane specs reading from inFilenames to the cluster
-	err = ApplyManifests(applyFlagAliases(args.set, args.manifestsPath, ""), args.inFilenames, args.force, rootArgs.dryRun,
-		client, args.readinessTimeout, l)
+	err = ApplyManifests(ApplyManifestsOptions{
+		Client:      nil,
+		InFilenames: args.inFilenames,
+		SetOverlay:  applyFlagAliases(args.set, args.manifestsPath, ""),
+		Force:       args.force,
+		DryRun:      rootArgs.dryRun,
+		WaitTimeout: args.readinessTimeout,
+	}, l)
 	if err != nil {
 		return fmt.Errorf("failed to apply the Istio Control Plane specs. Error: %v", err)
 	}


Test patch:
diff --git a/operator/cmd/mesh/manifest-generate_test.go b/operator/cmd/mesh/manifest-generate_test.go
index 86e20c4353..b04305a7af 100644
--- a/operator/cmd/mesh/manifest-generate_test.go
+++ b/operator/cmd/mesh/manifest-generate_test.go
@@ -650,7 +650,10 @@ func TestLDFlags(t *testing.T) {
 	version.DockerInfo.Hub = "testHub"
 	version.DockerInfo.Tag = "testTag"
 	l := clog.NewConsoleLogger(os.Stdout, os.Stderr, installerScope)
-	_, iops, err := GenerateConfig(nil, []string{"installPackagePath=" + string(liveCharts)}, true, nil, l)
+	_, iops, err := GenerateConfig(GenerateConfigOptions{
+		SetOverlay: []string{"installPackagePath=" + string(liveCharts)},
+		Force:      true,
+	}, l)
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/pkg/test/framework/components/istio/operator.go b/pkg/test/framework/components/istio/operator.go
index 44c4d3a643..3d3b2dc840 100644
--- a/pkg/test/framework/components/istio/operator.go
+++ b/pkg/test/framework/components/istio/operator.go
@@ -498,7 +498,14 @@ func applyManifest(ctx resource.Context, inFiles, setOverlay []string, cluster r
 	}
 
 	scopes.Framework.Infof("Running istio control plane on cluster %s", cluster.Name())
-	err = mesh.ApplyManifests(inFiles, setOverlay, false, false, cluster, 300*time.Second, l)
+	err = mesh.ApplyManifests(mesh.ApplyManifestsOptions{
+		Client:      cluster,
+		InFilenames: inFiles,
+		SetOverlay:  setOverlay,
+		Force:       false,
+		DryRun:      false,
+		WaitTimeout: 300 * time.Second,
+	}, l)
 	if err != nil {
 		return fmt.Errorf("manifest apply failed: %v", err)
 	}