After some discussions, we decided that the right way to fix this is to rewrite the helper functionality moving it into another helper (and possibly aligning it with IPv4's one).

Created attachment 1621 [details]
New interfces

These are the new (tentative) interfaces to setup the default routes.
Rationale:
1) They're not belongs to InterfaceContainer. Illogical placement
2) They should serve the (limited) scope of setting a default route for the Static routing.
3) Any more esoteric setup (such as a backup route) can be done through the StaticRouting API.

If we agree on those, I can do a similar thing for IPv4's helpers.

Created attachment 1622 [details]
Better APIs

Fixed some API clumsyness and updated the example so to comply with the new APIs

(In reply to comment #2)
> Created attachment 1621 [details]
> New interfces
> 
> These are the new (tentative) interfaces to setup the default routes.
> Rationale:
> 1) They're not belongs to InterfaceContainer. Illogical placement
> 2) They should serve the (limited) scope of setting a default route for the
> Static routing.
> 3) Any more esoteric setup (such as a backup route) can be done through the
> StaticRouting API.
> 
> If we agree on those, I can do a similar thing for IPv4's helpers.


We have this for IPv4 already
void
Ipv4StaticRouting::SetDefaultRoute (Ipv4Address nextHop,
                                    uint32_t interface,
                                    uint32_t metric)

and for IPv6
  void SetDefaultRoute (Ipv6Address nextHop, uint32_t interface, Ipv6Address prefixToUse = Ipv6Address ("::"), uint32_t metric = 0);

I believe that fine-grained control on setting this can be done by exposing these static routing objects to the user, so I think we are just discussing now how to automate the most common operations that just pick as many defaults as possible.

I was thinking about this API in your example:
+  staticRoutes.SetDefaultRoute (iic1, 1);

it is not very readable as to what action is being done to what node.  I also have had difficulty keeping track of the various container indices, in my IPv4 ns-3 experience.

I think that perhaps a common operation will be to assign a global prefix to a link, and then point *all* hosts on the link to use one of the addresses as a default.  That is, one is deemed a router, and the others are hosts to that router.  Here, it might not be illogical to put it in the interface container.  Something like:

/*
 * For each interface in the container, check that 'addr' is on-link
 * for that interface.  If so, assign a default static route to 'addr'
 * through that interface.  If not, abort program execution with an error
 * message.
 *
 * If 'addr' is already assigned to that interface, skip (do not install
 * default route to self)
 *
 */
Ipv6InterfaceContainer::AssignDefaultRoutes (Ipv6Address addr)
{
  // After sanity checking the input, call into Ipv6StaticRouting::SetDefaultRoute() for each node affected...
}


usage:

   ipv6.SetBase (Ipv6Address ("2001:1::"), Ipv6Prefix (64));
   Ipv6InterfaceContainer iic1 = ipv6.Assign (ndc1);
   iic1.SetDefaultRoute (Ipv6Address("2001:1::1"));

will that cover most initial use cases?


p.s. For radvd use cases, we will skip this and let radvd handle this, right?

It does make sense, and it basically goes back to the original design (i.e., to have the functionality in the InterfaceContainer Helper.

The pros is that it does make sense. The cons is that it doesn't "force" the user to understand that he/she is for real using a StaticRouting.

As for the most used case, I do agree that usually an user just wants to add a default router to the network. So the UI makes sense.

The main problem is with the "two routers (or more) in a network" case. The functions should be robust on creating loops, as we can easily skip adding a default router to the router itself, but it's nearly impossible to track the following calls (i.e., where the user will add the 2nd and 3rd routers).

This is the bug I was trying to fix at start... and it's still there no matter the changes. I guess the only thing to do is:
1) A bit of cleanup to the actual helper (leaving it in the InterfaceContainer),
2) Document it better
3) Write that the call is friggin' dangerous when you have two routers in the same network
4) Separate the "SetRouter" and SetDefaultRouter" things.

SetRouter should just do what it says, as is enable the Ipv6Forwarding, and have a SetHost counterpart (which isn't really necessary, but it's easier for the n00b).

SetRouter and SetHost aren't really necessary, as they're just wrappers to the Ipv6 attribute, however it does make sense to have the, again, for the n00b.

I'll try again a new patch later today (aka, tonight).

(In reply to comment #5)
> It does make sense, and it basically goes back to the original design (i.e., to
> have the functionality in the InterfaceContainer Helper.
> 
> The pros is that it does make sense. The cons is that it doesn't "force" the
> user to understand that he/she is for real using a StaticRouting.
> 
> As for the most used case, I do agree that usually an user just wants to add a
> default router to the network. So the UI makes sense.
> 
> The main problem is with the "two routers (or more) in a network" case. The
> functions should be robust on creating loops, as we can easily skip adding a
> default router to the router itself, but it's nearly impossible to track the
> following calls (i.e., where the user will add the 2nd and 3rd routers).

I don't see how it will lead to loops in typical usage.  I think it is possible to use the API to set loops with multiple calls, but would we be documenting or recommending that way? 

> 
> This is the bug I was trying to fix at start... and it's still there no matter
> the changes. 

"Two routers on the same network, one should be the default, the other shouldn't".  

possible solutions:  

1) exclude the second router from the interface container that is passed to the method I proposed.
2) use the same container, but the node that is the second router, remove its default route as a second step

it still probably doesn't lead to loops unless the default router points back to the secondary router somehow

Perhaps I do not understand the use case of concern clearly.


> I guess the only thing to do is:
> 1) A bit of cleanup to the actual helper (leaving it in the
> InterfaceContainer),
> 2) Document it better
> 3) Write that the call is friggin' dangerous when you have two routers in the
> same network
> 4) Separate the "SetRouter" and SetDefaultRouter" things.
> 
> SetRouter should just do what it says, as is enable the Ipv6Forwarding, and
> have a SetHost counterpart (which isn't really necessary, but it's easier for
> the n00b).
> 
> SetRouter and SetHost aren't really necessary, as they're just wrappers to the
> Ipv6 attribute, however it does make sense to have the, again, for the n00b.
> 
> I'll try again a new patch later today (aka, tonight).

SetRouter perhaps should be "EnableForwarding"; it is something completely different from SetDefaultRouter (which can be applied both to a router or a host).

Created attachment 1623 [details]
New proposed changes

This time I went for the minimal changes.
1) SetRouter is deprecated
2) SetForwarding is now a sort of wrapper around the IPv6 attribute
3) There is a new "SetDefaultRouteInAllNodes"

The "old" behaviour of SetRouter is accomplished by SetForwarding + SetDefaultRouteInAllNodes, giving more control over what is going to be a default router (thus installing a default route on the "other" nodes in the InterfaceContainer) and what a "normal" router.

(In reply to comment #7)
> Created attachment 1623 [details]
> New proposed changes
> 
> This time I went for the minimal changes.
> 1) SetRouter is deprecated
> 2) SetForwarding is now a sort of wrapper around the IPv6 attribute
> 3) There is a new "SetDefaultRouteInAllNodes"
> 
> The "old" behaviour of SetRouter is accomplished by SetForwarding +
> SetDefaultRouteInAllNodes, giving more control over what is going to be a
> default router (thus installing a default route on the "other" nodes in the
> InterfaceContainer) and what a "normal" router.

I found some guidance here:
http://www.telecom.otago.ac.nz/tele301/student_html/v6bootcamp-enable-disable.html

should we adopt this convention in ns-3?

forwarding - BOOLEAN
Configure interface-specific Host/Router behaviour.

Note: It is recommended to have the same setting on all
interfaces; mixed router/host scenarios are rather uncommon.

FALSE:

By default, Host behaviour is assumed.  This means:

1. IsRouter flag is not set in Neighbour Advertisements.
2. Router Solicitations are being sent when necessary.
3. If accept_ra is TRUE (default), accept Router
   Advertisements (and do autoconfiguration).
4. If accept_redirects is TRUE (default), accept Redirects.

TRUE:

If local forwarding is enabled, Router behaviour is assumed.
This means exactly the reverse from the above:

1. IsRouter flag is set in Neighbour Advertisements.
2. Router Solicitations are not sent.
3. Router Advertisements are ignored.
4. Redirects are ignored.

Default: FALSE if global forwarding is disabled (default),
 otherwise TRUE.

-----

I think there are two issues mixed in this patch; it might be easier to separate them into two patches:  1) setting the forwarding state of an interface and 2) setting default route on a node.

With regard to 1), if we adopt the above policy, do we need a node-scoped "forwarding" flag as in Linux, or do we just use the logical OR of all IPv6 interface forwarding flags to determine whether a node is forwarding or not?   If we have a node-scoped forwarding flag, the interaction between it and the interface-scoped flag needs to be defined.

The API you propose for 1) 
+  void SetForwarding (uint32_t i, bool state);
looks OK for the common use case (in which you select one router on a link).  Perhaps a "nice to have" API would allow you to specify a node index as an alternative (if you don't want to have to keep track of which interface index in the container matches which node).

With regard to 2), 
+  void SetDefaultRouteInAllNodes (uint32_t router);
this implementation detail looks fragile:
+  /* assume first global address is index 1 (0 is link-local) */
shouldn't it sanity check that the prefixes are correct?

also, this method and doxygen do not clearly imply that default router is not set in the 'router' specified.  

also, I have similar comment about the API; it might be nice to also have API that allows you to specify the node index as the parameter, or else specify the exact IPv6 address that is the default route; this might make scripts more readable.

(In reply to comment #8)
> (In reply to comment #7)
> > Created attachment 1623 [details]
> > New proposed changes
> > 
> > This time I went for the minimal changes.
> > 1) SetRouter is deprecated
> > 2) SetForwarding is now a sort of wrapper around the IPv6 attribute
> > 3) There is a new "SetDefaultRouteInAllNodes"
> > 
> > The "old" behaviour of SetRouter is accomplished by SetForwarding +
> > SetDefaultRouteInAllNodes, giving more control over what is going to be a
> > default router (thus installing a default route on the "other" nodes in the
> > InterfaceContainer) and what a "normal" router.
> 
> I found some guidance here:
> http://www.telecom.otago.ac.nz/tele301/student_html/v6bootcamp-enable-disable.html
> 
> should we adopt this convention in ns-3?
> 
> forwarding - BOOLEAN
> Configure interface-specific Host/Router behaviour.
> 
> Note: It is recommended to have the same setting on all
> interfaces; mixed router/host scenarios are rather uncommon.
> 
> FALSE:
> 
> By default, Host behaviour is assumed.  This means:
> 
> 1. IsRouter flag is not set in Neighbour Advertisements.
> 2. Router Solicitations are being sent when necessary.
> 3. If accept_ra is TRUE (default), accept Router
>    Advertisements (and do autoconfiguration).
> 4. If accept_redirects is TRUE (default), accept Redirects.
> 
> TRUE:
> 
> If local forwarding is enabled, Router behaviour is assumed.
> This means exactly the reverse from the above:
> 
> 1. IsRouter flag is set in Neighbour Advertisements.
> 2. Router Solicitations are not sent.
> 3. Router Advertisements are ignored.
> 4. Redirects are ignored.
> 
> Default: FALSE if global forwarding is disabled (default),
>  otherwise TRUE.
> 
> -----

Yes, the description is basically correct (it misses some esoteric settings, but it's ok).
 
> I think there are two issues mixed in this patch; it might be easier to
> separate them into two patches:  1) setting the forwarding state of an
> interface and 2) setting default route on a node.

Yes, they're two and they're (sadly) intertwined. This is because the old "SetRouter" helper function did BOTH operations. Hence the headaches.
We can separate the patches, but we'll need to apply them together anyway.

> With regard to 1), if we adopt the above policy, do we need a node-scoped
> "forwarding" flag as in Linux, or do we just use the logical OR of all IPv6
> interface forwarding flags to determine whether a node is forwarding or not?  
> If we have a node-scoped forwarding flag, the interaction between it and the
> interface-scoped flag needs to be defined.

We have both node-scope and interface-scope variables. At present the node-scope rule them all (changing it will change all the interface ones accordingly).
We can set the interface-specific flags as well, but it's not that easy/documented. Plus I'd have to triple check the various calls, so to be sure about what's checked and where.
That's the next task I'll do, after this one if closed.

> The API you propose for 1) 
> +  void SetForwarding (uint32_t i, bool state);
> looks OK for the common use case (in which you select one router on a link). 
> Perhaps a "nice to have" API would allow you to specify a node index as an
> alternative (if you don't want to have to keep track of which interface index
> in the container matches which node).

True. However that's would belong (probably) to a different Helper...
The function could look like:
"for all the nodes in a container (except the router itself) -> Set the default router to XXX".

This seems something yelling to be placed in the NodeContainer... but that's a definitive NO (NodeContainer is in /src/network).
Another useful place could be in the InternetStackHelper, however the name must be different (SetDefaultIpv6Route), as InternetStackHelper is also managing Ipv4.

> With regard to 2), 
> +  void SetDefaultRouteInAllNodes (uint32_t router);
> this implementation detail looks fragile:
> +  /* assume first global address is index 1 (0 is link-local) */
> shouldn't it sanity check that the prefixes are correct?

Good idea. It's kinda strange to have the link-local address elsewhere, but I'll do.

> also, this method and doxygen do not clearly imply that default router is not
> set in the 'router' specified.  

Hem... I doubt someone would like to have a router having itself as the default router. I'll point it out tho. 

> also, I have similar comment about the API; it might be nice to also have API
> that allows you to specify the node index as the parameter, or else specify the
> exact IPv6 address that is the default route; this might make scripts more
> readable.

Explicit address: that's easy. I'll do. However beware about the readability. You have to use a link-local address (fe80::etc), and that's hardly "readable". It doesn't tell you the network it belongs to.

About the node index, I have the same issue above mentioned. Which is the right Helper for this ?

Created attachment 1625 [details]
Another try...

New changes.
Improvements are:

-  Ipv6Address GetLinkLocalAddress (uint32_t i);
-  Ipv6Address GetLinkLocalAddress (Ipv6Address address);
Both functions will return the LinkLocal address for an node with an interface in the InterfaceContainer.

- void SetDefaultRoute (uint32_t i, Ipv6Address routerAddr);
- void SetDefaultRouteInAllNodes (Ipv6Address routerAddr);
Same as the old functions, but they do use an address to match the router.
PLUS: the address might be a global one as well, it will be silently converted in the proper LinkLocal Address (magic).

The assumption that the link-local address is in the 0th position in an Interface is gone, EXCEPT from Ipv6InterfaceContainer::SetRouter (uint32_t i, bool router), which is deprecated anyway (and I'd like very much to simply delete it).

Things left out from this patch (voluntarily):
1) further improvements to the forwarding node global and per-interface status.
This have to be further analysed. The patches here are only necessary to make sure that a router can be easily put in forwarding mode.
2) Use a node index suggestion: not going to do it. The node index is the very same in the InterfaceContainer and the corresponding NodeContainer. Thus it is not necessary.

Possible objections (to prevent them I'll list 'em here).
I do not want to automatically set the forwarding state of the router to true in a SetDefaultRouter-like function.
Rationale: the user shall be forced to know what he/she is doing. The examples (and the manual I'm writing) shows it very well.
Plus, there are cases where the forwarding state have to be forced to some uncommon cases. E.g., in a common home router the global node state is forwarding, the "internal" interfaces are in forwarding BUT the external interface is in "mixed mode", as it does have to acquire a dynamic IP address BUT it shall forward packets.
This is not yet supported by ns-3 and setting the state in the SetDefaultRouter-like function will cause huge headaches when (and if) I'll try to fix this use-case.
Hence, better be prepared for the changes.

(In reply to comment #10)
> Created attachment 1625 [details]
> Another try...
> 
> New changes.
> Improvements are:
> 
> -  Ipv6Address GetLinkLocalAddress (uint32_t i);
> -  Ipv6Address GetLinkLocalAddress (Ipv6Address address);
> Both functions will return the LinkLocal address for an node with an interface
> in the InterfaceContainer.
> 
> - void SetDefaultRoute (uint32_t i, Ipv6Address routerAddr);
> - void SetDefaultRouteInAllNodes (Ipv6Address routerAddr);
> Same as the old functions, but they do use an address to match the router.
> PLUS: the address might be a global one as well, it will be silently converted
> in the proper LinkLocal Address (magic).
> 
> The assumption that the link-local address is in the 0th position in an
> Interface is gone, EXCEPT from Ipv6InterfaceContainer::SetRouter (uint32_t i,
> bool router), which is deprecated anyway (and I'd like very much to simply
> delete it).
> 
> Things left out from this patch (voluntarily):
> 1) further improvements to the forwarding node global and per-interface status.
> This have to be further analysed. The patches here are only necessary to make
> sure that a router can be easily put in forwarding mode.
> 2) Use a node index suggestion: not going to do it. The node index is the very
> same in the InterfaceContainer and the corresponding NodeContainer. Thus it is
> not necessary.
> 
> Possible objections (to prevent them I'll list 'em here).
> I do not want to automatically set the forwarding state of the router to true
> in a SetDefaultRouter-like function.
> Rationale: the user shall be forced to know what he/she is doing. The examples
> (and the manual I'm writing) shows it very well.
> Plus, there are cases where the forwarding state have to be forced to some
> uncommon cases. E.g., in a common home router the global node state is
> forwarding, the "internal" interfaces are in forwarding BUT the external
> interface is in "mixed mode", as it does have to acquire a dynamic IP address
> BUT it shall forward packets.
> This is not yet supported by ns-3 and setting the state in the
> SetDefaultRouter-like function will cause huge headaches when (and if) I'll try
> to fix this use-case.
> Hence, better be prepared for the changes.

In general, I agree with the policy choices).  

- Please add NS_DEPRECATED to the SetRouter() method

- in SetDefaultRouterForAllNodes(Ipv6Address routerAddr), can you instead use 'routerAddr' as the default?  I find it a bit unusual to have the method instead use the link-local address instead of the address specified.

(In reply to comment #11)
> In general, I agree with the policy choices).  
> 
> - Please add NS_DEPRECATED to the SetRouter() method

It is :)
 
> - in SetDefaultRouterForAllNodes(Ipv6Address routerAddr), can you instead use
> 'routerAddr' as the default?  I find it a bit unusual to have the method
> instead use the link-local address instead of the address specified.

I triple checked, and it seems that, indeed, it IS possible to set the default gateway address to a global one.
However this is only to be done for debugging purposes and/or in specific cases (e.g., it could be useful in manually-configured networks).
On the other hand using the global address is largely discouraged in all the automatic address assignments, both DHCP and radvd-based.
The logic is (again) the one multiply stated: a default gateway must be on the same "network" as the host, so the link-local address shall be used.

Said so, probably I've been a bit too strict in enforcing this. So I'll remove the assert from the relevant functions. It is now a simple NS_LOG_WARN.

The behaviour of the functions is changed in this way:
void Ipv6InterfaceContainer::SetDefaultRouteInAllNodes (uint32_t router)
  still searches for a link-local address and complains if none is found.
void Ipv6InterfaceContainer::SetDefaultRouteInAllNodes (Ipv6Address routerAddress)
  use the given address without further checks. The user can (and should) use the "GetLinkLocalAddress" functions to get the right address. If they still want to use the global one. be my guest.

Cheers,

T.

Created attachment 1654 [details]
Latest version

Fixed in 10131:0dc090fc749d