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

Add California-SB237 feature. Requires to change default user password #12678

Merged
merged 3 commits into from
Feb 23, 2023

Conversation

andriydnvd
Copy link
Contributor

Signed-off-by: Andriy Dobush andriyd@nvidia.com

Why I did it

Add support of California-SB237 conformance.
https://github.com/sonic-net/SONiC/tree/master/doc/California-SB237

How I did it

Expire user passwords during build

How to verify it

Enable build flag and check if default user is prompted for a new password

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205

Description for the changelog

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.

Link to config_db schema for YANG module changes

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

Signed-off-by: Andriy Dobush <andriyd@nvidia.com>
Copy link
Contributor

@liuh-80 liuh-80 left a comment

Choose a reason for hiding this comment

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

Is there a PR to validate this change work?

And as we know if we enable this feature following will break:

  1. https://github.com/sonic-net/sonic-buildimage/blob/master/check_install.py run in build image and will try to login to device.
  2. UTs in sonic-mgmt will break because they will login to device.

I think we need fix 1, and what's the plan for fix 2?

Also, we need UT in sonic-mgmt to cover this feature.

@Yarden-Z
Copy link

Yarden-Z commented Dec 6, 2022

Is there a PR to validate this change work?

And as we know if we enable this feature following will break:

1. https://github.com/sonic-net/sonic-buildimage/blob/master/check_install.py run in build image and will try to login to device.

2. UTs in sonic-mgmt will break because they will login to device.

I think we need fix 1, and what's the plan for fix 2?

Also, we need UT in sonic-mgmt to cover this feature.

Why do we need UT in sonic-mgmt for this feature?
This only works on initial boot, so this will require adjustments...
Initial discussions in regards to testing talked about handling the infra required changes and that will suffice as the test suite.
Are there additional areas where you think we should test this?

…sword and revert it back

Signed-off-by: Andriy Dobush <andriyd@nvidia.com>
@andriydnvd
Copy link
Contributor Author

Is there a PR to validate this change work?

And as we know if we enable this feature following will break:

  1. https://github.com/sonic-net/sonic-buildimage/blob/master/check_install.py run in build image and will try to login to device.

Updated check_install.py in build image to be ready for prompting new password. Original password is reverted back by script

@liuh-80
Copy link
Contributor

liuh-80 commented Dec 7, 2022

Are there additional areas where you think we should test this?

The reason we need UT in sonic-mgmt are:

  1. This is a new feature, we need UT to protect this feature does not break by other change.
  2. If some customers enable this feature by default, Then because the login step changed, all existed UT will break. so the code in sonic-mgmt need update to make sure all UT still can work when this feature enabled.

@liuh-80
Copy link
Contributor

liuh-80 commented Dec 7, 2022

I create following PR to validate this change:
#12983

As you can see, after enable this feature, every UT failed in 'prepare testbed' stage:
https://dev.azure.com/mssonic/build/_build/results?buildId=185495&view=logs&j=a35c5435-5100-57cf-f655-60b256043d84&t=c9f737d3-1e43-5e60-26d7-f79fbfea8dbb

I'm didn't check detail of why that step failed, but I think that step failed because current code trying to login but not reset password.

I suggest fix UT break issue in sonic-mgmt before this PR merge. the fix can reuse the logic in check_install.py.

Current UT PR only add new UT to cover this feature, but existed UTs will break when this feature enabled.

Copy link
Contributor

@liuh-80 liuh-80 left a comment

Choose a reason for hiding this comment

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

According to discussion in sonic-net/sonic-mgmt#6863
There will be another PR to change sonic-mgmt to support this feature enabled.

check_install.py Outdated
@@ -11,6 +11,7 @@ def main():
parser = argparse.ArgumentParser(description='test_login cmdline parser')
parser.add_argument('-u', default="admin", help='login user name')
parser.add_argument('-P', default="YourPaSsWoRd", help='login password')
parser.add_argument('-P2', default="Test@2022", help='new password')
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 13, 2023

Choose a reason for hiding this comment

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

This will conflict with -P. For multiple char option, suggest --P2.
-P2 == -P + -2 in many command option conventions.
ref: https://stackoverflow.com/a/21286343
#Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, can it be some literal line '-N', default="Test@2022", help='new password' ?

Signed-off-by: Andriy Dobush <andriyd@nvidia.com>
@andriydnvd
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft merged commit c1dd94f into sonic-net:master Feb 23, 2023
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Mar 28, 2023
Related work items: sonic-net#276, sonic-net#305, sonic-net#332, sonic-net#338, sonic-net#339, sonic-net#1188, sonic-net#1192, sonic-net#1197, sonic-net#1206, sonic-net#1685, sonic-net#1690, sonic-net#1696, sonic-net#1699, sonic-net#1709, sonic-net#1727, sonic-net#1737, sonic-net#1741, sonic-net#1742, sonic-net#2511, sonic-net#2512, sonic-net#2532, sonic-net#2559, sonic-net#2626, sonic-net#2638, sonic-net#2645, sonic-net#2649, sonic-net#2660, sonic-net#2669, sonic-net#2670, sonic-net#2678, sonic-net#10084, sonic-net#11442, sonic-net#11873, sonic-net#12047, sonic-net#12110, sonic-net#12207, sonic-net#12529, sonic-net#12678, sonic-net#13235, sonic-net#13287, sonic-net#13372, sonic-net#13395, sonic-net#13456, sonic-net#13497, sonic-net#13522, sonic-net#13545, sonic-net#13547, sonic-net#13552, sonic-net#13569, sonic-net#13572, sonic-net#13578, sonic-net#13591, sonic-net#13611, sonic-net#13647, sonic-net#13649, sonic-net#13660, sonic-net#13710, sonic-net#13716, sonic-net#13724, sonic-net#13726, sonic-net#13732, sonic-net#13735, sonic-net#13739, sonic-net#13757, sonic-net#13786, sonic-net#13792, sonic-net#13800, sonic-net#13801, sonic-net#13802, sonic-net#13805, sonic-net#13806, sonic-net#13812, sonic-net#13814, sonic-net#13822, sonic-net#13831, sonic-net#13834, sonic-net#13847, sonic-net#13870, sonic-net#13882, sonic-net#13884, sonic-net#13885, sonic-net#13894, sonic-net#13895, sonic-net#13926, sonic-net#13932, sonic-net#13935, sonic-net#13942, sonic-net#13951, sonic-net#13953, sonic-net#13964
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.

5 participants