✨ Bring your own network #1472
✨ Bring your own network #1472
Conversation
There was a problem hiding this comment.
In general this approach seems good to me. Thanks a lot for this contribution!
@guettli @batistein what's your opinion?
There was a problem hiding this comment.
Thanks a lot again for this PR @johannesfrey. I think we can merge it if you follow the suggestions I gave. It's really good work!
|
|
||
| if network != nil { | ||
| if len(network.Subnets) > 1 { | ||
| return nil, fmt.Errorf("multiple subnets not allowed") |
There was a problem hiding this comment.
this should not be an error, but we rather should set an appropriate condition on the HetznerCluster object, create an event, and return nil here.
There was a problem hiding this comment.
I hope I understood you correctly here. I set the NetworkReadyCondition to false in that case. Also what do you mean with "create an event"?
There was a problem hiding this comment.
@johannesfrey didn't see your response here sorry. "record.Warnf()" would be appropriate here. that's actually something we should still add!
johannesfrey
force-pushed
the
bring-your-own-network
branch
from
3f6b8f6
to
b27b859
Compare
Sorry for the long delay 🙏 . Thx for the reviews! I hope I addressed your suggestions correctly. PTAL. Thx! |
syself-bot
bot
added
area/test
johannesfrey
force-pushed
the
bring-your-own-network
branch
from
b27b859
to
028f798
Compare
There was a problem hiding this comment.
thanks @johannesfrey ! I went another time over the details and found a few things.
| } | ||
| } else { | ||
| // If no ID is given check the other network settings for valid entries. | ||
| if r.Spec.HCloudNetwork.NetworkZone != nil { |
There was a problem hiding this comment.
what is supposed to happen if this is nil?
I just wanted to say that we should again avoid the nested if statements if possible and that we can do that here.
Currently, we don't handle the case where NetworkZone is nil at all. Shouldn't we though? Isn't that a problem?
Anyway, let's avoid the nested if statement and if you say we should, then let's handle the case where NetworkZone is nil
There was a problem hiding this comment.
I agree that we should probably append an error when NetworkZone is nil. But still I'm struggling to see how to really circumvent the nested statements because the method's idea seems to be to aggregate all errors that might occur instead of returning early. An early return would then potentially miss other validation errors that might happen afterwards. But probably I'm just missing a detail.
There was a problem hiding this comment.
I wanted to avoid the large code block for "NetworkZone != nil". However, I think you are right that this is not really possible, as we cannot just return at that point.
Therefore, I would say let's add a handling of "NetworkZone == nil" and otherwise don't change anything. Does that make sense to you?
| } | ||
| } | ||
|
|
||
| if r.Spec.HCloudNetwork.CIDRBlock != nil { |
There was a problem hiding this comment.
again here and below it should never be nil afaik. Should we return errors?
There was a problem hiding this comment.
Aligns with the comment above. Do you mean return with an error here? Because in this method we only aggregate and return at the bottom?!
There was a problem hiding this comment.
yeah I mean with "returning errors" to "add them". Sorry for being not precise
What this PR does / why we need it:
This PR makes it possible to "adopt" a pre-existing network by passing its ID to
hetznerCluster.spec.hcloudNetwork.idinstead of the network being created during cluster creation. Furthermore, during cluster deletion it only deletes the attached network if it does not have theownedlabel attached to it (currently the only way here to discriminate between a CAPH-managed network and an unmanaged one).Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #762
Special notes for your reviewer:
This has been lingering around for a while untouched on my fork and I decided to rebase it onto the current main branch. Please consider this as a first attempt to approach this topic as a whole. I also tried to already add some unit tests. I guess it also might require some e2e tests!? No idea if this is the desired way to do this and about other side-effects I did not see. So looking forward for feedback or any pointers. And also feel free to push changes to the PR, as I'll be pretty occupied with other things almost the whole September. Just wanted to push this out there already for you to take a look at 🙂
The most controversial changes so far:
hcloudNetwork.idmutually exclusive withcidrBlock,subnetCidrBlockandnetworkZonecidrBlock,subnetCidrBlockandnetworkZoneto be pointers (I guess this could also be done with empty strings, but pointers make it possible to be not shown at all, when not provided)Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs: