Hi Sam,
On Wed, Nov 13, 2024 at 12:43:00PM -0800, Sam Edwards wrote:
> > +# If the mngtmpaddr or tempaddr missing, return 0 and stop waiting
> > +check_tempaddr_exists()
> > +{
> > + local start=${1-"1"}
> > + addr_list=$(ip -j -n $testns addr show dev ${devdummy})
> > + for i in $(seq $start 4); do
> > + if ! echo ${addr_list} | \
> > + jq -r '.[].addr_info[] | select(.mngtmpaddr == true) | .local' | \
> > + grep -q "200${i}"; then
> > + check_err $? "No mngtmpaddr 200${i}:db8::1"
> > + return 0
> > + fi
> > +
> > + if ! echo ${addr_list} | \
> > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
> > + grep -q "200${i}"; then
> > + check_err $? "No tempaddr for 200${i}:db8::1"
> > + return 0
> > + fi
> > + done
> > + return 1
> > +}
>
> The variant of this function that I implemented is a lot less "fixed"
> and gathers all IPv6 prefixes (by /64) into one of 3 sets:
> 1. mngtmpaddr
> 2. temporary, not deprecated
> 3. temporary (whether deprecated or not)
>
> It then ensures that set 3 is a subset of set 1, and set 1 is a subset
> of set 2. (And if it's easy: it should also ensure that no 'temporary'
> has a *_lft in excess of its parent's.)
I'm not totally get your explanation here. e.g. with preferred_lft 10,
valid_lft 30. I got the following result.
# ip addr show dummy0
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 2e:f7:df:87:44:64 brd ff:ff:ff:ff:ff:ff
inet6 2001::743:ec1e:5c19:404f/64 scope global temporary dynamic
valid_lft 25sec preferred_lft 5sec
inet6 2001::938f:432:f32d:602f/64 scope global temporary dynamic
valid_lft 19sec preferred_lft 0sec
inet6 2001::5b65:c0a3:cd8c:edf8/64 scope global temporary deprecated dynamic
valid_lft 3sec preferred_lft 0sec
inet6 2001::8a7e:6e8d:83f1:9ea0/64 scope global temporary deprecated dynamic
valid_lft 0sec preferred_lft 0sec
inet6 2001::1/64 scope global mngtmpaddr
valid_lft forever preferred_lft forever
So there are 1 mngtmpaddr, 2 temporary address (not deprecated). 4 total
temporary address. Based on your rule. It should be set 1 is a subset of
set 2. Set 2 is a subset of 3.
And how do we ensure that no 'temporary' has a *_lft in excess of its parent's.
> Doing it this way allows the test case to create, modify, and delete
> mngtmpaddrs according to the needs of the test, and the check()
> function only ensures that the rules are being obeyed, it doesn't make
> assumptions about the expected state of the addresses.
I'm not sure if this is totally enough. What if there are 3 mngtmpaddrs
and 4 temporary address. But actually 1 mngtmpaddrs doesn't have temporary
address. Maybe check() needs to check only 1 prefix each time.
> > +
> > +kci_test_mngtmpaddr()
> > +{
> > + local ret=0
> > +
> > + setup_ns testns
> > + if [ $? -ne 0 ]; then
> > + end_test "SKIP mngtmpaddr tests: cannot add net namespace $testns"
> > + return $ksft_skip
> > + fi
> > +
> > + # 1. Create a dummy Ethernet interface
> > + run_cmd ip -n $testns link add ${devdummy} type dummy
> > + run_cmd ip -n $testns link set ${devdummy} up
> > + run_cmd ip netns exec $testns sysctl -w net.ipv6.conf.${devdummy}.use_tempaddr=1
>
> Test should also set .temp_prefered_lft and .temp_valid_lft here.
>
> I also set .max_desync_factor=0 because this is a dummy interface that
> doesn't have any latency, which allows the prefer lifetime to be
> pretty short. (See below.)
Thanks, I will fix the test.
>
> > + # 2. Create several (3-4) mngtmpaddr addresses on that interface.
> > + # with temp_*_lft configured to be pretty short (10 and 35 seconds
> > + # for prefer/valid respectively)
> > + for i in $(seq 1 4); do
> > + run_cmd ip -n $testns addr add 200${i}:db8::1/64 dev ${devdummy} mngtmpaddr
>
> I don't really like using 200X:db8::1 as the test addresses.
> 2001:db8::/32 is the IANA designated prefix for examples/documentation
> (and, by extension, unit tests) so we should really try to remain
> inside that.
>
> Personally, I tend to use 2001:db8:7e57:X::/64 ("test" in leetspeak)
> just to minimize the chances of conflicting with something else in the
> system. Though, with the test happening in its own netns, *that* level
> of caution may not be necessary.
>
> Still, 2001:db8::/32 is what IPv6 folks expect, so I'd want to stay in there.
OK, I will use 2001:db8::/32 for testing.
>
> > + tempaddr=$(ip -j -n $testns addr show dev ${devdummy} | \
> > + jq -r '.[].addr_info[] | select(.temporary == true) | .local' | \
> > + grep 200${i})
> > + #3. Confirm that temporary addresses are created immediately.
>
> This could simply be a call to the above genericized check() function.
>
> > + if [ -z $tempaddr ]; then
> > + check_err 1 "no tempaddr created for 200${i}:db8::1"
> > + else
> > + run_cmd ip -n $testns addr change $tempaddr dev ${devdummy} \
> > + preferred_lft 10 valid_lft 35
>
> While Linux is (apparently) happy to let userspace modify the
> tempaddr's remaining lifetime like this, I don't think this is a
> common or recommended practice. Rather, the test should be letting
> Linux manage the tempaddr lifetimes and rotate the addresses itself.
OK
>
> > + fi
> > + done
>
> Here is a good place to create an address that *isn't* mngtmpaddr,
> confirm there is no temporary (via call to check() function), then add
> the `mngtmpaddr` flag after the fact.
OK, I will
>
> > +
> > + #4. Confirm that a preferred temporary address exists for each mngtmpaddr
> > + # address at all times, polling once per second for at least 5 minutes.
> > + slowwait 300 check_tempaddr_exists
>
> So I previously said "wait 5 minutes" but I later saw in the
> documentation for the selftest suite that maintainers really don't
> like it when a test takes more than ~45 seconds to run. We might want
> to drop this wait to 30 by default and accelerate the timetable on
> prefer/valid lifetimes to something like 10/25.
Yes, 5m is too long for a single test.
> > +
> > + end_test "PASS: mngtmpaddr add/remove correctly"
> > + ip netns del "$testns"
>
> Do we need to make sure the netns gets cleaned up via `trap ... EXIT`
> so that it doesn't leak if the user interrupts the test? Or does the
> greater test fixture take care of that for us?
No, rtnetlink.sh doesn't have a trap function. I plan to add the trap
function separately.
Thanks
Hangbin
