Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kdump-Remote-SSH-Configurations #3400

Open
wants to merge 225 commits into
base: master
Choose a base branch
from

Conversation

Ghulam-Bahoo
Copy link

What I did
Added remote kdump functionality using SSH in SONiC.

How I did it
I added two new commands and two options to configure the kdump remote ssh feature.

How to verify it
Upon kernel crash, kdump will transfer the crash report files to the ssh server.

with self.assertRaises(SystemExit) as sys_exit:
sonic_kdump_config.read_ssh_string()
self.assertEqual(sys_exit.exception.code, 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

No test cases no negative scenarios i.e invalid ssh_key/ssh_path?

Choose a reason for hiding this comment

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

The handling of valid and invalid SSH keys and paths is already implemented in the utility code. It covers the necessary scenarios, including validation for incorrect SSH keys or paths

mock_read_ssh_string.return_value = 'user@ip_address' # Simulate reading existing SSH string

# Call the function to test
sonic_kdump_config.write_ssh_string('user@ip_address')
Copy link
Contributor

Choose a reason for hiding this comment

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

user@ip_address ? can we test with actual valid values?

Choose a reason for hiding this comment

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

The handling of valid and invalid SSH keys and paths is already implemented in the utility code. It covers the necessary scenarios, including validation for incorrect SSH keys or paths

"""Tests the function `read_ssh_path(...)` in script `sonic-kdump-config`."""

# Test successful case with valid SSH path
mock_run_cmd.return_value = (0, ['/path/to/keys'], None)
Copy link
Contributor

Choose a reason for hiding this comment

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

/path/to/keys ? can we test with actual valid values?

Choose a reason for hiding this comment

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

The handling of valid and invalid SSH keys and paths is already implemented in the utility code. It covers the necessary scenarios, including validation for incorrect SSH keys or paths

@wajahatrazi
Copy link
Contributor

Hi @qiluo-msft , please review to merge this PR

@wajahatrazi
Copy link
Contributor

Hi @qiluo-msft / @lguohan

PR has been reviewed by the reviewer with all checks passed, please help merge this.

@ridahanif96
Copy link
Contributor

@qiluo-msft help merge this PR, pending for long

Copy link
Contributor

@wajahatrazi wajahatrazi left a comment

Choose a reason for hiding this comment

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

LGTM

@zhangyanzhao
Copy link
Collaborator

@qiluo-msft this PR has been approved by reviewers and can you please help to check if you are ok to merge? Thanks.

@muhammadalihussnain
Copy link

@qiluo-msft this PR has been approved by reviewers and can you please help to merge? Thanks.

@muhammadalihussnain
Copy link

@qiluo-msft this PR has been approved by reviewers and can you please help to merge? Thanks.


def write_ssh_string(ssh_string):
ssh_string = ssh_string.replace('/', '\/') # Escape any slashes in the SSH string
cmd = "/bin/sed -i -e 's/#*SSH=.*/SSH=\"%s\"/' %s" % (ssh_string, kdump_cfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

ssh_string

There is command inject risk. @maipbui to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maipbui Hi, Can you please look into this and help us? Thanks

@qiluo-msft qiluo-msft requested a review from maipbui February 26, 2025 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants