Skip to content

Split gRPC converter into multiple files and remove convert_* functions #559

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

Merged
merged 12 commits into from
Jun 6, 2025

Conversation

mvachhar
Copy link
Contributor

@mvachhar mvachhar commented Jun 6, 2025

This PR:

  • Splits the gRPC converter into multiple files
  • Removes almost all of the convert_* functions in favor of TryFrom
    • The one convert that remains is the top level function because it adds defaults. We probably want to remove this at some point but I want to limit the blast radius of these changes.
    • The VRF commit still uses the first vrf as default if there is no VRF labeled default - this is broken IMHO but I don't want to break things for what is currently minor benefit. Plus the fuzzer in gateway-proto will have to be fixed to rememdy this issue.
  • Minor cleanups, elimination of constants, use of checked types, etc.
    • These are rolled into the file move commits since the problems were observed during the move
  • Adds parsing for InterfaceAddress types
    • Prefix is not the same as InterfaceAddress even though both look like <ip addr>/<mask> as Prefix zeros the low order bits, InterfaceAddress does not. It may be the case that we should error parsing a Prefix that does not already have the low order bits as 0 and error if the InterfaceAddress does have the low order bits as 0 (except for /31 and /127 masks) but I do not do that now as to limit the blast radius these changes.
    • This distinction also revealed a bug in the value generators used for fuzzing so I will fix that next.

Fixes #549

mvachhar added 8 commits June 5, 2025 22:22
Move BGP functions to their own file. Also eliminate redundant
convert_ functions and just use TryFrom.

Signed-off-by: Manish Vachharajani <[email protected]>
Display was being used for CLI output.  This creates a wrapper
for that use case and implements a FromStr and Display that
do the short form (so FromStr can parse Display)

Signed-off-by: Manish Vachharajani <[email protected]>
This commit also uses a checked type for interface addresses

Signed-off-by: Manish Vachharajani <[email protected]>
@mvachhar mvachhar requested review from qmonnet and sergeymatov June 6, 2025 13:52
@mvachhar mvachhar requested a review from a team as a code owner June 6, 2025 13:52
@mvachhar mvachhar force-pushed the pr/mvachhar/mgmt-cleanup branch from 3fead95 to 6645af7 Compare June 6, 2025 13:53
@mvachhar mvachhar force-pushed the pr/mvachhar/mgmt-cleanup branch from 6645af7 to b13e054 Compare June 6, 2025 13:54
@mvachhar mvachhar self-assigned this Jun 6, 2025
@mvachhar mvachhar added this to the GW R1 milestone Jun 6, 2025
@mvachhar mvachhar added the clean-up Code base clean-up, no functional change label Jun 6, 2025
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Lots of things moving in this PR, I wish GitHub could show moving lines (diff.colorMoved in Git). I assumed these commits where you move stuff around are only moving stuff around. Looks good to me

@mvachhar mvachhar added this pull request to the merge queue Jun 6, 2025
Merged via the queue into main with commit 59f4c43 Jun 6, 2025
14 checks passed
@mvachhar mvachhar deleted the pr/mvachhar/mgmt-cleanup branch June 6, 2025 15:56
#[repr(transparent)]
pub struct RenderInterfaceAddress<'a>(pub &'a InterfaceAddress);

impl<'a> Display for RenderInterfaceAddress<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

InvalidMaskLength(String),
}

impl FromStr for InterfaceAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why is this needed for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up Code base clean-up, no functional change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finish dataplane mgmt refactor/cleanup
3 participants