Repository: istio/istio (go)

Issue:
Setting workloadIP when configuring workloads
By default, when creating the configuration for a workload:
```
istioctl --context="cluster1" x workload entry configure \
    -f workloadgroup.yaml -o ./workload-files/ \
    --clusterID "cluster1" \
    --autoregister
```
We set up all the configurations needed for registering to the remote cluster. The issue is that as an IP address is determined by:
```
ISTIO_SVC_IP=$(hostname --all-ip-addresses | cut -d ' ' -f 1)
```

This works fine when the workload is in the same network but not when it is in a separate network.

We need a argument to specify the workload IP manually

Pull request description:
adding workloadIP to istioctl config workload
This PR resolves #32462 

[x] User Experience

[x] Does not have any changes that may affect Istio users.

---
This update adds the ability to specify the workload IP when configuring a new VM-based workload.

By default, when creating the configuration for a workload:
```
istioctl --context="cluster1" x workload entry configure \
    -f workloadgroup.yaml -o ./workload-files/ \
    --clusterID "cluster1" \
    --autoregister
```

We set up all the configurations needed for registering to the remote cluster. The issue is that as an IP address is determined by:
```
ISTIO_SVC_IP=$(hostname --all-ip-addresses | cut -d ' ' -f 1)
```
This works fine when the workload is in the same network but not when it is in a separate network. 

With the update you can define the workload IP manually:
```
istioctl --context="cluster1" x workload entry configure \
    -f workloadgroup.yaml -o ./workload-files/ \
    --clusterID "cluster1" \
    --workloadIP "108.47.9.33" \
    --autoregister
```

Patch:
diff --git a/.gitignore b/.gitignore
index 7874fc67b2..d157775783 100644
--- a/.gitignore
+++ b/.gitignore
@@ -19,6 +19,7 @@ tools/vagrant/.vagrant/
 *.pyc
 # Vm setup generated files
 cluster.env
+sidecar.env
 kubedns
 # pilot
 pilot/pkg/kube/config
diff --git a/go.mod b/go.mod
index 5b8b0dc9fe..a0e89c54a4 100644
--- a/go.mod
+++ b/go.mod
@@ -91,7 +91,7 @@ require (
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
 	helm.sh/helm/v3 v3.5.3
 	honnef.co/go/tools v0.0.1-2020.1.5 // indirect
-	istio.io/api v0.0.0-20210422202837-24c65c0415b6
+	istio.io/api v0.0.0-20210426022900-15c6bb524b69
 	istio.io/client-go v0.0.0-20210406025641-c740ff1f4def
 	istio.io/gogo-genproto v0.0.0-20210405163638-fcb891f20a5a
 	istio.io/pkg v0.0.0-20210405163638-bd457cbec517
diff --git a/go.sum b/go.sum
index 776904bcc1..d33b2d49c9 100644
--- a/go.sum
+++ b/go.sum
@@ -1582,8 +1582,8 @@ honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9
 honnef.co/go/tools v0.0.1-2020.1.5 h1:nI5egYTGJakVyOryqLs1cQO5dO0ksin5XXs2pspk75k=
 honnef.co/go/tools v0.0.1-2020.1.5/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 istio.io/api v0.0.0-20210406024326-a9c5294cac41/go.mod h1:nsSFw1LIMmGL7r/+6fJI6FxeG/UGlLxRK8bkojIvBVs=
-istio.io/api v0.0.0-20210422202837-24c65c0415b6 h1:rdbFYK2YlkG2oKZx1fQtJMMlmxQrh87nuyUB8u64QzE=
-istio.io/api v0.0.0-20210422202837-24c65c0415b6/go.mod h1:nsSFw1LIMmGL7r/+6fJI6FxeG/UGlLxRK8bkojIvBVs=
+istio.io/api v0.0.0-20210426022900-15c6bb524b69 h1:UDkiPAdMed3vsfRzd7vEBWyU56ufYzTaNbQ/sgzhwSY=
+istio.io/api v0.0.0-20210426022900-15c6bb524b69/go.mod h1:nsSFw1LIMmGL7r/+6fJI6FxeG/UGlLxRK8bkojIvBVs=
 istio.io/client-go v0.0.0-20210406025641-c740ff1f4def h1:gdehwtHGFaIDV/+JtmiuqD0SXiBwgWDQ1vGvkFAsEhU=
 istio.io/client-go v0.0.0-20210406025641-c740ff1f4def/go.mod h1:gJdlzn6YV0YaDKvATHuTBhfuFw9V7kkJ+RizMVEv/oE=
 istio.io/gogo-genproto v0.0.0-20210113155706-4daf5697332f/go.mod h1:6BwTZRNbWS570wHX/uR1Wqk5e0157TofTAUMzT7N4+s=
diff --git a/istio.deps b/istio.deps
index 4612fe01ef..bb5791b000 100644
--- a/istio.deps
+++ b/istio.deps
@@ -4,6 +4,6 @@
     "name": "PROXY_REPO_SHA",
     "repoName": "proxy",
     "file": "",
-    "lastStableSHA": "2a721fb1decadc29de26c6983249097713d79024"
+    "lastStableSHA": "efa1e0828c7601738462cd6437a1f83dfd15b2eb"
   }
 ]
diff --git a/istioctl/cmd/workload.go b/istioctl/cmd/workload.go
index 573f0c9cc9..809787ad85 100644
--- a/istioctl/cmd/workload.go
+++ b/istioctl/cmd/workload.go
@@ -61,6 +61,7 @@ var (
 	outputDir      string
 	clusterID      string
 	ingressIP      string
+	workloadIP     string
 	ingressSvc     string
 	autoRegister   bool
 	dnsCapture     bool
@@ -219,7 +220,7 @@ Configure requires either the WorkloadGroup artifact path or its location on the
 					return fmt.Errorf("workloadgroup %s not found in namespace %s: %v", name, namespace, err)
 				}
 			}
-			if err = createConfig(kubeClient, wg, clusterID, ingressIP, outputDir, cmd.OutOrStderr()); err != nil {
+			if err = createConfig(kubeClient, wg, clusterID, ingressIP, workloadIP, outputDir, cmd.OutOrStderr()); err != nil {
 				return err
 			}
 			fmt.Printf("configuration generation into directory %s was successful\n", outputDir)
@@ -237,6 +238,7 @@ Configure requires either the WorkloadGroup artifact path or its location on the
 	configureCmd.PersistentFlags().StringVar(&ingressIP, "ingressIP", "", "IP address of the ingress gateway")
 	configureCmd.PersistentFlags().BoolVar(&autoRegister, "autoregister", false, "Creates a WorkloadEntry upon connection to istiod (if enabled in pilot).")
 	configureCmd.PersistentFlags().BoolVar(&dnsCapture, "capture-dns", true, "Enables the capture of outgoing DNS packets on port 53, redirecting to istio-agent")
+	configureCmd.PersistentFlags().StringVar(&workloadIP, "workloadIP", "", "IP address of the workload used in the WorkloadEntry")
 	opts.AttachControlPlaneFlags(configureCmd)
 	return configureCmd
 }
@@ -266,7 +268,7 @@ func readWorkloadGroup(filename string, wg *clientv1alpha3.WorkloadGroup) error
 }
 
 // Creates all the relevant config for the given workload group and cluster
-func createConfig(kubeClient kube.ExtendedClient, wg *clientv1alpha3.WorkloadGroup, clusterID, ingressIP,
+func createConfig(kubeClient kube.ExtendedClient, wg *clientv1alpha3.WorkloadGroup, clusterID, ingressIP, workloadIP,
 	outputDir string, out io.Writer) error {
 	if err := os.MkdirAll(outputDir, filePerms); err != nil {
 		return err
@@ -281,6 +283,9 @@ func createConfig(kubeClient kube.ExtendedClient, wg *clientv1alpha3.WorkloadGro
 	if err := createClusterEnv(wg, proxyConfig, outputDir); err != nil {
 		return err
 	}
+	if err := createSidecarEnv(workloadIP, outputDir); err != nil {
+		return err
+	}
 	if err := createCertsTokens(kubeClient, wg, outputDir, out); err != nil {
 		return err
 	}
@@ -329,6 +334,25 @@ func createClusterEnv(wg *clientv1alpha3.WorkloadGroup, config *meshconfig.Proxy
 	return ioutil.WriteFile(filepath.Join(dir, "cluster.env"), []byte(mapToString(clusterEnv)), filePerms)
 }
 
+func createSidecarEnv(workloadIP string, dir string) error {
+	sidecarEnv := map[string]string{
+		"ISTIO_SVC_IP": workloadIP,
+	}
+
+	// If there is no sidecar specific configuration, then don't write the file and exit first.
+	allEmpty := true
+	for _, v := range sidecarEnv {
+		if len(v) > 0 {
+			allEmpty = false
+		}
+	}
+	if allEmpty {
+		return nil
+	}
+
+	return ioutil.WriteFile(filepath.Join(dir, "sidecar.env"), []byte(mapToString(sidecarEnv)), filePerms)
+}
+
 // Get and store the needed certificate and token. The certificate comes from the CA root cert, and
 // the token is generated by kubectl under the workload group's namespace and service account
 // TODO: Make the following accurate when using the Kubernetes certificate signer
diff --git a/operator/pkg/helm/helm.go b/operator/pkg/helm/helm.go
index 2273f4d7a9..dce3a62071 100644
--- a/operator/pkg/helm/helm.go
+++ b/operator/pkg/helm/helm.go
@@ -15,9 +15,7 @@
 package helm
 
 import (
-	"bytes"
 	"fmt"
-	"html/template"
 	"io/ioutil"
 	"path/filepath"
 	"sort"
@@ -184,21 +182,7 @@ spec:
 		Hub: hub,
 		Tag: tag,
 	}
-	return renderTemplate(hubTagYAMLTemplate, ts)
-}
-
-// helper method to render template
-func renderTemplate(tmpl string, ts interface{}) (string, error) {
-	t, err := template.New("").Parse(tmpl)
-	if err != nil {
-		return "", err
-	}
-	buf := new(bytes.Buffer)
-	err = t.Execute(buf, ts)
-	if err != nil {
-		return "", err
-	}
-	return buf.String(), nil
+	return util.RenderTemplate(hubTagYAMLTemplate, ts)
 }
 
 // DefaultFilenameForProfile returns the profile name of the default profile for the given profile.
diff --git a/pilot/pkg/serviceregistry/kube/controller/controller.go b/pilot/pkg/serviceregistry/kube/controller/controller.go
index 3aa8a076db..329e92734d 100644
--- a/pilot/pkg/serviceregistry/kube/controller/controller.go
+++ b/pilot/pkg/serviceregistry/kube/controller/controller.go
@@ -439,11 +439,6 @@ func (c *Controller) onServiceEvent(curr interface{}, event model.Event) error {
 			needsFullPush = c.updateServiceNodePortAddresses(svcConv)
 		}
 
-		if needsFullPush {
-			// networks are different, we need to update all eds endpoints
-			c.xdsUpdater.ConfigUpdate(&model.PushRequest{Full: true, Reason: []model.TriggerReason{model.NetworksTrigger}})
-		}
-
 		// instance conversion is only required when service is added/updated.
 		instances := kube.ExternalNameServiceInstances(svc, svcConv)
 		c.Lock()
@@ -452,6 +447,11 @@ func (c *Controller) onServiceEvent(curr interface{}, event model.Event) error {
 			c.externalNameSvcInstanceMap[svcConv.Hostname] = instances
 		}
 		c.Unlock()
+
+		if needsFullPush {
+			// networks are different, we need to update all eds endpoints
+			c.xdsUpdater.ConfigUpdate(&model.PushRequest{Full: true, Reason: []model.TriggerReason{model.NetworksTrigger}})
+		}
 	}
 
 	// We also need to update when the Service changes. For Kubernetes, a service change will result in Endpoint updates,
@@ -712,7 +712,6 @@ func (c *Controller) Services() ([]*model.Service, error) {
 	}
 	c.RUnlock()
 	sort.Slice(out, func(i, j int) bool { return out[i].Hostname < out[j].Hostname })
-
 	return out, nil
 }
 
diff --git a/pilot/pkg/serviceregistry/kube/controller/endpoint_builder.go b/pilot/pkg/serviceregistry/kube/controller/endpoint_builder.go
index 7f22f4f831..aa2b12e8fb 100644
--- a/pilot/pkg/serviceregistry/kube/controller/endpoint_builder.go
+++ b/pilot/pkg/serviceregistry/kube/controller/endpoint_builder.go
@@ -178,8 +178,3 @@ func (b *EndpointBuilder) endpointNetwork(endpointIP string) string {
 	// Fallback to legacy fromRegistry setting, all endpoints from this cluster are on that network.
 	return b.controller.defaultNetwork()
 }
-
-// TODO(lambdai): Make it true everywhere.
-func (b *EndpointBuilder) supportsH2Tunnel() bool {
-	return false
-}
diff --git a/pilot/pkg/serviceregistry/kube/controller/network.go b/pilot/pkg/serviceregistry/kube/controller/network.go
index 3587025480..4486efc0df 100644
--- a/pilot/pkg/serviceregistry/kube/controller/network.go
+++ b/pilot/pkg/serviceregistry/kube/controller/network.go
@@ -142,6 +142,7 @@ func (c *Controller) reloadNetworkGateways() {
 	for _, svc := range c.servicesMap {
 		if c.extractGatewaysInner(svc) {
 			gwsChanged = true
+			break
 		}
 	}
 	if gwsChanged {
diff --git a/pkg/config/validation/validation.go b/pkg/config/validation/validation.go
index e547b5eb5f..a6e7f4d624 100644
--- a/pkg/config/validation/validation.go
+++ b/pkg/config/validation/validation.go
@@ -2499,7 +2499,7 @@ func validateHTTPRewrite(rewrite *networking.HTTPRewrite) error {
 
 // ValidateWorkloadEntry validates a workload entry.
 var ValidateWorkloadEntry = registerValidateFunc("ValidateWorkloadEntry",
-	func(cfg config.Config) (warnings Warning, errs error) {
+	func(cfg config.Config) (Warning, error) {
 		we, ok := cfg.Spec.(*networking.WorkloadEntry)
 		if !ok {
 			return nil, fmt.Errorf("cannot cast to workload entry")
@@ -2507,13 +2507,40 @@ var ValidateWorkloadEntry = registerValidateFunc("ValidateWorkloadEntry",
 		return validateWorkloadEntry(we)
 	})
 
-func validateWorkloadEntry(we *networking.WorkloadEntry) (warnings Warning, errs error) {
+func validateWorkloadEntry(we *networking.WorkloadEntry) (Warning, error) {
+	errs := Validation{}
 	if we.Address == "" {
 		return nil, fmt.Errorf("address must be set")
 	}
-	// TODO: add better validation. The tricky thing is that we don't know if its meant to be
-	// DNS or STATIC type without association with a ServiceEntry
-	return nil, nil
+	// Since we don't know if its meant to be DNS or STATIC type without association with a ServiceEntry,
+	// check based on content and try validations.
+	addr := we.Address
+	// First check if it is a Unix endpoint - this will be specified for STATIC.
+	if strings.HasPrefix(we.Address, UnixAddressPrefix) {
+		errs = appendValidation(errs, ValidateUnixAddress(strings.TrimPrefix(addr, UnixAddressPrefix)))
+		if len(we.Ports) != 0 {
+			errs = appendValidation(errs, fmt.Errorf("unix endpoint %s must not include ports", we.Address))
+		}
+	} else {
+		// This could be IP (in STATIC resolution) or DNS host name (for DNS).
+		ipAddr := net.ParseIP(we.Address)
+		if ipAddr == nil {
+			if err := ValidateFQDN(we.Address); err != nil { // Otherwise could be an FQDN
+				errs = appendValidation(errs,
+					fmt.Errorf("endpoint address %q is not a valid FQDN or an IP address", we.Address))
+			}
+		}
+	}
+
+	errs = appendValidation(errs,
+		labels.Instance(we.Labels).Validate())
+	for name, port := range we.Ports {
+		// TODO: Validate port is part of Service Port - which is tricky to validate with out service entry.
+		errs = appendValidation(errs,
+			ValidatePortName(name),
+			ValidatePort(int(port)))
+	}
+	return errs.Unwrap()
 }
 
 // ValidateWorkloadGroup validates a workload group.
diff --git a/releasenotes/notes/32462.yaml b/releasenotes/notes/32462.yaml
new file mode 100644
index 0000000000..b43ba8bcc4
--- /dev/null
+++ b/releasenotes/notes/32462.yaml
@@ -0,0 +1,11 @@
+apiVersion: release-notes/v2
+kind: feature
+area: istioctl
+
+issue:
+- https://github.com/istio/istio/issues/32462
+
+releaseNotes:
+- |
+  **Added** `--workloadIP` flag to `istioctl x workload entry configure`, which sets the configuration for the workload IP that the sidecar proxy uses to auto register a workload Entry. 
+  Usually required when the VM workloads aren't in the same network as the primary cluster to which they register. 


Test patch:
diff --git a/istioctl/cmd/testdata/vmconfig/simple/sidecar.env.golden b/istioctl/cmd/testdata/vmconfig/simple/sidecar.env.golden
new file mode 100644
index 0000000000..c34d1479f6
--- /dev/null
+++ b/istioctl/cmd/testdata/vmconfig/simple/sidecar.env.golden
@@ -0,0 +1,1 @@
+ISTIO_SVC_IP='10.10.10.10'
diff --git a/istioctl/cmd/workload_test.go b/istioctl/cmd/workload_test.go
index a9ce6da901..dbf8401a7a 100644
--- a/istioctl/cmd/workload_test.go
+++ b/istioctl/cmd/workload_test.go
@@ -197,6 +197,7 @@ func TestWorkloadEntryConfigure(t *testing.T) {
 			cmd := []string{
 				"x", "workload", "entry", "configure",
 				"-f", path.Join("testdata/vmconfig", dir.Name(), "workloadgroup.yaml"),
+				"--workloadIP", "10.10.10.10",
 				"-o", testdir,
 			}
 			if _, err := runTestCmd(t, cmd); err != nil {
@@ -205,7 +206,7 @@ func TestWorkloadEntryConfigure(t *testing.T) {
 
 			checkFiles := map[string]bool{
 				// outputs to check
-				"mesh.yaml": true, "istio-token": true, "hosts": true, "root-cert.pem": true, "cluster.env": true,
+				"mesh.yaml": true, "istio-token": true, "hosts": true, "root-cert.pem": true, "cluster.env": true, "sidecar.env": true,
 				// inputs that we allow to exist, if other files seep in unexpectedly we fail the test
 				".gitignore": false, "meshconfig.yaml": false, "workloadgroup.yaml": false,
 			}
diff --git a/operator/pkg/object/objects_test.go b/operator/pkg/object/objects_test.go
index 88680608e3..6ff138edd3 100644
--- a/operator/pkg/object/objects_test.go
+++ b/operator/pkg/object/objects_test.go
@@ -258,7 +258,7 @@ func TestParseJSONToK8sObject(t *testing.T) {
 				if k8sObj.Group != tt.wantGroup {
 					t.Errorf("ParseJsonToK8sObject(%s): got group %s for k8s object %s, want %s", tt.desc, k8sObj.Group, k8sObjStr, tt.wantGroup)
 				}
-				if k8sObj.Group != tt.wantGroup {
+				if k8sObj.Kind != tt.wantKind {
 					t.Errorf("ParseJsonToK8sObject(%s): got kind %s for k8s object %s, want %s", tt.desc, k8sObj.Kind, k8sObjStr, tt.wantKind)
 				}
 				if k8sObj.Name != tt.wantName {
diff --git a/pilot/pkg/serviceregistry/kube/controller/controller_test.go b/pilot/pkg/serviceregistry/kube/controller/controller_test.go
index 619cf7f333..527555e185 100644
--- a/pilot/pkg/serviceregistry/kube/controller/controller_test.go
+++ b/pilot/pkg/serviceregistry/kube/controller/controller_test.go
@@ -1368,16 +1368,16 @@ func TestController_ExternalNameService(t *testing.T) {
 			}
 			for i, exp := range expectedSvcList {
 				if exp.Hostname != svcList[i].Hostname {
-					t.Fatalf("got hostname of %dst service, got:\n%#v\nwanted:\n%#v\n", i, svcList[i].Hostname, exp.Hostname)
+					t.Fatalf("got hostname of %dst service, got:\n%#v\nwanted:\n%#v\n", i+1, svcList[i].Hostname, exp.Hostname)
 				}
 				if !reflect.DeepEqual(exp.Ports, svcList[i].Ports) {
-					t.Fatalf("got ports of %dst service, got:\n%#v\nwanted:\n%#v\n", i, svcList[i].Ports, exp.Ports)
+					t.Fatalf("got ports of %dst service, got:\n%#v\nwanted:\n%#v\n", i+1, svcList[i].Ports, exp.Ports)
 				}
 				if svcList[i].MeshExternal != exp.MeshExternal {
-					t.Fatalf("i=%v, MeshExternal==%v, should be %v: externalName='%s'", i, exp.MeshExternal, svcList[i].MeshExternal, k8sSvcs[i].Spec.ExternalName)
+					t.Fatalf("i=%v, MeshExternal==%v, should be %v: externalName='%s'", i+1, exp.MeshExternal, svcList[i].MeshExternal, k8sSvcs[i].Spec.ExternalName)
 				}
 				if svcList[i].Resolution != exp.Resolution {
-					t.Fatalf("i=%v, Resolution=='%v', should be '%v'", i, svcList[i].Resolution, exp.Resolution)
+					t.Fatalf("i=%v, Resolution=='%v', should be '%v'", i+1, svcList[i].Resolution, exp.Resolution)
 				}
 				instances := controller.InstancesByPort(svcList[i], svcList[i].Ports[0].Port, labels.Collection{})
 				if len(instances) != 1 {
diff --git a/pkg/config/validation/validation_test.go b/pkg/config/validation/validation_test.go
index 4f8db5f946..6a2bfd2aee 100644
--- a/pkg/config/validation/validation_test.go
+++ b/pkg/config/validation/validation_test.go
@@ -2636,6 +2636,31 @@ func TestValidateWorkloadEntry(t *testing.T) {
 			in:    &networking.WorkloadEntry{},
 			valid: false,
 		},
+		{
+			name:  "valid unix endpoint",
+			in:    &networking.WorkloadEntry{Address: "unix:///lon/google/com"},
+			valid: true,
+		},
+		{
+			name:  "invalid unix endpoint",
+			in:    &networking.WorkloadEntry{Address: "unix:///lon/google/com", Ports: map[string]uint32{"7777": 7777}},
+			valid: false,
+		},
+		{
+			name:  "valid FQDN",
+			in:    &networking.WorkloadEntry{Address: "validdns.com", Ports: map[string]uint32{"7777": 7777}},
+			valid: true,
+		},
+		{
+			name:  "invalid FQDN",
+			in:    &networking.WorkloadEntry{Address: "invaliddns.com:9443", Ports: map[string]uint32{"7777": 7777}},
+			valid: false,
+		},
+		{
+			name:  "valid IP",
+			in:    &networking.WorkloadEntry{Address: "172.16.1.1", Ports: map[string]uint32{"7777": 7777}},
+			valid: true,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {