From 5f807a894be10e83aa0def3c0e755362ee45c70c Mon Sep 17 00:00:00 2001
From: David Son <davbson@amazon.com>
Date: Fri, 10 Jan 2025 22:55:09 +0000
Subject: [PATCH] Return correct CNI version string

Prior to this commit, we were using the output of a result to return a
CNI version. This would always be the latest supported version returned
by the CNI library. This meant that, regardless of the CNI version used
in a user's conflist, it would always report back that the latest
version was being used, which is both incorrect behavior and breaks
workflows where the latest version was not recognized as a valid
version.

This change fixes this behavior and returns the same CNI version
specified in the conflist, which mirrors the behavior of other CNI
plugins.

Signed-off-by: David Son <davbson@amazon.com>
---
 cmd/tc-redirect-tap/main.go      | 27 ++++++++++++++++++---------
 cmd/tc-redirect-tap/main_test.go |  8 +++++---
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/cmd/tc-redirect-tap/main.go b/cmd/tc-redirect-tap/main.go
index d65c1c9..a496fc2 100644
--- a/cmd/tc-redirect-tap/main.go
+++ b/cmd/tc-redirect-tap/main.go
@@ -64,7 +64,7 @@ func add(args *skel.CmdArgs) error {
 		return err
 	}
 
-	return types.PrintResult(p.currentResult, p.currentResult.CNIVersion)
+	return types.PrintResult(p.currentResult, p.confCNIVersion)
 }
 
 func del(args *skel.CmdArgs) error {
@@ -116,7 +116,7 @@ func newPlugin(args *skel.CmdArgs) (*plugin, error) {
 		}
 	}
 
-	currentResult, err := getCurrentResult(args)
+	currentResult, confCNIVersion, err := getCurrentResult(args)
 	if err != nil {
 		switch err.(type) {
 		case *NoPreviousResultError:
@@ -138,7 +138,8 @@ func newPlugin(args *skel.CmdArgs) (*plugin, error) {
 
 		netNS: netNS,
 
-		currentResult: currentResult,
+		currentResult:  currentResult,
+		confCNIVersion: confCNIVersion,
 	}
 	parsedArgs, err := extractArgs(args.Args)
 	if err != nil {
@@ -168,29 +169,34 @@ func newPlugin(args *skel.CmdArgs) (*plugin, error) {
 	return plugin, nil
 }
 
-func getCurrentResult(args *skel.CmdArgs) (*current.Result, error) {
+func getCurrentResult(args *skel.CmdArgs) (*current.Result, string, error) {
 	// parse the previous CNI result (or throw an error if there wasn't one)
 	cniConf := types.NetConf{}
 	err := json.Unmarshal(args.StdinData, &cniConf)
 	if err != nil {
-		return nil, fmt.Errorf("failure checking for previous result output: %w", err)
+		return nil, "", fmt.Errorf("failure checking for previous result output: %w", err)
 	}
 
+	// Store the CNI version here, since current.NewResultFromResult will
+	// change the CNI version to ImplementedSpecVersion, which is usually
+	// the latest CNI version, not the version specified in the conf.
+	confCNIVersion := cniConf.CNIVersion
+
 	err = version.ParsePrevResult(&cniConf)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse previous CNI result: %w", err)
+		return nil, "", fmt.Errorf("failed to parse previous CNI result: %w", err)
 	}
 
 	if cniConf.PrevResult == nil {
-		return nil, &NoPreviousResultError{}
+		return nil, "", &NoPreviousResultError{}
 	}
 
 	currentResult, err := current.NewResultFromResult(cniConf.PrevResult)
 	if err != nil {
-		return nil, fmt.Errorf("failed to generate current result from previous CNI result: %w", err)
+		return nil, "", fmt.Errorf("failed to generate current result from previous CNI result: %w", err)
 	}
 
-	return currentResult, nil
+	return currentResult, confCNIVersion, nil
 }
 
 type plugin struct {
@@ -223,6 +229,9 @@ type plugin struct {
 	// currentResult is the CNI result object, initialized to the previous CNI
 	// result if there was any or to nil if there was no previous result provided
 	currentResult *current.Result
+
+	// confCniVersion is the CNI version specified by the conflist passed to tc-redirect-tap
+	confCNIVersion string
 }
 
 func (p plugin) add() error {
diff --git a/cmd/tc-redirect-tap/main_test.go b/cmd/tc-redirect-tap/main_test.go
index 78990a7..8f87238 100644
--- a/cmd/tc-redirect-tap/main_test.go
+++ b/cmd/tc-redirect-tap/main_test.go
@@ -195,13 +195,14 @@ func TestAddFailsCreateTapErr(t *testing.T) {
 
 func TestGetCurrentResult(t *testing.T) {
 	expectedResult := defaultTestPlugin().currentResult
+	expectedCNIVersion := "0.3.1"
 
 	netConf := &types.NetConf{
-		CNIVersion: "0.3.1",
+		CNIVersion: expectedCNIVersion,
 		Name:       "my-lil-network",
 		Type:       "my-lil-plugin",
 		RawPrevResult: map[string]interface{}{
-			"cniVersion": "0.3.1",
+			"cniVersion": expectedCNIVersion,
 			"interfaces": expectedResult.Interfaces,
 			"ips":        expectedResult.IPs,
 			"routes":     expectedResult.Routes,
@@ -216,10 +217,11 @@ func TestGetCurrentResult(t *testing.T) {
 		StdinData: rawPrevResultBytes,
 	}
 
-	actualResult, err := getCurrentResult(cmdArgs)
+	actualResult, actualCNIVersion, err := getCurrentResult(cmdArgs)
 	require.NoError(t, err, "failed to get current result from mock net conf")
 
 	assert.Equal(t, expectedResult, actualResult)
+	assert.Equal(t, expectedCNIVersion, actualCNIVersion)
 }
 
 func TestDel(t *testing.T) {