V4Config normalizes strings to types after resolving merged_v4_config

[kdienes]

If a layout passes V4Config(network='10.0.0.0') into a link, the string will get stored into merged_v4_config and not resolved to a IPv4Network.

I think we want IPv4 types resolved from strings at the earliest possible point; e.g., __post_init__()

[hartmans]

I agree that we want strings normalized to types at the earliest possible point, but I think __post_init__ is too early, exactly because of resolution. Consider something like

def find_network(): return '10.0.0.0/8'

n = NetworkLink(v4_config=V4Config(network=find_network))

The issue is that the find_network function is not a valid argument to IPv4Network, and so after_resolve (or __post_init__ if we move the code back there where it used to be before resolution) will fail.

I had some fun with the debugger. What's happening is that 5ca548a540 added a type annotation for merged_v4_config so dataclasses.dataclass will include it in repr. Unfortunately that means that NetworkLink.init picks up on it in the following code:

        for k in NetworkLink.__annotations__:
            if not hasattr(self, k):
                setattr(self, k, None)

And so init calls hasattr(self, 'merged_v4_config') And hasattr actually calls getattr under the covers. And so before resolution, we call merged_v4_config and memoize the result.

There are a few solutions here:

• drop the annotation and manually write repr
• In after_resolve clear merged_v4_config from the dict
• Call after_resolve on the merged result in the definition of merged_v4_config
• Specifically exclude merged_v4_config in NetworkLink.init

With one of the above possibly combined with code that detects merged_v4_config somehow getting called before after_resolve and raises an error.

[kdienes]

Of the three, I don't think calling after_resolve on the merged result will work, because the merged result might have unresolved content (like you noted).

Clearing merged_v4_config makes intuitive sense to me, since it's showing me what that field would have in it were I to access it right at the time of repr.

I agree that you can't fully normalize everything by __post_init__, but normalizing the things that we can (e.g. strings, I think) would detect bad config earlier and make it easier to trace down.

[hartmans]

Of the three, I don't think calling after_resolve on the merged result will work, because the merged result might have unresolved >content (like you noted). Yeah, but it will error out nicely if somehow you've gotten into a situation where things aren't resolved.

I agree that you can't fully normalize everything by post_init, but normalizing the things that we can (e.g. strings, I think) would >detect bad config earlier and make it easier to trace down.

That would be nice, wouldn't it? I tried to write code that tracked all the things that could be resolved and only gave errors as soon as possible. It twisted my brain into knots and I found the complexity of duplicating logic for all the things that resolve_deferred could touch all throughout the code too painful. In each function called by after_resolve, you need to track whether you have already called resolve_deferred, and thus shouldn't expect functions or injection keys, or whether you might still resolve in the future. Then at each validation step you need to also accept all the things that resolve_deffered handles specially. And I just couldn't find a way of doing that clealy, especially if resolve_deferred gains new functionality in the future.

[kdienes]

Yeah, but it will error out nicely if somehow you've gotten into a situation where things aren't resolved.

I'm assuming that will be true after your other fix, but not before?

I'm reading your second half as saying that you don't want to include an opportunistic canonicalization step for obvious things like "if isinstance(self.network, str): self.network=IPv4Network(self.network)" because you haven't found a good general way to do it, and because you think the error experience of finding it in after_resolve() is good enough that added special-casing isn't worth it.

If so, that seems valid (since after your fix it should be pretty easy to trace the bad value back to source) and I'll take it as a challenge to come up with something more general if I really want earlier validation.

[hartmans]

I certainly haven't found a general way to do it, and it's clear that the set of special cases are going to be user specific. As an example, I'd basically never set a network on a v4_config attached to a link, because that is typically set on the v4_config attached to the network. But clearly your usage patterns are different. So if I were going to do special cases they'd be for things like address not network.

I hope the error handling is going to be good enough if we error out at resolve time. It's not going to bring you to the source line of the add call in NetworkConfigModel, but we ought to be able to tell you which machine and which interface has the broken link. But this is a case where I did try to find a general way because I thought it would have made the error handling a bit better.

I'll fix this in the morning, because it's clearly quite broken now.

[kdienes]

I agree that I should have picked address as an example, and I think you're right about the error handling.