Time to remove LISP, overlay, and underlay docs and names #3516
Conversation
eriknordmark
requested review from
deitch,
milan-zededa and
rouming
as code owners
pkg/pillar/types/zedroutertypes.go
Outdated
| for i := range status.UnderlayNetworkList { | ||
| ul := &status.UnderlayNetworkList[i] | ||
| func (status AppNetworkStatus) GetULStatusForNI(netUUID uuid.UUID) []*AppNetAdapterStatus { | ||
| var uls []*AppNetAdapterStatus |
There was a problem hiding this comment.
I think we should also rename uls and ul variables (there are several occurrences of them across the code base)
There was a problem hiding this comment.
Perhaps uls->adapters, ul -> adapter?
There was a problem hiding this comment.
I used ana elsewhere but missed this. adapters makes sense here.
Do you think adapter is more clear elsewhere where I'm using ana?
There was a problem hiding this comment.
I think that I would prefer adapter, but ana is fine too.
There was a problem hiding this comment.
OK, I added those changes plus some other changes in three additional commits. If that looks OK I'll squash them into the commit with the underlay rename.
Also, I had a question about this use of underlay:
nireconciler/genericitems/radvd.go:# Low preference to allow underlay to have high preference default
nireconciler/genericitems/radvd.go:# TBD but preference is high. To which underlay does it refer?
Is this a comment which was copied from older code? If so then it refers to IPv6 LISP overlay. And I don't see code which sets a low preference. So maybe I should drop that comment?
There was a problem hiding this comment.
Yes, this was just copied and can be removed.
| DisplayName string | ||
| Activate bool | ||
| GetStatsIPAddr net.IP | ||
| AppNetAdapterList []AppNetAdapterConfig |
There was a problem hiding this comment.
What about using the plural AppNetAdapters and avoid List at the end?
There was a problem hiding this comment.
I didn't feel like changing those as part of the PR. And there is a question whether one can be more confused hence risk creating bugs due to the less visible single character "s" vs "List", or whether the compiler will catch such confusing since the types will not match.
|
I think we can go ahead with this - I will rebase #3513 and fix merge conflicts once this is merged. |
|
Note that some workflows are currently failing because of this issue: golang/go#63684 |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3516 +/- ##
=======================================
Coverage 20.51% 20.52%
=======================================
Files 209 208 -1
Lines 45391 45377 -14
=======================================
- Hits 9314 9313 -1
+ Misses 35397 35383 -14
- Partials 680 681 +1
☔ View full report in Codecov by Sentry. |
Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: eriknordmark <erik@zededa.com>
We have removed LISP and the default overlay, but the underlay remains. Replacing UnderlayNetwork* with AppNetAdapter*, and ul* with adapter*. Fixed some yetus complaints. Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: eriknordmark <erik@zededa.com>
A long time ago we had integrated LISP secure overlay network code, and as a result zedrouter had functions and variables named "underlay*" or "ul*".
I noticed we still have some text left in SECURITY.md so I did a pass over the code.
Note that there are several commits with different potential for merge conflicts with existing work; the "underlay" commit with "ul*" renames is the one most likely to cause such issues. Note that the Underlay struct is renamed to AppNetAdapter{Config,Status} but I'm open to other name suggestions.
The underlay term still remains for the geolocation code and API, but added comments explaining that this is the underlay/public IP address.
Let me know if this is a good time to do this cleanup or if it will cause conflicts with ongoing network work and should be deferred.