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.

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

switch external loadbalancer back to haproxy, health check #645

Merged
merged 2 commits into from
Jun 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions images/haproxy/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright 2019 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.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# standard haproxy image + minimal config so the container will not exit
ARG ARCH="amd64"
ARG BASE="haproxy:2.0.0-alpine"
FROM ${ARCH}/${BASE}
COPY haproxy.cfg /usr/local/etc/haproxy/haproxy.cfg
23 changes: 23 additions & 0 deletions images/haproxy/haproxy.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Copyright 2019 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.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# minimal config file to avoid haproxy exiting due to invalid / missing config
# kind will rewrite this config at runtime
frontend controlPlane
bind 0.0.0.0:6443
mode tcp
default_backend kube-apiservers

backend kube-apiservers
mode tcp
75 changes: 75 additions & 0 deletions images/haproxy/push-cross.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/bin/bash

# Copyright 2019 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.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# builds and pushes the haproxy image for all architectures

set -o errexit
set -o nounset
set -o pipefail

# cd to the repo root
REPO_ROOT=$(git rev-parse --show-toplevel)
cd "${REPO_ROOT}"

# cd to the image
cd ./images/haproxy

IMAGE="${IMAGE:-kindest/haproxy}"
TAG="${TAG:-2.0.0-alpine}"
BASE="haproxy:${TAG}"

# tag arch, manifest architecture, variant
ARCHES=(
"amd64,amd64,"
"arm32v6,arm,v6"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comma a typo or intended? arm,v6 or armv6

Copy link
Member Author

@BenTheElder BenTheElder Jun 24, 2019

Choose a reason for hiding this comment

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

docker manifest is a mess, this is intended. see the code below
this is:

  • arm32v6 when pulling the matching base image
  • arm for the architecture in the manifest
  • v6 for the variant

The comment above the array references this, but it's all kinda manifest internal stuff. I didn't want to inline an explanation of manifests 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

these are basically tuple (tagged_architecture,architecture,variant)
then we make another one below with (image_we_build,tagged_architecture,architecture,variant)

Copy link
Member

Choose a reason for hiding this comment

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

do we need add arm v7?

Copy link
Member Author

@BenTheElder BenTheElder Jun 24, 2019

Choose a reason for hiding this comment

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

Alpine doesn't support v7 last I checked, but arm v6 images / binaries work on v7. I'm pretty sure our only "users" so far are on arm64v8

Copy link
Member

Choose a reason for hiding this comment

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

 {                                                                                                              
    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",                                         
    "size": 946,
    "digest": "sha256:35d94f72942e907a56cc201a700e8f2378a204f40b641c55969fabbd259d1ca7",
    "platform": {                                                                                                
      "architecture": "arm",                                                                                     
      "os": "linux",                                                                                             
      "variant": "v7"                                                                                            
    }                                                                                                            
  },       

it has arm v7 manifest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed that all the other kubernetes/kubernetes images only have v6 and v8, we can revisit later if that changes.

"arm64v8,arm64,v8"
"ppc64le,ppc64le,"
)

# build all images
images=()
image_infos=()
for arch_info in "${ARCHES[@]}"; do
# build image
tag_arch="$(sed -E 's#^([^,]+),[^,]*,[^,]*$#\1#' <<<"${arch_info}")"
image="${IMAGE}:${tag_arch}-${TAG}"
docker build "--build-arg=ARCH=${tag_arch}" "--build-arg=BASE=${BASE}" -f Dockerfile -t "${image}" .
docker push "${image}"
# join image we tagged with arch info for building the manifest later
images+=("${image}")
image_infos+=("${image},${arch_info}")
done

# This option is required for running the docker manifest command
export DOCKER_CLI_EXPERIMENTAL="enabled"

# create and push the manifest
Copy link
Contributor

Choose a reason for hiding this comment

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

why not building and creating the manifest?
just curious, I never did this before, but seems too complicated having to use all this regex

Copy link
Member Author

Choose a reason for hiding this comment

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

Manifests are a meta image pointing to images for different hosts in this context, I'm not sure what you mean by "why not building and creating the manifest?"

Copy link
Member Author

@BenTheElder BenTheElder Jun 24, 2019

Choose a reason for hiding this comment

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

The regex is actually pretty simple and just to extract parts of the comma-joined tuple.
Each image has some metadata we need. We need to know:

  • what we built & tagged the specific image as
  • what architecture it was for
  • what variant, if any it was for
  • when building: what to pull the base as (which isn't always the same as arch + variant ...)

When we build the "meta image" (manifest list image) we have to supply most of that info for each specific image we built.

Copy link
Member Author

Choose a reason for hiding this comment

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

Kubernetes has some extremely similar logic for all of the cross platform images.

At some point I'll DRY this out to be nicer with kindnetd but for now this should be suitable and we only need to build if / when we want to upgrade haproxy anyhow.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that makes sense.
For "why not building and creating the manifest? I meant to do everything in the same for loop, seems that the variables you are extracting are available at build time

Copy link
Member Author

@BenTheElder BenTheElder Jun 24, 2019

Choose a reason for hiding this comment

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

Oh! Because if an image build fails we leak the manifest. It only removes the manifest with the -p/--purge on pushing the manifest. We don't want to increase the chance of having a lingering manifest. You have to literally find and remove the files on disk in what I think is an undocumented directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we only build the manifest after all the images are good to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it , thanks

docker manifest create "${IMAGE}:${TAG}" "${images[@]}"
for image_info in "${image_infos[@]}"; do
# split out image info
# image:arch-tag, grab arch
image="$(sed -E 's#^([^,]+),[^,]+,[^,]+,[^,]*$#\1#' <<<"${image_info}")"
#tag_arch="$(sed -E 's#^[^,]+,([^,]+),[^,]+,[^,]*$#\1#' <<<"${image_info}")"
architecture="$(sed -E 's#^[^,]+,[^,]+,([^,]+),[^,]*$#\1#' <<<"${image_info}")"
variant="$(sed -E 's#^[^,]+,[^,]+,[^,]+,([^,]*)$#\1#' <<<"${image_info}")"
args=("--arch" "${architecture}")
if [ -n "${variant}" ]; then
args+=("--variant" "${variant}")
fi
# add the image to the manifest
docker manifest annotate "${IMAGE}:${TAG}" "${image}" "${args[@]}"
done
docker manifest push --purge "${IMAGE}:${TAG}"
41 changes: 24 additions & 17 deletions pkg/cluster/internal/loadbalancer/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,33 @@ type ConfigData struct {

// DefaultConfigTemplate is the loadbalancer config template
const DefaultConfigTemplate = `# generated by kind
Copy link
Member Author

Choose a reason for hiding this comment

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

this should look familiar, it's probably good enough, but I'm still iterating on it.

global
log /dev/log local0
log /dev/log local1 notice
daemon

# load balance over the control planes
stream {
upstream tcp_backend {
{{- range $server, $address := .BackendServers}}
server {{ $address }};
{{- end}}
}
defaults
log global
mode tcp
option dontlognull
# TODO: tune these
timeout connect 5000
timeout client 50000
timeout server 50000

server {
listen {{ .ControlPlanePort }};
{{ if .IPv6 -}}
listen [::]:{{ .ControlPlanePort }};
{{- end }}
proxy_pass tcp_backend;
}
}
frontend control-plane
bind *:{{ .ControlPlanePort }}
{{ if .IPv6 -}}
bind :::{{ .ControlPlanePort }};
{{- end }}
default_backend kube-apiservers

# minimal events entry to make nginx happy
events {}
backend kube-apiservers
option httpchk GET /healthz
# TODO: we should be verifying (!)
Copy link
Member Author

Choose a reason for hiding this comment

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

This aspect is super not ideal but could be solved later.
Ideally where possible kind should behave like production. There's no real reason we need to be more insecure here other than :effort: and logistics around the certs. We should be able to do SSL backends with verified certs.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the status quo so it shouldn't block the PR, but ...

Copy link
Contributor

Choose a reason for hiding this comment

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

this is an interesting topic. If we move that way we can use other load balancers, currently, we are load balancing at TCP level commonly named SSL Passtrough, and haproxy is the only one (that I know) that allows to do health checking on a HTTPS endpoint.

If we are able to get the certificates to the load balancer we can start doing load balancer at L7

{{range $server, $address := .BackendServers}}
server {{ $server }} {{ $address }} check check-ssl verify none
{{- end}}
`

// Config returns a kubeadm config generated from config data, in particular
Expand Down
4 changes: 2 additions & 2 deletions pkg/cluster/internal/loadbalancer/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ package loadbalancer
const ControlPlanePort = 6443

// Image defines the loadbalancer image:tag
const Image = "nginx:1.15.12-alpine"
const Image = "kindest/haproxy:2.0.0-alpine"
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember talking about prepulling the loadbalancer image before but don´t remember the conclusion 😅 , should we prepull it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not prepulled because it runs on the host directly as a node. It only needs to pull once though.


// ConfigPath defines the path to the config file in the image
const ConfigPath = "/etc/nginx/nginx.conf"
const ConfigPath = "/usr/local/etc/haproxy/haproxy.cfg"