feat!(connect): support for listen address on zarf connect #3914
Conversation
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
✅ Deploy Preview for zarf-docs canceled.
|
Codecov ReportAttention: Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
|
Moving to draft - going to align to |
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
| giteaClient, err := gitea.NewClient(tunnelURL, s.GitServer.PushUsername, s.GitServer.PushPassword) | ||
| // tunnel is created with the default listenAddress - there will only be one endpoint until otherwise supported | ||
| tunnelURLs := tunnel.HTTPEndpoints() | ||
| giteaClient, err := gitea.NewClient(tunnelURLs[0], s.GitServer.PushUsername, s.GitServer.PushPassword) |
There was a problem hiding this comment.
I think it would read better to have a function like FirstEndpoint for this, which errors if there isn't an endpoint. We will always have at least one based on how you setup the defaults, but this would just future proof it to 100% never panicing.
There was a problem hiding this comment.
I am good with making that addition - I felt retaining the Endpoint() method and simply returning the "first" arbitrarily as potentially deceiving (rather the user should decide which they want/need) - but the addition of a FirstEndpoint() is still reasonable.
There was a problem hiding this comment.
Thinking on this more - and the distinction between tunnel.Endpoints() and tunnel.HTTPEndpoints() and tunnel.FullURLs() -> I am a bit torn here.
I don't think it makes sense to create tunnel.First*()` methods for each. For sake of error traceability I do think it may make sense to return errors on zero present - otherwise let the consumer consume the endpoints as they see fit.
There was a problem hiding this comment.
Yeah I think that's reasonable. Though instead of an error I think we should check if the list is nil (go automatically treats lists of 0 length as nil)
There was a problem hiding this comment.
instead of an error I think we should check if the list is nil (go automatically treats lists of 0 length as nil)
Can you elaborate?
I generally understand the length check to be more idiomatic as it is safe for nil and non-nil states (such as empty non-nil states).
But maybe you are suggesting something else such as not returning the error?
There was a problem hiding this comment.
Sorry yeah I had some incorrect assumptions on how slices work and my suggestion was unclear. I'm suggesting that we only return a list then callers can error if the len is 0, before using the first item in the list.
There was a problem hiding this comment.
ah! I am good with that - change in progress.
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
…3914) Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com> Signed-off-by: Cade Thomas <cadethomas23@gmail.com>
Description
Adds the ability to specify listening addresses for binding the tunnel connection. This allows users to specify other addresses than the hardcoded
localhost/127.0.0.1in such a way as to bind to all interfaces for connectivity on the zarf CLI machine.IE
zarf connect <target> --address 0.0.0.0to bind to all interfaces.Primarily this replaces the use of
portforward.New()which haslocalhosthardcoded and callsportforward.NewOnAddresses()with directly callingportfoward.NewOnAddresses()and ensuring we pass inlocalhosteverywhere except for when overriding.Minor Note - added
lsalias tozarf connect listcommand as this follows the same patterns seen elsewhere (zarf package ls)The goal was minimal impact to tunnel function signature - that said, updates were required to support the tunnel endpoint methods now potentially returning many.
Related Issue
Fixes #3913
Checklist before merging