fix host_cert test for Arista #4768
Conversation
Summary of ChangesHello @ram-mac, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a failure in the host certificate test specifically for Arista devices. The core change involves ensuring that the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Functional Test Report for #4768 / 3a78502Virtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix a host certificate test for Arista devices by including the FQDN when scanning for the host key. The changes modify GetConfiguredHostKey to accept an FQDN and update the test to provide one. My review has identified a few areas for improvement. The new logic in GetConfiguredHostKey could be more robust when handling an empty FQDN. The test now hardcodes an FQDN, which impacts portability. Finally, there's a potential inconsistency between the hostname used in the test and the principal name embedded in the host certificate, which could make the test brittle. Details and suggestions are in the specific comments.
| } | ||
| wantHostKey := strings.Trim(string(ssh.MarshalAuthorizedKey(cert.Key)), "\n") | ||
| gotHostKey := credz.GetConfiguredHostKey(t, dut, "ssh-ed25519") | ||
| gotHostKey := credz.GetConfiguredHostKey(t, dut, "ssh-ed25519", devFqdn) |
There was a problem hiding this comment.
There appears to be an inconsistency between the certificate principal and the hostname used for connection. The host certificate is created with the principal dut.test.com (hardcoded in credz.CreateHostCertificate), but GetConfiguredHostKey is called with an FQDN that results in connecting to <dut.Name()>.net.google.com. For a robust test of SSH certificate authentication, the connection hostname should match a principal in the certificate. This might be working due to ssh-keyscan not strictly validating the hostname, but it makes the test brittle. It would be better to use a consistent hostname for both creating the certificate and for ssh-keyscan.
feature/gnsi/credentialz/tests/host_certificates/host_certificates_test.go
Outdated
Show resolved
Hide resolved
| // keys. | ||
| func GetConfiguredHostKey(t *testing.T, dut *ondatra.DUTDevice, algo string) string { | ||
| func GetConfiguredHostKey(t *testing.T, dut *ondatra.DUTDevice, algo string, fqdn string) string { | ||
| dutTarget := fmt.Sprintf("%s.%s", dut.Name(), fqdn) |
There was a problem hiding this comment.
The current logic fmt.Sprintf("%s.%s", dut.Name(), fqdn) will append a trailing dot to dut.Name() if fqdn is an empty string. This could lead to unexpected behavior or connection failures, and differs from the original behavior where just dut.Name() would be used. It would be more robust to only append the FQDN when it's provided.
For example, you could replace this line with logic like this:
dutTarget := dut.Name()
if fqdn != "" {
dutTarget = fmt.Sprintf("%s.%s", dut.Name(), fqdn)
}
Pull Request Test Coverage Report for Build 18979385340Details
💛 - Coveralls |
singhavnish2516
left a comment
singhavnish2516
left a comment
There was a problem hiding this comment.
LGTM after the latest commit.
* fix host_cert test for Arista * added args --------- Co-authored-by: Avnish Singh <singhavnish@google.com>
* fix host_cert test for Arista * added args --------- Co-authored-by: Avnish Singh <singhavnish@google.com>
5 participants
Fixed the host_certificate test by just adding the fqdn