Adding conditional passing of metadata to devices#5297
Adding conditional passing of metadata to devices#5297ram-mac wants to merge 3 commits intoopenconfig:mainfrom
Conversation
Summary of ChangesHello, 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 implements a necessary adjustment for Arista device compatibility within the gRPC communication layer. By introducing vendor-aware logic into the credential handling, the system can now conditionally suppress password metadata during mTLS sessions, ensuring that devices relying on certificate-based identity do not reject connections due to extraneous authentication headers. 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. Footnotes
|
![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 updates the gRPC credential logic to include vendor-specific handling, specifically for Arista devices. However, the current implementation contains several critical issues, including a compilation error due to an undeclared variable returnMap, typos in field and constant names, and hardcoded vendor logic that hinders multi-vendor support.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the gRPC credential handling in topologies/binding/binding.go by adding a vendor field to the creds struct and implementing a mechanism to omit passwords for Arista devices on secure connections. It also refactors GetRequestMetadata to respect existing context metadata and handle credential overrides. Feedback was provided regarding the hardcoding of the Arista vendor in dialOpts, which could break authentication for other vendors, and the need to distinguish between general TLS and mTLS when applying the password omission logic to ensure it only triggers in the intended scenarios.
| } | ||
| if bopts.Username != "" { | ||
| c := &creds{bopts.Username, bopts.Password, !bopts.Insecure} | ||
| c := &creds{bopts.Username, bopts.Password, opb.Device_ARISTA, !bopts.Insecure} |
There was a problem hiding this comment.
The device vendor is hardcoded to opb.Device_ARISTA here. This causes the Arista-specific logic in GetRequestMetadata (omitting the password during secure connections) to be applied to all devices, regardless of their actual vendor. This will break authentication for other vendors that require a password even when using TLS/mTLS. To fix this, dialOpts and makeDialer should be refactored to accept the opb.Device_Vendor as an argument. Keeping logic general rather than hardcoding vendor-specific constants promotes reusability and prevents side effects on other vendors.
References
- Constants that may be applicable to multiple vendors in the future should be kept general, even if they are currently used in a vendor-specific context, to promote reusability.
| vendor opb.Device_Vendor | ||
| secure bool |
There was a problem hiding this comment.
The secure field (set to !bopts.Insecure) is used for both RequireTransportSecurity() and the Arista password omission logic. However, !bopts.Insecure is true for any TLS connection (including SkipVerify), while the PR description indicates the password omission is specifically for mTLS scenarios.
To avoid incorrectly omitting the password on non-mTLS secure connections, consider adding a separate boolean field (e.g., isMTLS) to the creds struct, initialized from bopts.MutualTls, and use that field for the Arista-specific check in GetRequestMetadata.
| } | ||
| if bopts.Username != "" { | ||
| c := &creds{bopts.Username, bopts.Password, !bopts.Insecure} | ||
| c := &creds{bopts.Username, bopts.Password, opb.Device_ARISTA, !bopts.Insecure} |
3 participants
Arista devices expect metadata to have username, password even in mTLS scenario.
So, this change modifies GetRequestMetadata to conditionally omit the "password" from the returned metadata when using mTLS with Arista devices.
This change ensures that Arista devices, which extract the username from the certificate in mTLS mode, do not fail due to an unexpected password in the metadata.
The vendor is now passed to PassCred to enable this vendor-specific logic.