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.

Sign up for GitHub

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

Jump to bottom

Implemented ssh configurations #32

Merged
merged 14 commits into from
Jun 27, 2023
Merged

Implemented ssh configurations #32

merged 14 commits into from
Jun 27, 2023

Conversation

ycoheNvidia
Copy link
Contributor

@ycoheNvidia ycoheNvidia commented Jan 10, 2023 •
edited
Loading

Why I did it

Implemented ssh configurations

How I did it

Added ssh config table in configDB, once changed - hostcfgd will change the relevant OS files (sshd_config)

How to verify it

Tests in added in this PR. User can change relevant configs in configDB such as ports, and see sshd port was modified

Description for the changelog

Added ssh config infrastructure

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Which release branch to backport (provide reason below if selected)

  • 202205
  • 202211
  • 202305

Link to config_db schema for YANG module changes

https://github.com/ycoheNvidia/SONiC/blob/ssh_config/doc/ssh_config/ssh_config.md

A picture of a cute animal (not mandatory but encouraged)

HLD in sonic-net/SONiC#1075
This PR is related to sonic-net/sonic-buildimage#13338

@ycoheNvidia ycoheNvidia mentioned this pull request Jan 10, 2023
Merged
@ycoheNvidia ycoheNvidia marked this pull request as draft January 10, 2023 09:38
@ycoheNvidia ycoheNvidia marked this pull request as ready for review January 10, 2023 09:42
@liat-grozovik
Copy link
Collaborator

@ycoheNvidia please handle conflicts
@qiluo-msft could you please help to review or assign someone?

@ycoheNvidia
Copy link
Contributor Author

@ycoheNvidia please handle conflicts @qiluo-msft could you please help to review or assign someone?

resolved

@liat-grozovik
Copy link
Collaborator

@ycoheNvidia could you please rerun checkers and follow up on failures?

@ycoheNvidia
Copy link
Contributor Author

@ycoheNvidia could you please rerun checkers and follow up on failures?

fixed

dgsudharsan
dgsudharsan suggested changes Mar 10, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
scripts/hostcfgd Outdated Show resolved Hide resolved
dgsudharsan
dgsudharsan suggested changes Mar 10, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
@ycoheNvidia ycoheNvidia deleted the sonic-host-services-ycoheNvidia-ssh_config branch March 13, 2023 07:16
@ycoheNvidia ycoheNvidia restored the sonic-host-services-ycoheNvidia-ssh_config branch March 19, 2023 07:03
@ycoheNvidia ycoheNvidia reopened this Mar 19, 2023
dgsudharsan
dgsudharsan suggested changes Mar 29, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
@liat-grozovik
Copy link
Collaborator

@dgsudharsan @qiluo-msft any further comments on this PR or can it be merged?

@dgsudharsan
Copy link
Contributor

@dgsudharsan @qiluo-msft any further comments on this PR or can it be merged?

@liat-grozovik , There is still one comment pending. I don't want run_cmd to be replaced with os.system since its a security vulnerability.

ycoheNvidia and others added 2 commits May 17, 2023 11:46
@ycoheNvidia
Merge branch 'sonic-net:master' into sonic-host-services-ycoheNvidia-…
4d4337f 
…ssh_config
@ycoheNvidia
Replaced os.cmd to subprocess.run
200bf27 
Replaced all ssh related os.cmd calls with modify_single_file_inplace and fixed it on the way
@ycoheNvidia ycoheNvidia force-pushed the sonic-host-services-ycoheNvidia-ssh_config branch from 73a2fff to 200bf27 Compare May 18, 2023 13:19
@ycoheNvidia ycoheNvidia mentioned this pull request May 22, 2023
Merged
3 tasks
@ycoheNvidia
Copy link
Contributor Author

@dgsudharsan @qiluo-msft any further comments on this PR or can it be merged?

@liat-grozovik , There is still one comment pending. I don't want run_cmd to be replaced with os.system since its a security vulnerability.

Comment was addressed. Thanks!

@ycoheNvidia
Copy link
Contributor Author

@qiluo-msft please add your comments/approval

@qiluo-msft qiluo-msft requested a review from liuh-80 June 19, 2023 06:58
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Jun 19, 2023 •
edited
Loading

Could you add HLD PR to this PR's description?
Could you add this PR's link into HLD PR's PR description? There should be a table containing all the PR for that HLD.

In reply to: 1596617344

@liuh-80
Copy link
Contributor

liuh-80 commented Jun 19, 2023 •
edited
Loading

@ycoheNvidia , in the PR description this PR depends on another PR merge first: sonic-net/sonic-buildimage#13319
But it's closed, what's the plan for that PR?

#closed

@ycoheNvidia
Copy link
Contributor Author

@ycoheNvidia , in the PR description this PR depends on another PR merge first: sonic-net/sonic-buildimage#13319 But it's closed, what's the plan for that PR?

Updated the description - link was updated to sonic-net/sonic-buildimage#13338

@qiluo-msft qiluo-msft requested a review from maipbui June 19, 2023 07:07
qiluo-msft
qiluo-msft reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
@qiluo-msft qiluo-msft requested a review from Blueve June 19, 2023 07:08
qiluo-msft
qiluo-msft reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
liuh-80
liuh-80 reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
liuh-80
liuh-80 reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
liuh-80
liuh-80 reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
liuh-80
liuh-80 reviewed Jun 19, 2023
View reviewed changes
scripts/hostcfgd Outdated Show resolved Hide resolved
@ycoheNvidia
Copy link
Contributor Author

Could you add HLD PR to this PR's description? Could you add this PR's link into HLD PR's PR description? There should be a table containing all the PR for that HLD.

done

@sonic-net sonic-net deleted a comment from azure-pipelines bot Jun 20, 2023
@sonic-net sonic-net deleted a comment from azure-pipelines bot Jun 20, 2023
@qiluo-msft
Copy link
Contributor

Could you merge the latest master code? Semgrep check is recently added into master, and this PR did not trigger it.

@ycoheNvidia
Copy link
Contributor Author

ycoheNvidia commented Jun 20, 2023 •
edited
Loading

Could you merge the latest master code? Semgrep check is recently added into master, and this PR did not trigger it.

Done

@ycoheNvidia
Copy link
Contributor Author

@liuh-80 @qiluo-msft all comments addressed, please take another look

qiluo-msft
qiluo-msft approved these changes Jun 25, 2023
View reviewed changes
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please check with other active reviewers.

@liat-grozovik liat-grozovik merged commit bc08806 into sonic-net:master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@dgsudharsan dgsudharsan dgsudharsan approved these changes

@liuh-80 liuh-80 liuh-80 approved these changes

@maipbui maipbui Awaiting requested review from maipbui

@Blueve Blueve Awaiting requested review from Blueve

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ycoheNvidia @liat-grozovik @dgsudharsan @qiluo-msft @liuh-80