- Notifications
You must be signed in to change notification settings - Fork1k
chore: add configMaps component to tailnet#11400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
spikecurtis commentedJan 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
9704400 tod9c25c1Compareda115a4 to86113e2Compared9c25c1 to9af1797Compare86113e2 to8d938f7Compare3b7ebdf to032d52dCompareb03a760 to6f37b9bCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Definitely feels like a significant improvement over the previous mutex-guarded engine
Uh oh!
There was an error while loading.Please reload this page.
| return out | ||
| } | ||
| func (c *configMaps) setAddresses(ips []netip.Prefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Even though these won't be used outside of tailnet, I think it would be clearer to export the functions that callers are expected to use. Otherwise they feel a little lost next to all of the internal-only functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you think it's worth putting into its own package? Otherwise I don't think the internal/external distinction is meaningful and I'm not sure readers would will pick up on the difference.
Maybe adding some method comments would help make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I was going through, other than the configLoop() routine, I don't know if any internal/external distinction is going to pass the test of time. For example, right now the functions that return the current config are only called from the configLoop(), but there isn't any strong reason we couldn't call them from the Conn later---there are a lot of functions already that query various aspects of config---so long as they obey the locking requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That makes sense. I was initially a bit confused on what was intended to be called externally. Comments sound helpful 👍
6f37b9b to0ec3722Compare0ec3722 tod013264Compared013264 to018e3b8CompareMerge activity
|
Work in progress on a subcomponent of the Conn which will handle configuring the wireguard engine on changes. I've implemented setAddresses as the simplest case and added unit tests of the reconfiguration loop.
Besides making the code easier to test and understand, the goal is for this component to handle disconnect and loss updates about peers, and thereby, implement the v2 Tailnet API.
Further PRs will handle peer updates, status updates, and net info updates.
Then, after the subcomponent is implemented and tested, I will refactor Conn to use it instead of the current monolithic architecture.