diff --git a/pkg/gnoi/system/system.go b/pkg/gnoi/system/system.go index b14764971..7e7d6cc16 100644 --- a/pkg/gnoi/system/system.go +++ b/pkg/gnoi/system/system.go @@ -86,10 +86,6 @@ func HandleSetPackage(ctx context.Context, req *syspb.SetPackageRequest) (*syspb log.Errorf("Filename is missing in package request") return nil, status.Errorf(codes.InvalidArgument, "filename is missing in package request") } - if pkg.Package.Version == "" { - log.Errorf("Version is missing in package request") - return nil, status.Errorf(codes.InvalidArgument, "version is missing in package request") - } // Reject RemoteDownload - require local image if pkg.Package.RemoteDownload != nil { @@ -97,16 +93,28 @@ func HandleSetPackage(ctx context.Context, req *syspb.SetPackageRequest) (*syspb return nil, status.Errorf(codes.InvalidArgument, "remote download is not supported, image must be local") } - // Log the package details - log.V(1).Infof("Installing package: filename=%s, version=%s, activate=%v", - pkg.Package.Filename, pkg.Package.Version, pkg.Package.Activate) - // Validate filename is absolute path if !filepath.IsAbs(pkg.Package.Filename) { log.Errorf("Filename must be an absolute path: %s", pkg.Package.Filename) return nil, status.Errorf(codes.InvalidArgument, "filename must be an absolute path") } + // Resolve version: use provided version, or auto-resolve from image binary + version := pkg.Package.Version + if version == "" { + resolved, err := getBinaryVersion(ctx, pkg.Package.Filename) + if err != nil { + log.Errorf("Failed to resolve version from image %s: %v", pkg.Package.Filename, err) + return nil, status.Errorf(codes.Internal, "failed to resolve version from image: %v", err) + } + version = resolved + log.V(1).Infof("Auto-resolved version from image: %s", version) + } + + // Log the package details + log.V(1).Infof("Installing package: filename=%s, version=%s, activate=%v", + pkg.Package.Filename, version, pkg.Package.Activate) + // Install the package using sonic-installer if err := installPackage(ctx, pkg.Package.Filename); err != nil { log.Errorf("Failed to install package %s: %v", pkg.Package.Filename, err) @@ -117,11 +125,11 @@ func HandleSetPackage(ctx context.Context, req *syspb.SetPackageRequest) (*syspb // If activate is requested, set as next boot image if pkg.Package.Activate { - if err := activatePackage(ctx, pkg.Package.Version); err != nil { - log.Errorf("Failed to activate package %s: %v", pkg.Package.Version, err) + if err := activatePackage(ctx, version); err != nil { + log.Errorf("Failed to activate package %s: %v", version, err) return nil, status.Errorf(codes.Internal, "failed to activate package: %v", err) } - log.V(1).Infof("Successfully activated package %s", pkg.Package.Version) + log.V(1).Infof("Successfully activated package %s", version) } return &syspb.SetPackageResponse{}, nil @@ -136,7 +144,7 @@ func installPackage(ctx context.Context, filename string) error { opts := &exec.RunHostCommandOptions{ Timeout: 10 * time.Minute, // Allow up to 10 minutes for installation } - result, err := exec.RunHostCommand(ctx, "sonic-installer", []string{"install", "-y", filename}, opts) + result, err := exec.RunHostCommand(ctx, "/usr/local/bin/sonic-installer", []string{"install", "-y", filename}, opts) if err != nil { return fmt.Errorf("failed to run sonic-installer install: %v", err) } @@ -150,6 +158,32 @@ func installPackage(ctx context.Context, filename string) error { return nil } +// getBinaryVersion extracts the version string from a SONiC image using sonic-installer binary-version. +func getBinaryVersion(ctx context.Context, filename string) (string, error) { + log.V(1).Infof("Resolving binary version for: %s", filename) + + opts := &exec.RunHostCommandOptions{ + Timeout: 1 * time.Minute, + } + result, err := exec.RunHostCommand(ctx, "/usr/local/bin/sonic-installer", []string{"binary-version", filename}, opts) + if err != nil { + return "", fmt.Errorf("failed to run sonic-installer binary-version: %v", err) + } + + if result.Error != nil { + return "", fmt.Errorf("sonic-installer binary-version failed with exit code %d: %s", + result.ExitCode, result.Stderr) + } + + version := strings.TrimSpace(result.Stdout) + if version == "" { + return "", fmt.Errorf("sonic-installer binary-version returned empty output for %s", filename) + } + + log.V(1).Infof("Resolved binary version: %s", version) + return version, nil +} + // activatePackage sets a SONiC image as the next boot image using sonic-installer set-default. func activatePackage(ctx context.Context, version string) error { log.V(1).Infof("Activating package version: %s", version) @@ -159,7 +193,7 @@ func activatePackage(ctx context.Context, version string) error { opts := &exec.RunHostCommandOptions{ Timeout: 2 * time.Minute, // Allow up to 2 minutes for setting default } - result, err := exec.RunHostCommand(ctx, "sonic-installer", []string{"set-default", version}, opts) + result, err := exec.RunHostCommand(ctx, "/usr/local/bin/sonic-installer", []string{"set-default", version}, opts) if err != nil { return fmt.Errorf("failed to run sonic-installer set-default: %v", err) } diff --git a/pkg/gnoi/system/system_monkey_test.go b/pkg/gnoi/system/system_monkey_test.go index aa65f32b7..033dddf1a 100644 --- a/pkg/gnoi/system/system_monkey_test.go +++ b/pkg/gnoi/system/system_monkey_test.go @@ -16,7 +16,7 @@ func TestHandleSetPackage_SuccessWithoutActivation(t *testing.T) { // Mock exec.RunHostCommand to return successful installation patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { - if cmd == "sonic-installer" && len(args) >= 1 && args[0] == "install" { + if cmd == "/usr/local/bin/sonic-installer" && len(args) >= 1 && args[0] == "install" { return &exec.CommandResult{ Stdout: "Installation completed successfully", Stderr: "", @@ -48,13 +48,206 @@ func TestHandleSetPackage_SuccessWithoutActivation(t *testing.T) { } } +func TestHandleSetPackage_ActivateWithAutoResolvedVersion(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + // Mock exec.RunHostCommand to handle binary-version, install, and set-default + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + if cmd == "/usr/local/bin/sonic-installer" { + if len(args) >= 1 && args[0] == "binary-version" { + return &exec.CommandResult{ + Stdout: "SONiC-OS-4.2.0-Enterprise\n", + ExitCode: 0, + }, nil + } + if len(args) >= 1 && args[0] == "install" { + return &exec.CommandResult{ + Stdout: "Installation completed successfully", + ExitCode: 0, + }, nil + } + if len(args) >= 2 && args[0] == "set-default" { + if args[1] != "SONiC-OS-4.2.0-Enterprise" { + return nil, fmt.Errorf("unexpected version: %s", args[1]) + } + return &exec.CommandResult{ + Stdout: "Default image set successfully", + ExitCode: 0, + }, nil + } + } + return nil, fmt.Errorf("unexpected command: %s %v", cmd, args) + }) + + ctx := context.Background() + req := &syspb.SetPackageRequest{ + Request: &syspb.SetPackageRequest_Package{ + Package: &syspb.Package{ + Filename: "/tmp/test-image.bin", + Version: "", // Empty! Should auto-resolve before install + Activate: true, + }, + }, + } + + resp, err := HandleSetPackage(ctx, req) + if err != nil { + t.Fatalf("HandleSetPackage() returned error: %v", err) + } + if resp == nil { + t.Fatal("HandleSetPackage() returned nil response") + } +} + +func TestHandleSetPackage_AutoResolveVersionFails(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + // Mock: binary-version fails — should reject before install + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + if cmd == "/usr/local/bin/sonic-installer" && len(args) >= 1 && args[0] == "binary-version" { + return nil, fmt.Errorf("binary-version command failed") + } + return nil, fmt.Errorf("unexpected command (install should not be called): %s %v", cmd, args) + }) + + ctx := context.Background() + req := &syspb.SetPackageRequest{ + Request: &syspb.SetPackageRequest_Package{ + Package: &syspb.Package{ + Filename: "/tmp/test-image.bin", + Version: "", + Activate: true, + }, + }, + } + + _, err := HandleSetPackage(ctx, req) + if err == nil { + t.Fatal("HandleSetPackage() should return error when binary-version fails") + } + if !containsSubstring(err.Error(), "failed to resolve version from image") { + t.Errorf("HandleSetPackage() error = %v, should contain 'failed to resolve version from image'", err) + } +} + +func TestHandleSetPackage_EmptyVersionNoActivate(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + // Mock: binary-version and install succeed, set-default should NOT be called + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + if cmd == "/usr/local/bin/sonic-installer" { + if len(args) >= 1 && args[0] == "binary-version" { + return &exec.CommandResult{ + Stdout: "SONiC-OS-4.2.0-Enterprise\n", + ExitCode: 0, + }, nil + } + if len(args) >= 1 && args[0] == "install" { + return &exec.CommandResult{ + Stdout: "Installation completed successfully", + ExitCode: 0, + }, nil + } + if len(args) >= 1 && args[0] == "set-default" { + return nil, fmt.Errorf("set-default should not be called when activate=false") + } + } + return nil, fmt.Errorf("unexpected command: %s %v", cmd, args) + }) + + ctx := context.Background() + req := &syspb.SetPackageRequest{ + Request: &syspb.SetPackageRequest_Package{ + Package: &syspb.Package{ + Filename: "/tmp/test-image.bin", + Version: "", + Activate: false, + }, + }, + } + + resp, err := HandleSetPackage(ctx, req) + if err != nil { + t.Fatalf("HandleSetPackage() returned error: %v", err) + } + if resp == nil { + t.Fatal("HandleSetPackage() returned nil response") + } +} + +func TestGetBinaryVersion_Success(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + if cmd == "/usr/local/bin/sonic-installer" && len(args) >= 1 && args[0] == "binary-version" { + return &exec.CommandResult{ + Stdout: "SONiC-OS-4.2.0-Enterprise\n", + ExitCode: 0, + }, nil + } + return nil, fmt.Errorf("unexpected command: %s %v", cmd, args) + }) + + ctx := context.Background() + version, err := getBinaryVersion(ctx, "/tmp/test-image.bin") + if err != nil { + t.Fatalf("getBinaryVersion() returned error: %v", err) + } + if version != "SONiC-OS-4.2.0-Enterprise" { + t.Errorf("getBinaryVersion() = %q, want %q", version, "SONiC-OS-4.2.0-Enterprise") + } +} + +func TestGetBinaryVersion_EmptyOutput(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + return &exec.CommandResult{ + Stdout: "", + ExitCode: 0, + }, nil + }) + + ctx := context.Background() + _, err := getBinaryVersion(ctx, "/tmp/test-image.bin") + if err == nil { + t.Fatal("getBinaryVersion() should return error for empty output") + } + if !containsSubstring(err.Error(), "returned empty output") { + t.Errorf("getBinaryVersion() error = %v, should contain 'returned empty output'", err) + } +} + +func TestGetBinaryVersion_CommandError(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { + return nil, fmt.Errorf("command not found") + }) + + ctx := context.Background() + _, err := getBinaryVersion(ctx, "/tmp/test-image.bin") + if err == nil { + t.Fatal("getBinaryVersion() should return error when command fails") + } + if !containsSubstring(err.Error(), "failed to run sonic-installer binary-version") { + t.Errorf("getBinaryVersion() error = %v, should contain 'failed to run sonic-installer binary-version'", err) + } +} + func TestHandleSetPackage_SuccessWithActivation(t *testing.T) { patches := gomonkey.NewPatches() defer patches.Reset() // Mock exec.RunHostCommand to handle both install and set-default commands patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { - if cmd == "sonic-installer" { + if cmd == "/usr/local/bin/sonic-installer" { if len(args) >= 1 && args[0] == "install" { return &exec.CommandResult{ Stdout: "Installation completed successfully", @@ -132,7 +325,7 @@ func TestHandleSetPackage_ActivateCommandError(t *testing.T) { // Mock exec.RunHostCommand: install succeeds, set-default fails patches.ApplyFunc(exec.RunHostCommand, func(ctx context.Context, cmd string, args []string, opts *exec.RunHostCommandOptions) (*exec.CommandResult, error) { - if cmd == "sonic-installer" { + if cmd == "/usr/local/bin/sonic-installer" { if len(args) >= 1 && args[0] == "install" { return &exec.CommandResult{ Stdout: "Installation completed successfully", diff --git a/pkg/gnoi/system/system_test.go b/pkg/gnoi/system/system_test.go index 2006e3d13..22723dca0 100644 --- a/pkg/gnoi/system/system_test.go +++ b/pkg/gnoi/system/system_test.go @@ -41,7 +41,7 @@ func TestHandleSetPackageValidation(t *testing.T) { errMsg: "filename is missing", }, { - name: "missing version", + name: "empty version - resolved from image before install", req: &syspb.SetPackageRequest{ Request: &syspb.SetPackageRequest_Package{ Package: &syspb.Package{ @@ -50,8 +50,7 @@ func TestHandleSetPackageValidation(t *testing.T) { }, }, }, - wantErr: true, - errMsg: "version is missing", + wantErr: false, // Version is auto-resolved from image; no longer rejected }, { name: "remote download not supported",