Skip to content
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.

Sign up for GitHub

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

🌱 Make load balancer first-party package in CAPD #8246

Merged
merged 1 commit into from
Mar 9, 2023
Merged

🌱 Make load balancer first-party package in CAPD #8246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/book/src/developer/providers/v1.3-to-v1.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ maintainers of providers and consumers of our Go API.
For more information, please see: https://github.com/kubernetes/enhancements/issues/2845
- A new `KCPRemediationSpec` test has been added providing better test coverage for KCP remediation most common use cases. As a consequence `MachineRemediationSpec` now only tests remediation of
worker machines (NOTE: we plan to improve this test as well in a future iteration).

- Package `test/infrastructure/docker/internal/third_party/forked/loadbalancer` has been moved to `test/infrastructure/docker/internal/loadbalancer` to allow it to diverge from the upstream Kind package.
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
-
### Suggested changes for providers

- Providers should add an explicit security context to their controllers deployment, see [#7831](https://github.com/kubernetes-sigs/cluster-api/pull/7831) for reference.
2 changes: 1 addition & 1 deletion test/infrastructure/docker/internal/docker/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"sigs.k8s.io/cluster-api/test/infrastructure/container"
infrav1 "sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1beta1"
"sigs.k8s.io/cluster-api/test/infrastructure/docker/internal/docker/types"
"sigs.k8s.io/cluster-api/test/infrastructure/docker/internal/third_party/forked/loadbalancer"
"sigs.k8s.io/cluster-api/test/infrastructure/docker/internal/loadbalancer"
)

type lbCreator interface {
Expand Down
26 changes: 18 additions & 8 deletions ...third_party/forked/loadbalancer/config.go → ...re/docker/internal/loadbalancer/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2019 The Kubernetes Authors.
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -23,19 +23,26 @@ import (
"sigs.k8s.io/kind/pkg/errors"
)

// ConfigData is supplied to the loadbalancer config template
// ConfigData is supplied to the loadbalancer config template.
type ConfigData struct {
ControlPlanePort int
BackendServers map[string]string
IPv6 bool
}

// DefaultConfigTemplate is the loadbalancer config template
const DefaultConfigTemplate = `# generated by kind
// ConfigTemplate is the loadbalancer config template.
const ConfigTemplate = `# generated by kind
global
log /dev/log local0
log /dev/log local1 notice
daemon
# limit memory usage to approximately 18 MB
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
# (see https://github.com/kubernetes-sigs/kind/pull/3115)
maxconn 100000

resolvers docker
nameserver dns 127.0.0.11:53

defaults
log global
mode tcp
Expand All @@ -44,24 +51,27 @@ defaults
timeout connect 5000
timeout client 50000
timeout server 50000
# allow to boot despite dns don't resolve backends
default-server init-addr none

frontend control-plane
bind *:{{ .ControlPlanePort }}
{{ if .IPv6 -}}
bind :::{{ .ControlPlanePort }};
{{- end }}
default_backend kube-apiservers

backend kube-apiservers
option httpchk GET /healthz
# TODO: we should be verifying (!)
{{range $server, $address := .BackendServers}}
server {{ $server }} {{ $address }} check check-ssl verify none
server {{ $server }} {{ $address }} check check-ssl verify none resolvers docker resolve-prefer {{ if $.IPv6 -}} ipv6 {{- else -}} ipv4 {{- end }}
{{- end}}
`

// Config returns a kubeadm config generated from config data, in particular
// the kubernetes version
// Config generates the loadbalancer config from the ConfigTemplate and ConfigData.
func Config(data *ConfigData) (config string, err error) {
t, err := template.New("loadbalancer-config").Parse(DefaultConfigTemplate)
t, err := template.New("loadbalancer-config").Parse(ConfigTemplate)
if err != nil {
return "", errors.Wrap(err, "failed to parse config template")
}
Expand Down
20 changes: 11 additions & 9 deletions .../third_party/forked/loadbalancer/const.go → ...ure/docker/internal/loadbalancer/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2019 The Kubernetes Authors.
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -16,14 +16,16 @@ limitations under the License.

package loadbalancer

// Image defines the loadbalancer image name
const Image = "haproxy"
const (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably also change to use the "official" haproxy image, so

Image = "haproxy-alpine"
DefaultImageRepository = "haproxytech"
DefaultImageTag        = "2.8"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue (same as the issue for the Kind repo) is the official image doesn't have a config file attached which means it won't start. We could definitely workaround this by setting up the image in the code, but AFAIU CAPI needs to make some changes either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used the official image for the sample provider used in the kubecon na tutorial: https://github.com/capi-samples/cluster-api-provider-docker/tree/main/pkg/loadbalancer

I don't overly remember us having to change anything in the loadbalancer package but i can check.

Copy link
Member

@sbueringer sbueringer Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please consider that the more we move away from what kind is doing (probably) the more maintenance effort we have. After all CAPD clusters are still very closely modeled after kind clusters and there are dependencies.

Copy link
Contributor Author

@killianmuldoon killianmuldoon Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer @richardcase - this is working. Looks like it's the 'global' haproxy image which doesn't have a config loaded. The haproxytech image has a default config which lets it run initially.

Let's please consider that the more we move away from what kind is doing (probably) the more maintenance effort we have. After all CAPD clusters are still very closely modeled after kind clusters and there are dependencies.

I'm fine with pushing changing the image to a follow up - but I don't think there's additional maintenance effort here compared to using a third_party folder. The change is that this part of CAPD wasn't really maintained at all previously.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. I didn't look into it in detail, but if you're sure that there are no dependencies to the kind haproxy I'm fine with switching to the "upstream" one. I just know that we have a lot of dependencies to the kindest/node image and even make sure that we start the node image exactly the same way as kind does.

If we don't have stuff like this for the lb image, I agree, it shouldn't matter

Copy link
Member

@sbueringer sbueringer Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's this comment in the kind repo:

We cannot merely use the upstream haproxy image as haproxy will exit without a minimal config, so we introduce one that will list on the intended port and hot reload it at runtime with the actual desired config.
https://github.com/kubernetes-sigs/kind/tree/main/images/haproxy

I assume hot reloading works with the haproxytech image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly - this image exists with the _/haproxy image, but not the haproxytext/haproxy image.

I'm 100 percent fine with sticking with the haproxy image from kind for now btw - I don't see any motivation to change this. That said, making this directory first party will allow changes of that kind in future if someone is interested.

For now let's not change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

// Image is the loadbalancer image name.
Image = "haproxy"

// DefaultImageRepository defines the loadbalancer image repository
const DefaultImageRepository = "kindest"
// DefaultImageRepository is the loadbalancer image repository.
DefaultImageRepository = "kindest"

// DefaultImageTag defines the loadbalancer image tag
const DefaultImageTag = "v20210715-a6da3463"
// DefaultImageTag is the loadbalancer image tag.
DefaultImageTag = "v20230227-d46f45b6"

// ConfigPath defines the path to the config file in the image
const ConfigPath = "/usr/local/etc/haproxy/haproxy.cfg"
// ConfigPath is the path to the config file in the image.
ConfigPath = "/usr/local/etc/haproxy/haproxy.cfg"
)
5 changes: 3 additions & 2 deletions ...al/third_party/forked/loadbalancer/doc.go → ...cture/docker/internal/loadbalancer/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2019 The Kubernetes Authors.
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,5 +14,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Package loadbalancer contains external loadbalancer related constants and configuration
// Package loadbalancer contains external loadbalancer related constants and configuration.
// This package was originally forked from the kind load balancer at https://github.com/kubernetes-sigs/kind/tree/v0.7.0/pkg/cluster/internal/loadbalancer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the correct level of attribution, open to different opinions here for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's fine given that we copy from kubernetes-sigs to kubernetes-sigs.

If you want to make sure I would ask Ben.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kubernetes has done this sort of thing before, including in KIND!

IMHO we have a nice shortcut here in that these projects are both under the same license and "owner" (the project / contributors) so it's fine to just copy. I recommend attribution comments like this both for historical context and to acknowledge that another project originally wrote them.

So yes, this is fine to me. For prior art see also kubeadm/kinder tool 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thx for chiming in!

package loadbalancer
7 changes: 0 additions & 7 deletions test/infrastructure/docker/internal/third_party/forked/README.md
View file
Open in desktop

This file was deleted.