add stunnel client implementation #7
Conversation
alaypatel07
changed the title
alaypatel07
force-pushed
the
stunnel-client
branch
from
e46581f to
61e4c85
Compare
alaypatel07
force-pushed
the
stunnel-client
branch
4 times, most recently
from
e6427c5 to
a477b7b
Compare
Signed-off-by: Alay Patel <[email protected]>
alaypatel07
force-pushed
the
stunnel-client
branch
from
a477b7b to
c5f95af
Compare
transport/stunnel/client.go
Outdated
|
|
||
| type client struct { | ||
| logger logr.Logger | ||
| ingressPort int32 |
There was a problem hiding this comment.
I'm struggling a bit w/ the ingressPort name. It's ConnectPort in the Transport interface, but the NewClient params also refer to it as ingressPort. And Endpoint has IngressPort().
Did we make a mistake by having Transport.ConnectPort() and Endpoint.IngressPort()?
There was a problem hiding this comment.
It's part of a bad refactor, it should be connectPort here in the transport package.
IMO, having ListenPort and ConnectPort at the transport level makes sense because out of the port one is used to accept connection other is used to connect.
In the case of endpoint interface both are used to accept connections, the difference is whether it is exposed external to the cluster or it is internal because of which BackendPort and IngressPort made much more sense, assuming Ingress is the terms used widely to accept traffic into the cluster. Most Kube resources use targetPort and port to mean the same thing respectively but that was turning out to be a little confusing when we were having conversations in the team about this. I would be open to renaming the endpoint interface methods if we want to be consistent with Kube resource fields
| return "localhost" | ||
| } | ||
|
|
||
| func NewClient(ctx context.Context, c ctrlclient.Client, logger logr.Logger, |
alaypatel07
force-pushed
the
stunnel-client
branch
from
10e7f2c to
be65fd2
Compare
Co-authored-by: John Strunk <[email protected]> Signed-off-by: Alay Patel <[email protected]>
alaypatel07
force-pushed
the
stunnel-client
branch
from
be65fd2 to
207aed0
Compare
|
@JohnStrunk Thanks for the feedback, updated a couple of things and added clarification on the naming, PTAL. If we do want to revisit the endpoint interface naming, can we let this PR in and handle it in a separate issue? Thanks |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alaypatel07, JohnStrunk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Describe what this PR does
This adds stunnel client implementation. Depends on #4 fixes #5
Is there anything that requires special attention?
Related issues: