Repository: linkerd/linkerd2
Pull request #8622 (2022-06-06 09:29:33)

Refactor the function PublicIPToString to handle both IPv4 and IPv6 in similar behavior
Signed-off-by: zhlsunshine <huailong.zhang@intel.com>

1. Fixed issue for `PublicIPToString` if IPv6 address has **:** as its prefix, such as **::1** and **::c0a8:0:0:0:1**
2. Added the test case for `PublicIPToString`

zhlsunshine: Hi @adleong Any comment for this PR?
adleong: Hi @zhlsunshine.  I think many of the questions in my comment above still stand.  Can you explain more about the bug that this fixes and explain what the purpose of each step is here?
zhlsunshine: > Hi @zhlsunshine. I think many of the questions in my comment above still stand. Can you explain more about the bug that this fixes and explain what the purpose of each step is here?

Hi @adleong, thank you for your reply, 
Firstly, I think it‘s okay for INT-format IP address converts into net.IP-format and reverse the process now in `addr.go`, because there some functions for this, such as: `IntToIPv4`, `IntToIPv6` and `IPToInt`
Secondly, for IPv6 struct as below:
```
type IPv6 struct {
	......
	First uint64 `protobuf:"fixed64,1,opt,name=first,proto3" json:"first,omitempty"` // hextets 1-4
	Last  uint64 `protobuf:"fixed64,2,opt,name=last,proto3" json:"last,omitempty"`   // hextets 5-8
}
```
I think IPv6 can be defined as following
```
type IPv6 struct {
	......
	IPv6 big.Int`protobuf:"[]byte,1,opt,name=ipv6,proto3" json:"ipv6,omitempty"` // hextets 1-4
}
```

then, specially for `IntToIPv6`, it can be okay for IPv6 INT-format to net.IP

so does it make sense?
adleong: I don't think that answers my questions.  I don't think there's a compelling reason to change the API.
zhlsunshine: > I don't think that answers my questions. I don't think there's a compelling reason to change the API.

Hi @adleong, for your question: if using current IPv6 struct, it's enough to use `binary.BigEndian.PutUint64(b[:8], ip.GetFirst())` and `binary.BigEndian.PutUint64(b[8:], ip.GetLast())` for IPv6 conversion from its struct to net.IP and unnecessary to call `IntToIPv6`. 
However, the purpose of  `IntToIPv6` is to use for handling the conversion if changing the IPv6 struct to `big.Int`.

Sure, I will respect your view if you do not want to change the current IPv6 struct. As I mentioned above, if so, I will  change the function `decodeIPv6ToNetIP` as below and remove the function `IntToIPv6` :

```
func decodeIPv6ToNetIP(ip *l5dNetPb.IPv6) net.IP {
        b := make([]byte, 16)
        binary.BigEndian.PutUint64(b[:8], ip.GetFirst())
        binary.BigEndian.PutUint64(b[8:], ip.GetLast())
        return net.IP(b)
}
```

adleong: Hi @zhlsunshine.  It seems like this

```
	if netIP == nil {
		return ""
	}
```

is the only change necessary to handle zero-length ip addresses.  The rest of this refactor isn't necessary and I don't think it improves readability of the code.
zhlsunshine: > Hi @zhlsunshine. It seems like this
> 
> ```
> 	if netIP == nil {
> 		return ""
> 	}
> ```
> 
> is the only change necessary to handle zero-length ip addresses. The rest of this refactor isn't necessary and I don't think it improves readability of the code.

Hi @adleong, yes, it might be the only necessary change from the functionality view. However, the refactor which creates a new function `decodeIPv6ToNetIP` can also make the code hierarchy more clear in `PublicIPToString` which includes handling both for IPv6 and IPv4. Does it make sense?
adleong: I don't think there's much to be gained from moving 3 lines of code behind a new function in this case.  Having this code inline makes the behavior more clear, in my opinion.  Also, when possible, I prefer to separate functional changes and stylistic refactors into separate pull requests.   
zhlsunshine: Hi @adleong,  as you required,  I re-submit again to remove `decodeIPv6ToNetIP`
adleong: Hi @zhlsunshine, it looks like something has gone wrong with your merge.  Make sure you've pulled and merged the latest main.
zhlsunshine: Hi @mateiidavid & @adleong,  please let me know what else should I do to merge this PR, thanks! ^_^
zhlsunshine: Hi @adleong, I rebased my local code and re-submitted again, thanks!

Patch:
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index d4b10a24f..ef12317e7 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -44,7 +44,7 @@ jobs:
         docker-ghcr-pat: ${{ secrets.DOCKER_GHCR_PAT }}
         component: ${{ matrix.component }}
     - name: Install Cosign
-      uses: sigstore/cosign-installer@main
+      uses: sigstore/cosign-installer@v2
     - name: Sign the images with Cosign
       env:
         COSIGN_EXPERIMENTAL: 1
@@ -98,7 +98,7 @@ jobs:
         arch: ${{ matrix.arch }}
         tag: ${{ env.TAG }}
     - name: Install Cosign
-      uses: sigstore/cosign-installer@main
+      uses: sigstore/cosign-installer@v2
     - name: Sign the image with Cosign
       env:
         COSIGN_EXPERIMENTAL: 1
diff --git a/.proxy-version b/.proxy-version
index 87c7e3144..04f7b11d7 100644
--- a/.proxy-version
+++ b/.proxy-version
@@ -1,1 +1,1 @@
-v2.180.0
+v2.181.0
diff --git a/Cargo.lock b/Cargo.lock
index b74e83e86..5b38ddfb2 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -24,9 +24,9 @@ dependencies = [
 
 [[package]]
 name = "anyhow"
-version = "1.0.57"
+version = "1.0.58"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "08f9b8508dccb7687a1d6c4ce66b2b0ecef467c94667de27d8d7fe1f8d2a9cdc"
+checksum = "bb07d2053ccdbe10e2af2995a2f116c1330396493dc1269f6a91d0ae82e19704"
 
 [[package]]
 name = "async-stream"
@@ -300,9 +300,9 @@ dependencies = [
 
 [[package]]
 name = "dyn-clone"
-version = "1.0.5"
+version = "1.0.6"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "21e50f3adc76d6a43f5ed73b698a87d0760ca74617f60f7c3b879003536fdd28"
+checksum = "140206b78fb2bc3edbcfc9b5ccbd0b30699cfe8d348b8b31b330e47df5291a5a"
 
 [[package]]
 name = "either"
@@ -487,9 +487,9 @@ dependencies = [
 
 [[package]]
 name = "hashbrown"
-version = "0.11.2"
+version = "0.12.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ab5ef0d4909ef3724cc8cce6ccc8572c5c817592e9285f5464f8e86f8bd3726e"
+checksum = "db0d4cf898abf0081f964436dc980e96670a0f36863e4b83aaacdb65c9d7ccc3"
 
 [[package]]
 name = "heck"
@@ -647,9 +647,9 @@ dependencies = [
 
 [[package]]
 name = "indexmap"
-version = "1.8.2"
+version = "1.9.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e6012d540c5baa3589337a98ce73408de9b5a25ec9fc2c6fd6be8f0d39e0ca5a"
+checksum = "6c6392766afd7964e2531940894cffe4bd8d7d17dbc3c1c4857040fd4b33bdb3"
 dependencies = [
  "autocfg",
  "hashbrown",
@@ -1330,9 +1330,9 @@ dependencies = [
 
 [[package]]
 name = "proc-macro2"
-version = "1.0.39"
+version = "1.0.40"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c54b25569025b7fc9651de43004ae593a75ad88543b17178aa5e1b9c4f15f56f"
+checksum = "dd96a1e8ed2596c337f8eae5f24924ec83f5ad5ab21ea8e455d3566c69fbcaf7"
 dependencies = [
  "unicode-ident",
 ]
@@ -1372,9 +1372,9 @@ dependencies = [
 
 [[package]]
 name = "quote"
-version = "1.0.18"
+version = "1.0.19"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a1feb54ed693b93a84e14094943b84b7c4eae204c512b7ccb95ab0c66d278ad1"
+checksum = "f53dc8cf16a769a6f677e09e7ff2cd4be1ea0f48754aac39520536962011de0d"
 dependencies = [
  "proc-macro2",
 ]
@@ -1729,9 +1729,9 @@ checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623"
 
 [[package]]
 name = "syn"
-version = "1.0.96"
+version = "1.0.98"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0748dd251e24453cb8717f0354206b91557e4ec8703673a4b30208f2abaf1ebf"
+checksum = "c50aef8a904de4c23c788f104b7dddc7d6f79c647c7c8ce4cc8f73eb0ca773dd"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1969,9 +1969,9 @@ dependencies = [
 
 [[package]]
 name = "tower"
-version = "0.4.12"
+version = "0.4.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9a89fd63ad6adf737582df5db40d286574513c69a11dac5214dc3b5603d6713e"
+checksum = "b8fa9be0de6cf49e536ce1851f987bd21a43b771b09473c3549a6c853db37c1c"
 dependencies = [
  "futures-core",
  "futures-util",
diff --git a/pkg/addr/addr.go b/pkg/addr/addr.go
index 3aa536395..5a983bcc2 100644
--- a/pkg/addr/addr.go
+++ b/pkg/addr/addr.go
@@ -29,16 +29,19 @@ func PublicAddressToString(addr *l5dNetPb.TcpAddress) string {
 
 // PublicIPToString formats a Viz API IPAddress as a string.
 func PublicIPToString(ip *l5dNetPb.IPAddress) string {
-	var b []byte
+	var netIP net.IP
 	if ip.GetIpv6() != nil {
-		b = make([]byte, 16)
+		b := make([]byte, net.IPv6len)
 		binary.BigEndian.PutUint64(b[:8], ip.GetIpv6().GetFirst())
 		binary.BigEndian.PutUint64(b[8:], ip.GetIpv6().GetLast())
+		netIP = net.IP(b)
 	} else if ip.GetIpv4() != 0 {
-		b = make([]byte, 4)
-		binary.BigEndian.PutUint32(b, ip.GetIpv4())
+		netIP = decodeIPv4ToNetIP(ip.GetIpv4())
 	}
-	return net.IP(b).String()
+	if netIP == nil {
+		return ""
+	}
+	return netIP.String()
 }
 
 // ProxyAddressToString formats a Proxy API TCPAddress as a string.
@@ -94,7 +97,7 @@ func ParsePublicIPV4(ip string) (*l5dNetPb.IPAddress, error) {
 }
 
 // NetToPublic converts a Proxy API TCPAddress to a Viz API
-// TCPAddress
+// TCPAddress.
 func NetToPublic(net *pb.TcpAddress) *l5dNetPb.TcpAddress {
 	var ip *l5dNetPb.IPAddress
 
@@ -122,6 +125,7 @@ func NetToPublic(net *pb.TcpAddress) *l5dNetPb.TcpAddress {
 	}
 }
 
+// decodeIPv4ToNetIP converts IPv4 uint32 to an IPv4 net IP.
 func decodeIPv4ToNetIP(ip uint32) net.IP {
 	oBigInt := big.NewInt(0)
 	oBigInt = oBigInt.SetUint64(uint64(ip))
@@ -129,16 +133,16 @@ func decodeIPv4ToNetIP(ip uint32) net.IP {
 }
 
 // IPToInt converts net.IP to bigInt
-// It can support both IPv4 and IPv6
+// It can support both IPv4 and IPv6.
 func IPToInt(ip net.IP) *big.Int {
 	oBigInt := big.NewInt(0)
 	oBigInt.SetBytes(ip)
 	return oBigInt
 }
 
-// IntToIPv4 converts bigInt to an IPv4 net.IP
+// IntToIPv4 converts IPv4 bigInt into an IPv4 net IP.
 func IntToIPv4(intip *big.Int) net.IP {
-	ipByte := make([]byte, 4)
+	ipByte := make([]byte, net.IPv4len)
 	uint32IP := intip.Uint64()
 	binary.BigEndian.PutUint32(ipByte, uint32(uint32IP))
 	return net.IP(ipByte)


Test patch:
diff --git a/pkg/addr/addr_test.go b/pkg/addr/addr_test.go
index 1675e344c..6856e3088 100644
--- a/pkg/addr/addr_test.go
+++ b/pkg/addr/addr_test.go
@@ -46,7 +46,7 @@ func TestPublicAddressToString(t *testing.T) {
 		{
 			name:     "nil",
 			addr:     nil,
-			expected: "<nil>:0",
+			expected: ":0",
 		},
 	}
 
@@ -61,6 +61,63 @@ func TestPublicAddressToString(t *testing.T) {
 	}
 }
 
+func TestPublicIPToString(t *testing.T) {
+	cases := []struct {
+		name     string
+		addr     *l5dNetPb.IPAddress
+		expected string
+	}{
+		{
+			name: "ipv4",
+			addr: &l5dNetPb.IPAddress{
+				Ip: &l5dNetPb.IPAddress_Ipv4{
+					Ipv4: 3232235521,
+				},
+			},
+			expected: "192.168.0.1",
+		},
+		{
+			name: "narmal ipv6",
+			addr: &l5dNetPb.IPAddress{
+				Ip: &l5dNetPb.IPAddress_Ipv6{
+					Ipv6: &l5dNetPb.IPv6{
+						First: 2306139570357600256,
+						Last:  151930230829876,
+					},
+				},
+			},
+			expected: "2001:db8:85a3::8a2e:370:7334",
+		},
+		{
+			name: "ipv6 with zero as prefix",
+			addr: &l5dNetPb.IPAddress{
+				Ip: &l5dNetPb.IPAddress_Ipv6{
+					Ipv6: &l5dNetPb.IPv6{
+						First: 49320,
+						Last:  1,
+					},
+				},
+			},
+			expected: "::c0a8:0:0:0:1",
+		},
+		{
+			name:     "nil",
+			addr:     nil,
+			expected: "",
+		},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			got := PublicIPToString(c.addr)
+			if c.expected != got {
+				t.Errorf("expected: %v, got: %v", c.expected, got)
+			}
+		})
+	}
+}
+
 func TestProxyAddressesToString(t *testing.T) {
 	cases := []struct {
 		name     string
diff --git a/policy-test/tests/api.rs b/policy-test/tests/api.rs
index c57689760..e1e3e4573 100644
--- a/policy-test/tests/api.rs
+++ b/policy-test/tests/api.rs
@@ -1,3 +1,5 @@
+use std::time::Duration;
+
 use futures::prelude::*;
 use kube::ResourceExt;
 use linkerd_policy_controller_core::{Ipv4Net, Ipv6Net};
@@ -19,14 +21,7 @@ async fn server_with_server_authorization() {
         let pod = create_ready_pod(&client, mk_pause(&ns, "pause")).await;
         tracing::trace!(?pod);
 
-        // Port-forward to the control plane and start watching the pod's admin
-        // server's policy and ensure that the first update uses the default
-        // policy.
-        let mut policy_api = grpc::PolicyClient::port_forwarded(&client).await;
-        let mut rx = policy_api
-            .watch_port(&ns, &pod.name(), 4191)
-            .await
-            .expect("failed to establish watch");
+        let mut rx = retry_watch_server(&client, &ns, &pod.name()).await;
         let config = rx
             .next()
             .await
@@ -160,14 +155,7 @@ async fn server_with_authorization_policy() {
         let pod = create_ready_pod(&client, mk_pause(&ns, "pause")).await;
         tracing::trace!(?pod);
 
-        // Port-forward to the control plane and start watching the pod's admin
-        // server's policy and ensure that the first update uses the default
-        // policy.
-        let mut policy_api = grpc::PolicyClient::port_forwarded(&client).await;
-        let mut rx = policy_api
-            .watch_port(&ns, &pod.name(), 4191)
-            .await
-            .expect("failed to establish watch");
+        let mut rx = retry_watch_server(&client, &ns, &pod.name()).await;
         let config = rx
             .next()
             .await
@@ -331,3 +319,23 @@ fn mk_admin_server(ns: &str, name: &str) -> k8s::policy::Server {
         },
     }
 }
+
+async fn retry_watch_server(
+    client: &kube::Client,
+    ns: &str,
+    pod_name: &str,
+) -> tonic::Streaming<grpc::inbound::Server> {
+    // Port-forward to the control plane and start watching the pod's admin
+    // server's policy and ensure that the first update uses the default
+    // policy.
+    let mut policy_api = grpc::PolicyClient::port_forwarded(client).await;
+    loop {
+        match policy_api.watch_port(ns, pod_name, 4191).await {
+            Ok(rx) => return rx,
+            Err(error) => {
+                tracing::error!(?error, ns, pod_name, "failed to watch policy for port 4191");
+                time::sleep(Duration::from_secs(1)).await;
+            }
+        }
+    }
+}