Fully support installPackagePath (#23520)

* Fully support installPackagePath Currently the profiles are always grabbed from the VFS in some cases. This is especially broken in development, as the assets.gen.go is not updated. This changes the last few areas to properly support installPackagePath. As a result, we no longer check in the profile to assets.gen.go to github (it is added at release time). * gen
istio · May 5, 2020 · 6b17a18 · 6b17a18
1 parent 864835a
commit 6b17a18
Show file tree

Hide file tree

Showing 12 changed files with 115 additions and 969 deletions.
diff --git a/bin/nomodify.md5 b/bin/nomodify.md5
@@ -1 +1 @@
-cd928894875e6ba84de7dd13bb7ceda9  operator/pkg/vfs/assets.gen.go
+03d81631567676f312f383cb193d539d  operator/pkg/vfs/assets.gen.go
diff --git a/operator/cmd/mesh/manifest-generate_test.go b/operator/cmd/mesh/manifest-generate_test.go
@@ -641,7 +641,7 @@ func runManifestGenerate(filenames []string, flags string, chartSource chartSour
 	}
 	switch chartSource {
 	case snapshotCharts:
-		args += " --set installPackagePath=" + filepath.Join(testDataDir, "data-snapshot")
+		args += " --set installPackagePath=" + snapshotInstallPackageDir
 	case liveCharts:
 		args += " --set installPackagePath=" + liveInstallPackageDir
 	case compiledInCharts:

diff --git a/operator/cmd/mesh/profile-diff.go b/operator/cmd/mesh/profile-diff.go
@@ -25,7 +25,16 @@ import (
 	"istio.io/istio/operator/pkg/helm"
 )
 
-func profileDiffCmd(rootArgs *rootArgs) *cobra.Command {
+type profileDiffArgs struct {
+	// charts is a path to a charts and profiles directory in the local filesystem, or URL with a release tgz.
+	charts string
+}
+
+func addProfileDiffFlags(cmd *cobra.Command, args *profileDiffArgs) {
+	cmd.PersistentFlags().StringVarP(&args.charts, "charts", "d", "", ChartsFlagHelpStr)
+}
+
+func profileDiffCmd(rootArgs *rootArgs, pfArgs *profileDiffArgs) *cobra.Command {
 	return &cobra.Command{
 		Use:   "diff <file1.yaml> <file2.yaml>",
 		Short: "Diffs two Istio configuration profiles",
@@ -37,21 +46,21 @@ func profileDiffCmd(rootArgs *rootArgs) *cobra.Command {
 			return nil
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return profileDiff(rootArgs, args)
+			return profileDiff(rootArgs, pfArgs, args)
 		}}
 
 }
 
 // profileDiff compare two profile files.
-func profileDiff(rootArgs *rootArgs, args []string) error {
+func profileDiff(rootArgs *rootArgs, pfArgs *profileDiffArgs, args []string) error {
 	initLogsOrExit(rootArgs)
 
-	a, err := helm.ReadProfileYAML(args[0])
+	a, err := helm.ReadProfileYAML(args[0], pfArgs.charts)
 	if err != nil {
 		return fmt.Errorf("could not read %q: %v", args[0], err)
 	}
 
-	b, err := helm.ReadProfileYAML(args[1])
+	b, err := helm.ReadProfileYAML(args[1], pfArgs.charts)
 	if err != nil {
 		return fmt.Errorf("could not read %q: %v", args[1], err)
 	}

diff --git a/operator/cmd/mesh/profile-dump_test.go b/operator/cmd/mesh/profile-dump_test.go
@@ -17,6 +17,7 @@ package mesh
 import (
 	"io/ioutil"
 	"path/filepath"
+	"regexp"
 	"testing"
 
 	"istio.io/istio/operator/pkg/util"
@@ -36,15 +37,18 @@ func TestProfileDump(t *testing.T) {
 			configPath: "components",
 		},
 	}
+	installPackagePathRegex := regexp.MustCompile("  installPackagePath: .*")
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			inPath := filepath.Join(testDataDir, "input", tt.desc+".yaml")
 			outPath := filepath.Join(testDataDir, "output", tt.desc+".yaml")
 
-			got, err := runProfileDump(inPath, tt.configPath)
+			got, err := runProfileDump(inPath, tt.configPath, snapshotCharts)
 			if err != nil {
 				t.Fatal(err)
 			}
+			// installPackagePath may change, we will remove it for consistent output
+			got = installPackagePathRegex.ReplaceAllString(got, "")
 
 			if refreshGoldenFiles() {
 				t.Logf("Refreshing golden file for %s", outPath)
@@ -64,10 +68,16 @@ func TestProfileDump(t *testing.T) {
 	}
 }
 
-func runProfileDump(profilePath, configPath string) (string, error) {
+func runProfileDump(profilePath, configPath string, chartSource chartSourceType) (string, error) {
 	cmd := "profile dump -f " + profilePath
 	if configPath != "" {
 		cmd += " --config-path " + configPath
 	}
+	switch chartSource {
+	case liveCharts:
+		cmd += " --charts=" + liveInstallPackageDir
+	default:
+		cmd += " --charts=" + snapshotInstallPackageDir
+	}
 	return runCommand(cmd)
 }
diff --git a/operator/cmd/mesh/profile-list.go b/operator/cmd/mesh/profile-list.go
@@ -22,22 +22,34 @@ import (
 	"istio.io/istio/operator/pkg/helm"
 )
 
-func profileListCmd(rootArgs *rootArgs) *cobra.Command {
+type profileListArgs struct {
+	// charts is a path to a charts and profiles directory in the local filesystem, or URL with a release tgz.
+	charts string
+}
+
+func addProfileListFlags(cmd *cobra.Command, args *profileListArgs) {
+	cmd.PersistentFlags().StringVarP(&args.charts, "charts", "d", "", ChartsFlagHelpStr)
+}
+
+func profileListCmd(rootArgs *rootArgs, plArgs *profileListArgs) *cobra.Command {
 	return &cobra.Command{
 		Use:   "list",
 		Short: "Lists available Istio configuration profiles",
 		Long:  "The list subcommand lists the available Istio configuration profiles.",
 		Args:  cobra.ExactArgs(0),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return profileList(rootArgs)
+			return profileList(rootArgs, plArgs)
 		}}
 
 }
 
 // profileList list all the builtin profiles.
-func profileList(args *rootArgs) error {
+func profileList(args *rootArgs, plArgs *profileListArgs) error {
 	initLogsOrExit(args)
-	profiles := helm.ListBuiltinProfiles()
+	profiles, err := helm.ListProfiles(plArgs.charts)
+	if err != nil {
+		return err
+	}
 	if len(profiles) == 0 {
 		fmt.Println("No profiles available.")
 	} else {

diff --git a/operator/cmd/mesh/profile.go b/operator/cmd/mesh/profile.go
@@ -29,18 +29,22 @@ func ProfileCmd() *cobra.Command {
 	}
 
 	pdArgs := &profileDumpArgs{}
+	plArgs := &profileListArgs{}
+	pdfArgs := &profileDiffArgs{}
 	args := &rootArgs{}
 
-	plc := profileListCmd(args)
+	plc := profileListCmd(args, plArgs)
 	pdc := profileDumpCmd(args, pdArgs)
-	pdfc := profileDiffCmd(args)
+	pdfc := profileDiffCmd(args, pdfArgs)
 
 	addFlags(pc, args)
 	addFlags(plc, args)
 	addFlags(pdc, args)
 	addFlags(pdfc, args)
 
 	addProfileDumpFlags(pdc, pdArgs)
+	addProfileListFlags(plc, plArgs)
+	addProfileDiffFlags(pdfc, pdfArgs)
 
 	pc.AddCommand(plc)
 	pc.AddCommand(pdc)

diff --git a/operator/cmd/mesh/testdata/profile-dump/output/all_off.yaml b/operator/cmd/mesh/testdata/profile-dump/output/all_off.yaml
@@ -97,8 +97,8 @@ spec:
         service:
           ports:
           - name: status-port
-            port: 15021
-            targetPort: 15021
+            port: 15020
+            targetPort: 15020
           - name: http2
             port: 80
             targetPort: 8080
@@ -201,13 +201,12 @@ spec:
             maxSurge: 100%
             maxUnavailable: 25%
   hub: gcr.io/istio-testing
+
   meshConfig:
     enablePrometheusMerge: false
   profile: default
   tag: latest
   values:
-    base:
-      enableCRDTemplates: false
     clusterResources: true
     gateways:
       istio-egressgateway:
@@ -266,13 +265,16 @@ spec:
         requests:
           cpu: 10m
       enableHelmTest: false
+      enableTracing: true
       imagePullPolicy: ""
       imagePullSecrets: []
       istioNamespace: istio-system
       istiod:
         enableAnalysis: false
         enabled: true
       jwtPolicy: third-party-jwt
+      localityLbSetting:
+        enabled: true
       logAsJson: false
       logging:
         level: default:info
@@ -281,14 +283,19 @@ spec:
         useILB: false
       meshNetworks: {}
       mountMtlsCerts: false
+      mtls:
+        auto: true
       multiCluster:
         clusterName: ""
         enabled: false
       network: ""
       omitSidecarInjectorConfigMap: false
       oneNamespace: false
       operatorManageWebhooks: false
+      outboundTrafficPolicy:
+        mode: ALLOW_ANY
       pilotCertProvider: istiod
+      policyCheckFailOpen: false
       priorityClassName: ""
       proxy:
         autoInject: enabled

diff --git a/operator/cmd/mesh/testdata/profile-dump/output/config_path.yaml b/operator/cmd/mesh/testdata/profile-dump/output/config_path.yaml
@@ -76,8 +76,8 @@ ingressGateways:
     service:
       ports:
       - name: status-port
-        port: 15021
-        targetPort: 15021
+        port: 15020
+        targetPort: 15020
       - name: http2
         port: 80
         targetPort: 8080

diff --git a/operator/pkg/helm/helm.go b/operator/pkg/helm/helm.go
@@ -77,18 +77,13 @@ func NewHelmRenderer(operatorDataDir, helmSubdir, componentName, namespace strin
 
 // ReadProfileYAML reads the YAML values associated with the given profile. It uses an appropriate reader for the
 // profile format (compiled-in, file, HTTP, etc.).
-func ReadProfileYAML(profile string) (string, error) {
+func ReadProfileYAML(profile, chartsDir string) (string, error) {
 	var err error
 	var globalValues string
-	if profile == "" {
-		scope.Infof("ReadProfileYAML for profile name: [Empty]")
-	} else {
-		scope.Infof("ReadProfileYAML for profile name: %s", profile)
-	}
 
 	// Get global values from profile.
 	switch {
-	case IsBuiltinProfileName(profile):
+	case chartsDir == "":
 		if globalValues, err = LoadValuesVFS(profile); err != nil {
 			return "", err
 		}
@@ -98,7 +93,9 @@ func ReadProfileYAML(profile string) (string, error) {
 			return "", err
 		}
 	default:
-		return "", fmt.Errorf("unsupported Profile type: %s", profile)
+		if globalValues, err = LoadValues(profile, chartsDir); err != nil {
+			return "", fmt.Errorf("failed to read profile %v from %v: %v", profile, chartsDir, err)
+		}
 	}
 
 	return globalValues, nil
@@ -197,15 +194,12 @@ func renderTemplate(tmpl string, ts interface{}) (string, error) {
 }
 
 // DefaultFilenameForProfile returns the profile name of the default profile for the given profile.
-func DefaultFilenameForProfile(profile string) (string, error) {
+func DefaultFilenameForProfile(profile string) string {
 	switch {
 	case util.IsFilePath(profile):
-		return filepath.Join(filepath.Dir(profile), DefaultProfileFilename), nil
+		return filepath.Join(filepath.Dir(profile), DefaultProfileFilename)
 	default:
-		if _, ok := ProfileNames[profile]; ok || profile == "" {
-			return DefaultProfileString, nil
-		}
-		return "", fmt.Errorf("bad profile string %s", profile)
+		return DefaultProfileString
 	}
 }
 
@@ -308,23 +302,24 @@ func GetProfileYAML(installPackagePath, profileOrPath string) (string, error) {
 	if profileOrPath == "" {
 		profileOrPath = "default"
 	}
+	profiles, err := readProfiles(installPackagePath)
+	if err != nil {
+		return "", fmt.Errorf("failed to read profiles: %v", err)
+	}
 	// If charts are a file path and profile is a name like default, transform it to the file path.
-	if installPackagePath != "" && IsBuiltinProfileName(profileOrPath) {
+	if profiles[profileOrPath] {
 		profileOrPath = filepath.Join(installPackagePath, "profiles", profileOrPath+".yaml")
 	}
 	// This contains the IstioOperator CR.
-	baseCRYAML, err := ReadProfileYAML(profileOrPath)
+	baseCRYAML, err := ReadProfileYAML(profileOrPath, installPackagePath)
 	if err != nil {
 		return "", err
 	}
 
 	if !IsDefaultProfile(profileOrPath) {
 		// Profile definitions are relative to the default profileOrPath, so read that first.
-		dfn, err := DefaultFilenameForProfile(profileOrPath)
-		if err != nil {
-			return "", err
-		}
-		defaultYAML, err := ReadProfileYAML(dfn)
+		dfn := DefaultFilenameForProfile(profileOrPath)
+		defaultYAML, err := ReadProfileYAML(dfn, installPackagePath)
 		if err != nil {
 			return "", err
 		}
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		cd928894875e6ba84de7dd13bb7ceda9 operator/pkg/vfs/assets.gen.go
		03d81631567676f312f383cb193d539d operator/pkg/vfs/assets.gen.go