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 infra_ipv6 attribute to devices #297

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

lunkwill42
Copy link
Collaborator

@lunkwill42 lunkwill42 commented Mar 31, 2023

This PR fixes #276.

I could not find any existing tests that explicitly test the existing infra_ip attribute, so I'm not sure what the best strategy for testing this would be.

The existing code assigns values to Device.infra_ip when init-ing fabric devices, using the cnaas_nms.devicehandler.underlay.find_free_infra_ip() function:

infra_ip = cnaas_nms.devicehandler.underlay.find_free_infra_ip(session)

Question: Should this PR concern itself with also assigning an IPv6 address to infra_ipv6? It seems the assignment is based on the infra_lo_net setting, so this may actually also need a new setting for infra_ipv6_lo_net. Setting the value could be made dependent on infra_ipv6_lo_net actually being present (i.e. "do nothing if IPv6 isn't used/configured")

This PR is dependent on #266 being merged first (actual changes start from ce1d4c3)

Valid IPv6 interface addresses must be accepted by the mgmtdomain API
endpoints. Additionally, an mgmtdomain can be either IPv4, IPv6 or both,
meaning that there are now multiple sets of minimally required
attributes to post to the endpoint.

This should ensure backwards compatibility with API clients that only
support ipv4_gw
The new version of this function can take the optional version argument
to select whether to get an address from the IPv4 or the IPv6 network of
the mgmtdomain.
This config setting will be used to choose which IP address version is
preferred as the primary management address of a device in a dual-stack
management domain.
This allows (but does not require) devices to be configured with
dual-stack management (even though CNaaS-NMS will continue to use the
primary management address).
This updates init_access_device_step1 to fetch, reserv and assign
secondary management IPs from any dual-stack Mgmtdomain.
Prefix length and mgmt_gw should be available variables also for the
secondary management address.
This repeats a lot of what `init_access_device_step1` does. Not sure
why there is an overlap, but I'm not going to change it without being
sure.
This also refactors redundant IP field value validation code. The logic
for all IP addr fields is the same, so no need for duplication.
This field is required for installations that want to run IPv6 directly
on the underlay.
Since this field is explicitly IPv6-only, it needs a separate check
from the other IPv6 fields.
@lunkwill42 lunkwill42 self-assigned this Mar 31, 2023
@lunkwill42 lunkwill42 added the enhancement New feature or request label Mar 31, 2023
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Patch coverage: 58.09% and project coverage change: -0.13 ⚠️

Comparison is base (8cbd098) 65.19% compared to head (8bf8078) 65.06%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #297      +/-   ##
===========================================
- Coverage    65.19%   65.06%   -0.13%     
===========================================
  Files           65       65              
  Lines         6950     7011      +61     
===========================================
+ Hits          4531     4562      +31     
- Misses        2419     2449      +30     
Impacted Files Coverage Δ
src/cnaas_nms/api/device.py 48.01% <ø> (ø)
src/cnaas_nms/api/mgmtdomain.py 43.50% <17.39%> (-3.91%) ⬇️
src/cnaas_nms/devicehandler/init_device.py 52.74% <47.36%> (-0.46%) ⬇️
src/cnaas_nms/db/device.py 72.99% <52.38%> (-0.62%) ⬇️
src/cnaas_nms/devicehandler/sync_devices.py 74.90% <60.00%> (-0.23%) ⬇️
src/cnaas_nms/app_settings.py 96.62% <87.50%> (-0.94%) ⬇️
src/cnaas_nms/db/mgmtdomain.py 91.01% <93.10%> (+0.24%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sonarcloud
Copy link

sonarcloud bot commented May 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.7% 0.7% Duplication

@indy-independence
Copy link
Member

Blocking PR #266 has been merged, so maybe time to look at this again
I'm not very familiar with the use-case for having IPv6 in the underlay network, so I'm also not sure if anyone would want dual-stack in the underlay but I'm guessing probably no? In that case maybe auto assigning if infra_ipv6_lo_net is set, and then probably not assign a v4 if infra_lo_net is not set?

@indy-independence
Copy link
Member

Main usecase for this is for SIKT if they want to "merge" in existing installations that is not running cnaas-nms into cnaas-nms in the future. Currently a single loopback interface in those two (?) installations are used both for underlay vxlan tunnels and management traffic. Vidar will look into the history and reasoning for this design, and if future designs will use the same setup or separate loopbacks for underlay and management. This might affect how we want to implement this with dualstack and pools etc. In meantime this PR will remain in kind of frozen state because SIKT has no current need or time to work on it?

@lunkwill42
Copy link
Collaborator Author

In meantime this PR will remain in kind of frozen state because SIKT has no current need or time to work on it?

Sounds about right for the time being.

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.

Possibility to add infra_ip6 to devices
2 participants