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 support for host certificate deletion, display and installation #483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tejaswi-Goel
Copy link

@Tejaswi-Goel Tejaswi-Goel commented Nov 19, 2024

SUMMARY
Add ansible support for SONiC PKI host certificates install, delete and display.

GitHub Issues

List the GitHub issues impacted by this PR. If no Github issues are affected, please indicate this with "N/A".

GitHub Issue #
N/A
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

sonic_pki

OUTPUT

Regression Run Output
regression_sonic_pki_tasks_result.txt

Checklist:
  • I have performed a self-review of my own code to ensure there are no formatting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility or have provided any relevant "breaking_changes" descriptions in a "fragment" file in the "changelogs/fragments" directory of this repository.
  • I have provided a summary for this PR in valid "fragment" file format in the "changelogs/fragments" directory of this repository branch. Reference : Ansible Change Log Document
How Has This Been Tested?

Regression test with updated testcases.

@Tejaswi-Goel Tejaswi-Goel force-pushed the pki-host-cert-install-del branch from 4990cf9 to 48be4f8 Compare November 20, 2024 01:03
@Tejaswi-Goel Tejaswi-Goel marked this pull request as ready for review November 20, 2024 05:44
@stalabi1 stalabi1 added the enhancement New feature or request label Jan 23, 2025

PATCH = "patch"
DELETE = "delete"
PUT = "put"
POST = 'post'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
POST = 'post'
POST = "post"

A little nitpick but for consistency you should use double quotes here

"path": INSTALL_PATH,
"method": POST,
"data": {"openconfig-pki-rpc:input":
{"file-path": cert.get('file_path'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{"file-path": cert.get('file_path'),
{"file-path": cert.get("file_path"),

A little nitpick but for consistency you should use double quotes here

"data": {"openconfig-pki-rpc:input":
{"file-path": cert.get('file_path'),
"key-path": cert.get('key_path'),
"fips-cert": cert.get('fips_cert')}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"fips-cert": cert.get('fips_cert')}}
"fips-cert": cert.get("fips_cert")}}

A little nitpick but for consistency you should use double quotes here

Copy link
Collaborator

@awhaley-dell awhaley-dell left a comment

Choose a reason for hiding this comment

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

I am asking for some small formatting consistency requests and explanations for a few conditional code snippets before I am ready to approve this PR.

@@ -322,6 +325,20 @@ def _state_merged(self, want, have, diff):
}
)

# Handle INSTALL for certificates
for cert in commands.get("host_cert") or []:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why the empty list is necessary here? I don't immediately see a function of that or clause here.

@@ -371,6 +388,22 @@ def _state_deleted(self, want, have, diff):
if ts.get("name") in current_ts:
requests.extend(mk_ts_delete(ts, have))

# Handle DELETE for certificates
for cert in commands.get("host_cert") or []:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why the empty list is necessary here? I don't immediately see a function of that or clause here.

if cert_data and len(cert_data) > 0 and "openconfig-pki-rpc:output" in cert_data[0][1]:
cert_data = cert_data[0][1].get("openconfig-pki-rpc:output", {})
if cert_data and "filename" in cert_data:
filenames = cert_data.get("filename", []) or []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the "or []" do anything in this statement? If the get() does not find anything with the key "filename" it will return an empty list. I am unclear on what the "or []" snippet does here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants