-
Notifications
You must be signed in to change notification settings - Fork 53
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
join: join over lb if available #2348
Conversation
✅ Deploy Preview for constellation-docs canceled.
|
34813ed
to
a26d0ae
Compare
Coverage report
|
could you add the context why we need this? |
can u also provide a run for miniconstellation? |
looked mostly over the changes regarding TF. |
// JoinServiceEndpoints returns the list of endpoints for the join service, which are running on the control plane nodes. | ||
func JoinServiceEndpoints(ctx context.Context, lister InstanceLister) ([]string, error) { |
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.
Since joinclient.go
and rejoinclient.go
have to perform the same action to fetch loadbalancer endpoint, and control plane IPs, what about adapting this function to include fetching of LB endpoint and using it for both?
With the current implementation, the code is slightly different for joinclient.go
and rejoinclient.go
.
Function could look like the following:
type endpointLister interface {
List(ctx context.Context) ([]InstanceMetadata, error)
GetLoadBalancerEndpoint(ctx context.Context) (host, port string, err error)
}
func JoinServiceEndpoints(ctx context.Context, lister endpointLister) ([]string, error) {
lbIP, _, err := lister.GetLoadBalancerEndpoint(ctx)
if err != nil {
// Maybe do additional error handling here in case of no loadbalancer (ignore error?)
return nil, fmt.Errorf("retrieving load balancer endpoint from cloud provider: %w", err)
}
// Old JoinServiceEndpoints code
joinEndpoints := []string{}
...
// Return all endpoints, with LB as the first endpoint
return append(net.JoinHostPort(lbEndpoint, strconv.Itoa(constants.JoinServiceNodePort)), joinEndpoints), nil
}
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.
Hm, I didn't like the place of this function as part of the metadata package as it is too specific to me. Also, without this function the metadata package looks cleaner.
I see you point of introducing a potential difference in both implementation though, but I haven't found the right spot.
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.
started a mini test and lgtm when green
* join: join over lb if available
* join: join over lb if available
Context
This is needed since the upcoming node-to-node strict mode does not allow traffic which is not WireGuard encrypted by Cilium (with the exception of etcd). Therefore nodes cannot join via directly talking to another node.
Proposed change(s)
The fallback is needed to still support Miniconstellation as it does not have an LB.
e2e upgrade tests:
azure: https://github.com/edgelesssys/constellation/actions/runs/6237449241
gcp: https://github.com/edgelesssys/constellation/actions/runs/6238824048
aws: https://github.com/edgelesssys/constellation/actions/runs/6238826460
Checklist