Overhaul of peer endpoint's IP #149
Conversation
| # If source IP and source-interface, ensure source-ip is defined on source-interface: | ||
| if (self.source_ip and self.source_interface) and ( | ||
| not self.source_interface.ip_addresses.filter(pk=self.source_ip.pk)): | ||
| raise ValidationError("Selected source IP is not assigned to the selected source interface") |
There was a problem hiding this comment.
Just checking - is it invalid to e.g. use a loopback IP as the source_ip but specify a physical source_interface?
There was a problem hiding this comment.
I think You are correct - if we allow for the logic defined in local_ip() :
- source_ip
- soure_interface
so that source_interface should not be validated for its IP address
| raise ValidationError("Can not set both IP and Update source options") | ||
| # If source IP and source-interface, ensure source-ip is defined on source-interface: | ||
| if (self.source_ip and self.source_interface) and ( | ||
| not self.source_interface.ip_addresses.filter(pk=self.source_ip.pk)): |
There was a problem hiding this comment.
Probably should add an .exists() here and in the similar logic below.
There was a problem hiding this comment.
I would say we should remove this check entirely as per above comment
| """ | ||
| for endpoint in self.endpoints.all(): | ||
| if not endpoint.local_ip: | ||
| raise ValidationError(f"Peer endpoint does not have a local IP") |
There was a problem hiding this comment.
Probably would be good to indicate which endpoint(s) specifically are failing?
| @@ -589,28 +598,66 @@ def to_csv(self): | |||
| self.peer, | |||
| ) | |||
|
|
|||
| ip = models.ForeignKey( # Computed IP | |||
There was a problem hiding this comment.
should the __str__ method be changed to use this instead of local_ip?
| on_delete=models.PROTECT, | ||
| blank=True, | ||
| null=True, | ||
| related_name="bgp_peer_endpoints", |
There was a problem hiding this comment.
This needs a different related_name than the source_ip related_name, doesn't it?
| to="ipam.IPAddress", | ||
| on_delete=models.PROTECT, | ||
| blank=True, | ||
| null=True, |
There was a problem hiding this comment.
Should this be non-nullable?
There was a problem hiding this comment.
Interesting - now I'm thinking it's possible and would be a very good validation for the data consistency
|
|
||
| # Priority 4: Source IP defined through source-interface on the PeerGroup | ||
| if interface_source_ip and inherited_source_interface: | ||
| return interface_source_ip |
There was a problem hiding this comment.
Do we need a final else case to either return None or raise an exception?
ipattribute on thePeerEndpointmodel. This attribute should always store the computed endpoint's IP Address.source_interface- performprimary_iplookup.peer_endpoint.source_ipsource_ipis set - implemented bypeer_endpoint.save()source_ipis removed - implemented bypeer_endpoint.save()source_ipis changed - implemented bypeer_endpoint.save()peer_group.source_ipsource_ipset on peergroup - send signal any time this setting changes on the parent peer_groupthis will only impact peer_endpoints if they had
peer_group.source_interfaceinheritance beforeimplemented through
handle_peergroup_updatessignalsource_ipremoved on peergroup - send signal any time peer endpoint is added to the peer groupimplemented through
handle_peergroup_updatessignalsource_ipchanged on the peergroup - send signal any time peer endpoint is removed from the peer groupimplemented through
handle_peergroup_updatessignalprotect
peer_group.endpoints.all()from becoming invalid - implementedpeer_endpoint.source_interfacesource_interfaceis set - implemented bypeer_endpoint.save()source_interfaceis removed - implemented bypeer_endpoint.save()source_interfaceis changed - implemented bypeer_endpoint.save()peer_group.source_interfacehandle_peergroup_updatespeer_group.endpoints.all()from becoming invalid - implementedpeer_group.clean()peerendpointadded to apeer_group- IP validated duringpeerendpoint.clean()peerendpointremoved from apeer_group- IP validated duringpeerendpoint.clean()source_ipbased onis_primaryonIPAddressAssignmentsource_ipbased onis_primaryonIPAddressAssignmentpeer_endpoint.ipvalues