Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions common_utils/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ const (
GNOI_HEALTHZ_ACK
GNOI_HEALTHZ_CHECK
GNOI_HEALTHZ_COLLECT
GNSI_CREDZ_SET
GNSI_CREDZ_CHECKPOINT
DBUS
DBUS_FAIL
DBUS_APPLY_PATCH_DB
Expand Down Expand Up @@ -95,6 +97,10 @@ func (c CounterType) String() string {
return "GNOI Healthz Check"
case GNOI_HEALTHZ_COLLECT:
return "GNOI Healthz Collect"
case GNSI_CREDZ_SET:
return "GNSI Credz Set"
case GNSI_CREDZ_CHECKPOINT:
return "GNSI Credz Checkpoint"
case DBUS:
return "DBUS"
case DBUS_FAIL:
Expand Down
123 changes: 123 additions & 0 deletions sonic_service_client/dbus_client.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package host_service

import (
"context"
"fmt"
"reflect"
"strings"
Expand Down Expand Up @@ -49,8 +50,37 @@ type Service interface {
// Docker services APIs
LoadDockerImage(image string) error
InstallOS(req string) (string, error)
//Credentialz service APIs
SSHMgmtSet(cmd string) error
GLOMEConfigSet(ctx context.Context, cmd string) error
SSHCheckpoint(action CredzCheckpointAction) error
GLOMERestoreCheckpoint(ctx context.Context) error
ConsoleSet(cmd string) error
ConsoleCheckpoint(action CredzCheckpointAction) error
}

// Define a function type that matches the DbusApi signature
type DbusApiFunc func(busName, busPath, intName string, timeout int, args ...interface{}) (interface{}, error)

// Use a variable to hold the implementation. It defaults to the real DbusApi function.
var dbusApiCaller DbusApiFunc = DbusApi

// NewDbusClientFunc defines the signature for creating a D-Bus client.
type NewDbusClientFunc func() (Service, error)

// NewDbusClientProvider is a variable that defaults to the real constructor.
// Tests can overwrite this to return a FakeClient.
var NewDbusClientProvider NewDbusClientFunc = NewDbusClient

type CredzCheckpointAction string

const (
CredzCPCreate CredzCheckpointAction = ".create_checkpoint"
CredzCPDelete CredzCheckpointAction = ".delete_checkpoint"
CredzCPRestore CredzCheckpointAction = ".restore_checkpoint"
CredzGlomePushConfig CredzCheckpointAction = ".push_config"
)

type DbusClient struct {
busNamePrefix string
busPathPrefix string
Expand Down Expand Up @@ -424,3 +454,96 @@ func (c *DbusClient) HealthzAck(req string) (string, error) {
}
return strResult, nil
}

func (c *DbusClient) ConsoleSet(cmd string) error {
modName := "gnsi_console"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + ".set"

common_utils.IncCounter(common_utils.GNSI_CREDZ_SET)
_, err := dbusApiCaller(busName, busPath, intName, 10, cmd)
return err
}

func (c *DbusClient) SSHMgmtSet(cmd string) error {
modName := "ssh_mgmt"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + ".set"

common_utils.IncCounter(common_utils.GNSI_CREDZ_SET)
_, err := dbusApiCaller(busName, busPath, intName, 10, cmd)
return err
}

// GLOMEConfigSet is used to write the GLOME config in the host service file system.
func (c *DbusClient) GLOMEConfigSet(ctx context.Context, cmd string) error {
modName := "glome"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + string(CredzGlomePushConfig)

common_utils.IncCounter(common_utils.GNSI_CREDZ_SET)
timeout := 10 // Default timeout in seconds.
if deadline, ok := ctx.Deadline(); ok {
remaining := time.Until(deadline)
if remaining <= 0 {
return context.DeadlineExceeded
}
timeout = int(remaining.Seconds())
if timeout > 10 {
timeout = 10
}
}
_, err := dbusApiCaller(busName, busPath, intName, timeout, cmd)
return err
}

func (c *DbusClient) ConsoleCheckpoint(action CredzCheckpointAction) error {
modName := "gnsi_console"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + string(action)

common_utils.IncCounter(common_utils.GNSI_CREDZ_CHECKPOINT)
_, err := dbusApiCaller(busName, busPath, intName, 10, "")
return err
}

func (c *DbusClient) SSHCheckpoint(action CredzCheckpointAction) error {
modName := "ssh_mgmt"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + string(action)

common_utils.IncCounter(common_utils.GNSI_CREDZ_CHECKPOINT)
_, err := dbusApiCaller(busName, busPath, intName, 10, "")
return err
}

// GLOMERestoreCheckpoint is used to restore the GLOME config metadata to the
// checkpoint state. This is used to rollback the GLOME config in the host
// service file system.
func (c *DbusClient) GLOMERestoreCheckpoint(ctx context.Context) error {
modName := "glome"
busName := c.busNamePrefix + modName
busPath := c.busPathPrefix + modName
intName := c.intNamePrefix + modName + string(CredzCPRestore)

common_utils.IncCounter(common_utils.GNSI_CREDZ_CHECKPOINT)
// Default timeout in seconds. Set to 5 minutes to give enough time for rollback.
timeout := 300
if deadline, ok := ctx.Deadline(); ok {
remaining := time.Until(deadline)
if remaining <= 0 {
return context.DeadlineExceeded
}
timeout = int(remaining.Seconds())
if timeout > 10 {
timeout = 10
}
}
Comment on lines +536 to +546
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a typo here? Seems like it should be 300 in the nested if clause

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @Bojun-Feng , Thanks for pointing this.
Yes we think it is a typo error. So if timeout exceeds 300s, it can be reset.
Is it fine to change the lines 543 -545 as below.? Please suggest if you have any comments here.
if timeout > 300 {
timeout = 300
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ndas7 @Bojun-Feng . Please respond , so that I can update the code and submit for review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @niranjanivivek, apologies for the delay. For some context, I noticed the discrepancy in the code since I usually paste code blocks and forget to update the value in nested conditions. However, I am not fully knowledgeable on the context. I believe @ndas7 needs to confirm whether we want to align the value to 10 or 300.

I would recommend a single source of truth in the code block cited for this comment. Define another variable named threshold / timeout_cap / something descriptive, then reuse it for the entire block so the hard coded number only appears one time. This should reduce similar issues in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @niranjanivivek, apologies for the delay. For some context, I noticed the discrepancy in the code since I usually paste code blocks and forget to update the value in nested conditions. However, I am not fully knowledgeable on the context. I believe @ndas7 needs to confirm whether we want to align the value to 10 or 300.

I would recommend a single source of truth in the code block cited for this comment. Define another variable named threshold / timeout_cap / something descriptive, then reuse it for the entire block so the hard coded number only appears one time. This should reduce similar issues in the future.

@ndas7 Please suggest the correct timeout value here as requested by Bojun-Feng.

_, err := dbusApiCaller(busName, busPath, intName, timeout)
return err
}
132 changes: 132 additions & 0 deletions sonic_service_client/dbus_client_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package host_service

import (
"context"
"errors"
"fmt"
"github.com/agiledragon/gomonkey/v2"
Expand All @@ -11,6 +12,7 @@ import (
"reflect"
"strings"
"testing"
"time"
)

func TestNewDbusClient(t *testing.T) {
Expand Down Expand Up @@ -1656,3 +1658,133 @@ func TestHealthzAck_InvalidReturnType(t *testing.T) {
assert.Contains(t, err.Error(), "Invalid result type")
assert.Equal(t, "", result)
}

func TestCredentialzDbusMethods(t *testing.T) {
client := &DbusClient{
busNamePrefix: "org.SONiC.HostService.",
busPathPrefix: "/org/SONiC/HostService/",
intNamePrefix: "org.SONiC.HostService.",
}
// Save the original implementation to restore it later
originalDbusApi := dbusApiCaller
defer func() { dbusApiCaller = originalDbusApi }()

t.Run("ConsoleSet", func(t *testing.T) {
expectedCmd := "test-password-json"
// Overwrite the caller variable with a mock
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {
assert.Equal(t, "org.SONiC.HostService.gnsi_console", bus)
assert.Equal(t, "org.SONiC.HostService.gnsi_console.set", intf)
assert.Equal(t, expectedCmd, args[0])
return nil, nil
}

err := client.ConsoleSet(expectedCmd)
assert.NoError(t, err)
})

t.Run("SSHMgmtSet", func(t *testing.T) {
expectedCmd := "test-ssh-key-json"
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {
assert.Equal(t, "org.SONiC.HostService.ssh_mgmt", bus)
assert.Equal(t, "org.SONiC.HostService.ssh_mgmt.set", intf)
assert.Equal(t, expectedCmd, args[0])
return nil, nil
}

err := client.SSHMgmtSet(expectedCmd)
assert.NoError(t, err)
})

t.Run("SSHCheckpoint", func(t *testing.T) {
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {
assert.Equal(t, "org.SONiC.HostService.ssh_mgmt.create_checkpoint", intf)
assert.Equal(t, "", args[0])
return nil, nil
}

err := client.SSHCheckpoint(CredzCPCreate)
assert.NoError(t, err)
})

t.Run("GLOMEConfigSetWithContext", func(t *testing.T) {
expectedCmd := "glome-json"
// Create a context with a 5-second deadline to test timeout calculation
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {
assert.Equal(t, "org.SONiC.HostService.glome.push_config", intf)
assert.Equal(t, expectedCmd, args[0])
// Timeout should be roughly 5 (based on ctx)
assert.True(t, timeout <= 5)
return nil, nil
}

err := client.GLOMEConfigSet(ctx, expectedCmd)
assert.NoError(t, err)
})

t.Run("GLOMERestoreCheckpoint", func(t *testing.T) {
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {

assert.Equal(t, "org.SONiC.HostService.glome.restore_checkpoint", intf)
// Restore checkpoint for GLOME often has a high default timeout (300s)
assert.Equal(t, 300, timeout)
return nil, nil
}
err := client.GLOMERestoreCheckpoint(context.Background())
assert.NoError(t, err)
})
}

func TestConsoleCheckpoint(t *testing.T) {
client := &DbusClient{
busNamePrefix: "org.SONiC.HostService.",
busPathPrefix: "/org/SONiC/HostService/",
intNamePrefix: "org.SONiC.HostService.",
}
originalDbusApi := dbusApiCaller
defer func() { dbusApiCaller = originalDbusApi }()

tests := []struct {
name string
action CredzCheckpointAction
wantIntf string
}{
{
name: "Console Create Checkpoint",
action: CredzCPCreate,
wantIntf: "org.SONiC.HostService.gnsi_console.create_checkpoint",
},
{
name: "Console Delete Checkpoint",
action: CredzCPDelete,
wantIntf: "org.SONiC.HostService.gnsi_console.delete_checkpoint",
},
{
name: "Console Restore Checkpoint",
action: CredzCPRestore,
wantIntf: "org.SONiC.HostService.gnsi_console.restore_checkpoint",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
dbusApiCaller = func(bus, path, intf string, timeout int, args ...interface{}) (interface{}, error) {
assert.Equal(t, "org.SONiC.HostService.gnsi_console", bus)
assert.Equal(t, "/org/SONiC/HostService/gnsi_console", path)
assert.Equal(t, tt.wantIntf, intf)
assert.Equal(t, 10, timeout)

// Verify ConsoleCheckpoint passes an empty string as the payload
assert.Len(t, args, 1)
assert.Equal(t, "", args[0])

return nil, nil
}

err := client.ConsoleCheckpoint(tt.action)
assert.NoError(t, err)
})
}
}
32 changes: 32 additions & 0 deletions sonic_service_client/dbus_fake_client.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package host_service

import (
"context"
"errors"
"fmt"
)

// FakeClient is a mock implementation of the Service interface.
type FakeClient struct {
CollectResponse string
Command chan []string
}

func (f *FakeClient) Close() error { return nil }
Expand Down Expand Up @@ -90,3 +92,33 @@ func (f *FakeClient) HealthzAck(req string) (string, error) {
func (f *FakeClientWithError) HealthzAck(req string) (string, error) {
return "", fmt.Errorf("simulated dbus error")
}

func (f *FakeClient) ConsoleCheckpoint(action CredzCheckpointAction) error {
f.Command <- []string{"gnsi_console" + string(action), ""}
return nil
}

func (f *FakeClient) ConsoleSet(cmd string) error {
f.Command <- []string{"gnsi_console.set", cmd}
return nil
}

func (f *FakeClient) SSHCheckpoint(action CredzCheckpointAction) error {
f.Command <- []string{"ssh_mgmt" + string(action), ""}
return nil
}

func (f *FakeClient) SSHMgmtSet(cmd string) error {
f.Command <- []string{"ssh_mgmt.set", cmd}
return nil
}

func (f *FakeClient) GLOMEConfigSet(ctx context.Context, cmd string) error {
f.Command <- []string{"glome" + string(CredzGlomePushConfig), cmd}
return nil
}

func (f *FakeClient) GLOMERestoreCheckpoint(ctx context.Context) error {
f.Command <- []string{"glome" + string(CredzCPRestore), ""}
return nil
}
Loading
Loading