Merge pull request #185 from datum-cloud/fix/issue-157

fix: validate prevResult from preceding CNI plugin

Author: privateip

internal/cni/cni_test.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/containernetworking/cni/pkg/skel"
 	"github.com/containernetworking/cni/pkg/types"
+	type100 "github.com/containernetworking/cni/pkg/types/100"
 	bgpv1alpha1 "go.miloapis.com/cosmos/api/bgp/v1alpha1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -30,6 +31,15 @@ const (
 	testRD65000_1     = "65000:1"      // RD/RT for ASN 65000, NN 1
 	testContainerID   = "test-container"
 	testInvalidBase62 = "abc-def" // shared invalid base62 string for tests
+	testNetns         = "/proc/1/ns/net"
+	testMac           = "aa:bb:cc:dd:ee:ff"
+	testIfName        = "eth0"
+
+	// testPrevResult is a valid CNI v1.0.0 result used in prevResult tests.
+	testPrevResult = `{"cniVersion":"1.0.0",` +
+		`"interfaces":[{"name":"` + testIfName + `","mac":"` + testMac + `",` +
+		`"sandbox":"/proc/1/ns/net"}],` +
+		`"ips":[{"version":"6","address":"fd00:1::1/64"}]}`
 )
 
 func fakeClient(objs ...client.Object) client.Client {
@@ -292,6 +302,18 @@ func TestParseConf(t *testing.T) {
 			),
 			wantErr: srv6LocatorErrMsg,
 		},
+		{
+			name: "prevResult valid JSON result is accepted",
+			input: fmt.Sprintf(
+				`{"cniVersion":"1.0.0","name":"test",`+
+					`"type":"galactic-cni","vpc":"%s",`+
+					`"vpcattachment":"%s",`+
+					`"prevResult":%s}`,
+				testVPC, testAttachment, testPrevResult,
+			),
+			wantVPC:    testVPC,
+			wantIfType: interfaceTypeVeth,
+		},
 	}
 
 	for _, tt := range tests {
@@ -414,6 +436,92 @@ func TestSanitizeForError(t *testing.T) {
 	}
 }
 
+// ---- validatePrevResult --------------------------------------------------
+
+func TestValidatePrevResult(t *testing.T) {
+	validResult := &type100.Result{
+		CNIVersion: cniVersion100,
+		Interfaces: []*type100.Interface{
+			{Name: testIfName, Mac: testMac, Sandbox: testNetns},
+		},
+		IPs: []*type100.IPConfig{
+			{Address: *mustParseCIDR(t, "fd00:1::1/64")},
+		},
+	}
+
+	tests := []struct {
+		name    string
+		input   types.Result
+		wantErr bool
+	}{
+		{"nil result allowed", nil, false},
+		{"valid CNI result", validResult, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validatePrevResult(tt.input)
+			if tt.wantErr {
+				if err == nil {
+					t.Fatal("expected error, got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
+func TestValidatePrevResultAdd(t *testing.T) {
+	validWithInterface := &type100.Result{
+		CNIVersion: cniVersion100,
+		Interfaces: []*type100.Interface{
+			{Name: testIfName, Mac: testMac, Sandbox: testNetns},
+		},
+		IPs: []*type100.IPConfig{
+			{Address: *mustParseCIDR(t, "fd00:1::1/64")},
+		},
+	}
+	validWithIPsOnly := &type100.Result{
+		CNIVersion: cniVersion100,
+		IPs: []*type100.IPConfig{
+			{Address: *mustParseCIDR(t, "fd00:1::1/64")},
+		},
+	}
+	emptyResult := &type100.Result{
+		CNIVersion: cniVersion100,
+		// No interfaces, no IPs — should fail content validation.
+	}
+
+	tests := []struct {
+		name    string
+		input   types.Result
+		wantErr bool
+	}{
+		{"nil result allowed", nil, false},
+		{"valid result with interface", validWithInterface, false},
+		{"valid result with IPs only", validWithIPsOnly, false},
+		{"empty result (no interfaces or IPs)", emptyResult, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validatePrevResultAdd(tt.input)
+			if tt.wantErr {
+				if err == nil {
+					t.Fatal("expected error, got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
 // ---- bgpVRFInstanceName --------------------------------------------------
 
 func TestBGPVRFInstanceName(t *testing.T) {
@@ -1336,3 +1444,33 @@ func TestProbeAPIServerMalformedKubeconfig(t *testing.T) {
 		t.Fatalf("error %q does not contain original error", err.Error())
 	}
 }
+
+// ---- cmdAdd prevResult validation ----------------------------------------
+
+func TestCmdAddPrevResultValid(t *testing.T) {
+	// prevResult that is a valid CNI result. cmdAdd should pass prevResult
+	// validation and fail later due to missing NODE_NAME env var.
+	conf := fmt.Sprintf(
+		`{"cniVersion":"1.0.0","name":"test",`+
+			`"type":"galactic-cni","vpc":"%s",`+
+			`"vpcattachment":"%s",`+
+			`"prevResult":%s}`,
+		testVPC, testAttachment, testPrevResult,
+	)
+	args := &skel.CmdArgs{
+		ContainerID: testContainerID,
+		StdinData:   []byte(conf),
+	}
+
+	err := cmdAdd(args)
+	if err == nil {
+		t.Fatal("expected cmdAdd to fail for missing NODE_NAME, got nil")
+	}
+	// Should fail on NODE_NAME, not prevResult.
+	if strings.Contains(err.Error(), "prevResult validation in ADD") {
+		t.Fatalf("prevResult should not cause error when valid, got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "NODE_NAME") {
+		t.Fatalf("expected NODE_NAME error, got: %v", err)
+	}
+}

internal/cni/config.go

@@ -9,6 +9,9 @@ import (
 	"errors"
 	"fmt"
 	"net"
+
+	"github.com/containernetworking/cni/pkg/types"
+	type100 "github.com/containernetworking/cni/pkg/types/100"
 )
 
 const sanitizeForErrorBinary = "<binary>"
@@ -93,6 +96,54 @@ func parseStatusConf(data []byte) error {
 	return nil
 }
 
+// validatePrevResult checks that the prevResult (from a preceding plugin in
+// the CNI chain) is a valid, parseable CNI result. Returns an error if the
+// result is non-nil but cannot be parsed as a versioned CNI result, ensuring
+// galactic-cni fails fast rather than silently operating on garbage state.
+func validatePrevResult(res types.Result) error {
+	if res == nil {
+		return nil
+	}
+	// Marshal to JSON and re-parse to verify the result is structurally valid.
+	// This catches malformed results that survived CNI framework unmarshaling.
+	jsonBytes, err := json.Marshal(res)
+	if err != nil {
+		return fmt.Errorf("marshal prevResult: %w", err)
+	}
+	if _, err := type100.NewResult(jsonBytes); err != nil {
+		return fmt.Errorf("parse prevResult: %w", err)
+	}
+	return nil
+}
+
+// validatePrevResultAdd performs content-level validation of prevResult during
+// the ADD operation. It ensures the preceding plugin produced a result with at
+// least one interface or IP assignment, which is the minimum expected structure
+// for any meaningful CNI chain. Returns nil when prevResult is nil (no
+// preceding plugin) or structurally valid with expected content.
+func validatePrevResultAdd(res types.Result) error {
+	if res == nil {
+		return nil
+	}
+	jsonBytes, err := json.Marshal(res)
+	if err != nil {
+		return fmt.Errorf("marshal prevResult: %w", err)
+	}
+	result, err := type100.NewResult(jsonBytes)
+	if err != nil {
+		return fmt.Errorf("parse prevResult: %w", err)
+	}
+	versioned, err := type100.GetResult(result)
+	if err != nil {
+		return fmt.Errorf("get prevResult version: %w", err)
+	}
+	// A valid prevResult must declare at least one interface or IP assignment.
+	if len(versioned.Interfaces) == 0 && len(versioned.IPs) == 0 {
+		return errors.New("prevResult declares no interfaces or IP assignments")
+	}
+	return nil
+}
+
 // parseConf unmarshals the CNI configuration from stdin data and validates
 // the interface type and base62-encoded identifier fields. Returns an error
 // if the config is malformed, interface_type is unsupported, or VPC/
@@ -124,6 +175,11 @@ func parseConf(data []byte) (*PluginConf, error) {
 			srv6LocatorErrMsg, sanitizeForError(conf.SRv6Locator),
 		)
 	}
+	if conf.PrevResult != nil {
+		if err := validatePrevResult(conf.PrevResult); err != nil {
+			return nil, fmt.Errorf("invalid prevResult: %w", err)
+		}
+	}
 	if conf.InterfaceType == "" {
 		conf.InterfaceType = interfaceTypeVeth
 	}

internal/cni/ops_add.go

@@ -27,6 +27,16 @@ func cmdAdd(args *skel.CmdArgs) error {
 		return err
 	}
 
+	// Validate prevResult structure when present. The preceding plugin in the
+	// CNI chain should have produced a result with at least one interface or IP
+	// assignment. A nil or structurally broken prevResult indicates a mis-
+	// configured chain that galactic-cni should not silently ignore.
+	if pluginConf.PrevResult != nil {
+		if err := validatePrevResultAdd(pluginConf.PrevResult); err != nil {
+			return fmt.Errorf("prevResult validation in ADD: %w", err)
+		}
+	}
+
 	nodeName := os.Getenv("NODE_NAME")
 	if nodeName == "" {
 		return errors.New("NODE_NAME environment variable is not set")