Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

spikecurtis
Copy link
Contributor

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.

Copy link
Contributor Author

spikecurtis commented Jan 4, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spikecurtis and the rest of your teammates on Graphite Graphite

@spikecurtis spikecurtis requested a review from coadler January 4, 2024 09:23
@spikecurtis spikecurtis force-pushed the spike/unused-context-tailnet branch from 9704400 to d9c25c1 Compare January 4, 2024 09:46
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch from da115a4 to 86113e2 Compare January 4, 2024 09:46
@spikecurtis spikecurtis force-pushed the spike/unused-context-tailnet branch from d9c25c1 to 9af1797 Compare January 4, 2024 10:37
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch from 86113e2 to 8d938f7 Compare January 4, 2024 10:37
Base automatically changed from spike/unused-context-tailnet to main January 5, 2024 04:15
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch 2 times, most recently from 3b7ebdf to 032d52d Compare January 8, 2024 13:00
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch 2 times, most recently from b03a760 to 6f37b9b Compare January 9, 2024 11:37
Copy link
Contributor

@coadler coadler left a 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

return out
}

func (c *configMaps) setAddresses(ips []netip.Prefix) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 👍

@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch from 6f37b9b to 0ec3722 Compare January 10, 2024 06:10
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch from 0ec3722 to d013264 Compare January 10, 2024 06:34
@spikecurtis spikecurtis force-pushed the spike/10533-configmaps-initial branch from d013264 to 018e3b8 Compare January 10, 2024 06:51
@spikecurtis spikecurtis merged commit 89e3bbe into main Jan 10, 2024
@spikecurtis spikecurtis deleted the spike/10533-configmaps-initial branch January 10, 2024 06:58
Copy link
Contributor Author

Merge activity

@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants