Added test workflow for PR and upgraded dependencies #76

sonamtenzin6 · 2025-01-30T07:25:04Z

Description

Enabled build and unit tests in the repo
Fixed existing tests
Sorting logic edge case fix in SortIPAddrsByNetworkSize
Upgraded libraries which could cause this issue - https://hashicorp.atlassian.net/browse/SECVULN-17075

sonamtenzin6 · 2025-02-04T05:52:21Z

ipaddrs.go

@@ -67,9 +67,12 @@ func (s SortIPAddrsByNetworkSize) Less(i, j int) bool {
 	// that have a port (e.g. a host with a /32 and port number is more
 	// specific and should sort first over a host with a /32 but no port
 	// set).
-	if s.IPAddrs[i].IPPort() == 0 || s.IPAddrs[j].IPPort() == 0 {


This condition is currently incorrectly sorting for scenarios like these -

If we get an order like this, it will return false maintaining that 1>2

128.95.120.2:8600

128.95.120.2

And if we get input like this, it will still return 1>2

128.95.120.2

128.95.120.2:8600

It is essentially not sorting for these scenarios. The test TestSockAddr_IPAddrs_IPAddrsByNetworkSize was failing because of the issue

Just to note here, proposal to fix this has been in #50 since two years ago. It also includes an explanation why this became an issue with Go 1.19 onwards.

This PR will anyways need some changes to go through, I will merge #50 and take a rebase from master to incorporate the fix
cc: @schmichael

sonamtenzin6 · 2025-02-04T05:55:17Z

template/template_test.go

-			input:  `{{GetDefaultInterfaces | include "type" "IPv4" | attr "name" }}`,
-			output: `en0`,
-		},
+		// {


Commented out these tests because they are dependent on underlying interfaces of the machine, which may change

This sounds reasonable to me, but maybe just remove the test?

sonamtenzin6 · 2025-02-04T05:57:30Z

template/template_test.go

@@ -16,7 +177,7 @@ func TestSockAddr_Parse(t *testing.T) {
 	}{
 		{
 			name:   `basic include "name"`,
-			input:  `{{GetAllInterfaces | include "name" "lo0" | printf "%v"}}`,
+			input:  `{{. | include "name" "lo0" | printf "%v"}}`,


Changed this from GetAllInterfaces because this caused the test to depend on underlying machine interfaces. We are not getting mock input from getInputList method

sonamtenzin6 · 2025-02-04T05:58:26Z

template/template_test.go

@@ -265,7 +426,7 @@ func TestSockAddr_Parse(t *testing.T) {
 		}
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			out, err := socktmpl.Parse(test.input)
+			out, err := socktmpl.ParseIfAddrs(test.input, getInputList())


Changed to include mocked interfaces. Parse is essentially calling ParseIfAddrs

Instead of a function that gets called in each test, can we not just create this list once at the top of this test function and then use the value in all the test? Does ParseIfAddrs mutate the input? If so, could we instead create a copy of the list in each test with copy? I think I'd prefer not to have this helper function defined at all.

Sure, removed the function and replaced with a list

johanbrandhorst

Thanks for this, the security vulnerability fix is just the mod update, and I'm guessing you decided to fix the tests because they were failing on your machine? I think that makes sense. I added some comments, let me know what you think!

johanbrandhorst · 2025-02-07T16:13:52Z

.github/workflows/build_test.yml

+            - name: Setup Go
+              uses: actions/setup-go@3041bf56c941b39c61721a86cd11f3bb1338122a
+              with:
+                go-version: '1.19'


This is an ancient release. I see that the go.mod says 1.19, but that just means it's the oldest version supported. We should probably use a matrix here, including 1.19, 1.20, 1.21, 1.22 and 1.23. See https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow#using-a-matrix-strategy if you're unsure how to create a matrix strategy.

Also CC @schmichael, what is this repositories Go compatibility policy? I'd probably suggest we update the go directive to 1.22 and use the same policy as the Go team.

SGTM. This repo doesn't need special treatment. It's just been stable/neglected.

Added a matrix for testing on different versions

johanbrandhorst · 2025-02-07T16:18:07Z

template/template_test.go

-			input:  `{{GetDefaultInterfaces | include "type" "IPv4" | attr "name" }}`,
-			output: `en0`,
-		},
+		// {


This sounds reasonable to me, but maybe just remove the test?

johanbrandhorst · 2025-02-07T16:18:41Z

template/template_test.go

+		// 		{
+		// 			// NOTE(sean@): This is the HashiCorp default in 2016.
+		// 			// Indented for effect.  Using "true" as the output
+		// 			// instead of printing the correct $rfc*Addrs values.
+		// 			name: "HashiCorpDefault2016",
+		// 			input: `
+		// {{- with $addr := . | include "type" "IP" | include "rfc" "1918|6598" | sort "address" | attr "address" -}}

-  {{- if ($addr | len) gt 0 -}}
-    {{- print "true" -}}{{/* print $addr*/ -}}
-  {{- end -}}
-{{- end -}}`,
-			output: `true`,
-		},
+		//   {{- if ($addr | len) gt 0 -}}
+		//     {{- print "true" -}}{{/* print $addr*/ -}}
+		//   {{- end -}}
+		// {{- end -}}`,
+		// 			output: `true`,
+		// 		},


Can this test also not work?

As above I'm +1 for removing ancient tests as opposed to commenting them out.

The test is currently failing, so removed it

johanbrandhorst · 2025-02-07T16:18:55Z

template/template_test.go

+		// {
+		// 	// Assume the private IPs available on the host are: 10.1.2.3
+		// 	// fe80::1025:f732:1001:203
+		// 	name:   "GetPrivateIPs",
+		// 	input:  `{{GetPrivateIPs}}`,
+		// 	output: `10.1.2.3 fe80::1025:f732:1001:203`,
+		// },
+		// {
+		// 	// Assume the public IPs available on the host are: 1.2.3.4 6.7.8.9
+		// 	name:   "GetPublicIPs",
+		// 	input:  `{{GetPublicIPs}}`,
+		// 	output: `1.2.3.4 6.7.8.9`,
+		// },
+		// {
+		// 	// Assume the private IPs on this host are just the IPv4 addresses:
+		// 	// 10.1.2.3 and 172.16.4.6
+		// 	name:   "GetInterfaceIPs",
+		// 	input:  `{{GetInterfaceIPs "en0"}}`,
+		// 	output: `10.1.2.3 and 172.16.4.6`,
+		// },


Remove these

johanbrandhorst · 2025-02-07T16:20:56Z

template/template_test.go

@@ -265,7 +426,7 @@ func TestSockAddr_Parse(t *testing.T) {
 		}
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			out, err := socktmpl.Parse(test.input)
+			out, err := socktmpl.ParseIfAddrs(test.input, getInputList())


Instead of a function that gets called in each test, can we not just create this list once at the top of this test function and then use the value in all the test? Does ParseIfAddrs mutate the input? If so, could we instead create a copy of the list in each test with copy? I think I'd prefer not to have this helper function defined at all.

schmichael

+1 to @johanbrandhorst's comments about bumping Go and dropping commented out tests.

Logic fix looks good.

Can we update the title and description to drop the security vulnerabilities reference or at least clarify it's a vulnerability in a dependency that is unused by go-sockaddr. go-sockaddr does not have a vulnerability, and I would hate people (or security scanners) to flag the wrong package!

Please correct me if I'm missing something.

schmichael · 2025-02-07T17:16:02Z

template/template_test.go

+		// 		{
+		// 			// NOTE(sean@): This is the HashiCorp default in 2016.
+		// 			// Indented for effect.  Using "true" as the output
+		// 			// instead of printing the correct $rfc*Addrs values.
+		// 			name: "HashiCorpDefault2016",
+		// 			input: `
+		// {{- with $addr := . | include "type" "IP" | include "rfc" "1918|6598" | sort "address" | attr "address" -}}

-  {{- if ($addr | len) gt 0 -}}
-    {{- print "true" -}}{{/* print $addr*/ -}}
-  {{- end -}}
-{{- end -}}`,
-			output: `true`,
-		},
+		//   {{- if ($addr | len) gt 0 -}}
+		//     {{- print "true" -}}{{/* print $addr*/ -}}
+		//   {{- end -}}
+		// {{- end -}}`,
+		// 			output: `true`,
+		// 		},


As above I'm +1 for removing ancient tests as opposed to commenting them out.

schmichael · 2025-02-07T17:22:53Z

This will supersede and close #50. Many apologies for missing the opportunity to merge that one!

johanbrandhorst · 2025-02-13T17:21:51Z

ifaddrs_test.go

@@ -632,7 +633,7 @@ func TestGetIfAddrs(t *testing.T) {
 		t.Fatalf("No loopback interfaces found, loInt nil")
 	}

-	if val := sockaddr.IfAddrAttr(*loInt, "flags"); !(val == "up|loopback|multicast" || val == "up|loopback") {
+	if val := sockaddr.IfAddrAttr(*loInt, "flags"); !strings.Contains(val, "up|loopback") {


Why was this changed? This isn't the same behavior anymore.

johanbrandhorst · 2025-02-13T17:24:37Z

.github/workflows/build_test.yml

+              - '1.19'
+              - '1.20'
+              - '1.21'
+              - '1.22'
+              - '1.23'


To make this a little more future proof, we can use the stable and oldstable aliases to use the current and the previous release for this.

Suggested change

- '1.19'

- '1.20'

- '1.21'

- '1.22'

- '1.23'

- 'oldstable'

- 'stable'

johanbrandhorst · 2025-02-13T17:25:20Z

go.mod

@@ -27,6 +27,6 @@ require (
 	github.com/posener/complete v1.1.1 // indirect
 	github.com/shopspring/decimal v1.2.0 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
-	golang.org/x/crypto v0.17.0 // indirect
-	golang.org/x/sys v0.22.0 // indirect
+	golang.org/x/crypto v0.32.0 // indirect


Could you also change the go stanza at the top of the file to 1.23?

sonamtenzin6 force-pushed the sonamt/test-run branch 4 times, most recently from 61bb7c6 to 0b4bdf0 Compare February 4, 2025 05:29

sonamtenzin6 marked this pull request as ready for review February 4, 2025 05:35

sonamtenzin6 requested review from a team as code owners February 4, 2025 05:35

sonamtenzin6 commented Feb 4, 2025

View reviewed changes

sonamtenzin6 changed the title ~~Added build and test workflow for PRs~~ Added test workflow for PR and fixed security vulnerabilities Feb 4, 2025

johanbrandhorst requested changes Feb 7, 2025

View reviewed changes

schmichael previously approved these changes Feb 7, 2025

View reviewed changes

sonamtenzin6 added 2 commits February 13, 2025 11:27

Test workflow added and test fixes

666b14d

Fixed security vulnerablilities

66c72a0

sonamtenzin6 dismissed schmichael’s stale review via 66c72a0 February 13, 2025 06:14

sonamtenzin6 force-pushed the sonamt/test-run branch from 1791c24 to 66c72a0 Compare February 13, 2025 06:14

resolved comments

0d807b7

sonamtenzin6 changed the title ~~Added test workflow for PR and fixed security vulnerabilities~~ Added test workflow for PR and upgraded dependencies Feb 13, 2025

sonamtenzin6 requested review from johanbrandhorst and schmichael February 13, 2025 06:22

schmichael approved these changes Feb 13, 2025

View reviewed changes

johanbrandhorst requested changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added test workflow for PR and upgraded dependencies #76

Added test workflow for PR and upgraded dependencies #76

sonamtenzin6 commented Jan 30, 2025 •

edited

Loading

sonamtenzin6 Feb 4, 2025 •

edited

Loading

thevilledev Feb 10, 2025

sonamtenzin6 Feb 11, 2025 •

edited

Loading

sonamtenzin6 Feb 4, 2025

johanbrandhorst Feb 7, 2025

sonamtenzin6 Feb 4, 2025

sonamtenzin6 Feb 4, 2025

johanbrandhorst Feb 7, 2025

sonamtenzin6 Feb 13, 2025

johanbrandhorst left a comment

johanbrandhorst Feb 7, 2025

schmichael Feb 7, 2025

sonamtenzin6 Feb 13, 2025

johanbrandhorst Feb 7, 2025

johanbrandhorst Feb 7, 2025

schmichael Feb 7, 2025

sonamtenzin6 Feb 13, 2025

johanbrandhorst Feb 7, 2025

johanbrandhorst Feb 7, 2025

schmichael left a comment

schmichael Feb 7, 2025

schmichael commented Feb 7, 2025 •

edited

Loading

johanbrandhorst Feb 13, 2025

johanbrandhorst Feb 13, 2025

johanbrandhorst Feb 13, 2025

-              - '1.19'
-              - '1.20'
-              - '1.21'
-              - '1.22'
-              - '1.23'
+              - 'oldstable'
+              - 'stable'

Added test workflow for PR and upgraded dependencies #76

Are you sure you want to change the base?

Added test workflow for PR and upgraded dependencies #76

Conversation

sonamtenzin6 commented Jan 30, 2025 • edited Loading

Description

sonamtenzin6 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonamtenzin6 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael commented Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonamtenzin6 commented Jan 30, 2025 •

edited

Loading

sonamtenzin6 Feb 4, 2025 •

edited

Loading

sonamtenzin6 Feb 11, 2025 •

edited

Loading

schmichael commented Feb 7, 2025 •

edited

Loading