feat: derpmap field in config #1823
Conversation
WalkthroughThe changes introduce a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DERPConfig
participant DERPMapHandler
Client->>DERPConfig: Request DERP Map
DERPConfig->>DERPMapHandler: Check if DERPMap is not nil
alt DERPMap is not nil
DERPMapHandler->>DERPConfig: Append DERPMap to derpMaps
else DERPMap is nil
DERPMapHandler->>DERPConfig: Skip appending
end
DERPConfig->>Client: Return derpMaps
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if cfg.DERPMap != nil { | ||
| derpMaps = append(derpMaps, cfg.DERPMap) | ||
| } |
There was a problem hiding this comment.
Conditional check added to prevent nil dereferencing.
The addition of the conditional check at lines 85-87 is a good practice to ensure that only non-nil DERPMap instances are appended to the derpMaps slice. This change helps in preventing potential runtime errors due to nil dereferencing.
However, it's recommended to add unit tests to verify that this new logic correctly handles scenarios where cfg.DERPMap might be nil or non-nil.
Would you like me to help by writing some unit tests for this new logic?
| @@ -179,6 +179,7 @@ type DERPConfig struct { | |||
| STUNAddr string | |||
| URLs []url.URL | |||
| Paths []string | |||
| DERPMap *tailcfg.DERPMap | |||
There was a problem hiding this comment.
New field DERPMap added to DERPConfig struct.
The addition of the DERPMap field at line 182 enhances the configuration capabilities of the DERPConfig struct by allowing direct assignment of a DERP map. This change is beneficial for programmatically setting the DERP map, providing more flexibility compared to the previous methods.
It's recommended to update the project's documentation to include information about this new field and its usage.
Consider updating the documentation to reflect this new configuration option.
|
@kradalby bumping this. Are there any blockers for merging this and including it in the following releases? |
|
Hi sorry, I've missed this, you can already load DERP config via file, so another method seems excessive, I don't think we will include this. |
|
@kradalby I understand it can be loaded from a file, but that's a bit awkward when using headscale programmatically (as a lib). It would require the application to first write a file and then load it. Additionally, it provides more overhead if the configuration has to be updated after it was already written. From the looks of it, the PR wouldn't undermine loading the map from file, it simply allows users to add an initial derp map as an argument of the config. |
|
When you say lib, do you mean as part of a go program? because in that case you can just import it and override yourself. I still dont think we will include this, sorry. |
|
Ok fair, sorry, in my head I thought it would expose it to users via the config, as long as we dont let users set it via config.yaml, it is ok. |
Thanks! |
Currently DERP map can be set only from URLs or local yaml config paths. This PR adds a simple and optional
DERPMapconfig field which allows setting initial DERP map which is useful when using headscale programatically.Summary by CodeRabbit
DERPMapfield in the DERP configuration.