-
Notifications
You must be signed in to change notification settings - Fork 263
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
fix bug in Prefix factory #3318
Conversation
nautobot/ipam/factory.py
Outdated
# faker ipv6 provider generates networks with /0 cidr, change to anything but /0 | ||
ipv6_fixed = factory.LazyAttribute( | ||
lambda o: o.ipv6_cidr.replace("/0", f"/{UniqueFaker('pyint', min_length=1, max_length=128)!s}") | ||
lambda o: o.ipv6_cidr.replace("/0", f"/{faker.Faker().pyint(min_value=1, max_value=128)!s}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, after diving into the faker
code, it looks like the ipv4
faker explicitly excludes a number of reserved address ranges, with the net result that the largest possible IPv4 network it will ever generate is a /3 such as 32./3, 64./3, or 128./3. The ipv6
faker has no such exclusions (though honestly it probably should), but I wonder if we should restrict min_value
here to something like 8 just to avoid creating unrealistically large IPv6 subnets that cover the entire address space? (Unless you think we should work on a PR to faker
instead... :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also actually since ipv6_cidr
is a network already, maybe min_value
should be ipaddress.IPv6Network(ipv6_cidr).prefixlen
so we only ever generate subnets of the original network rather than potentially creating supernets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also actually since
ipv6_cidr
is a network already, maybemin_value
should beipaddress.IPv6Network(ipv6_cidr).prefixlen
so we only ever generate subnets of the original network rather than potentially creating supernets?
This is only replacing /0
which is only valid for ::/0
so anything >0 will be a subset of the original network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixing a bug we were encountering where it wasn't actually returning an integer. The collision chance here should be around 128*128 so not perfect but probably not worth refactoring right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying my confusion! So now we have a 1/128 chance of getting something from ::/1
to ::/128
right? I wonder if we could/should instead just reject /0
entirely and "reroll" ipv6_cidr
until we get something that isn't /0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying my confusion! So now we have a 1/128 chance of getting something from
::/1
to::/128
right? I wonder if we could/should instead just reject/0
entirely and "reroll"ipv6_cidr
until we get something that isn't /0?
I added a custom faker provider in 5202e0b to fix the problem in the default ipv6 faker provider and also allow it to use integers below the ipv4 max range (2**32
). I'm not sure what that was doing but it didn't seem necessary.
Closes: #DNE
What's Changed
UniqueFaker('pyint', min_length=1, max_length=128)
was the class name instead of the actual value. In this context we need to call faker directly instead of using the lazyUniqueFaker
(also, uniqueness wasn't actually needed here)ipv6_cidr
to unique to prevent duplicate prefix errorsTODO