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

CAPD: make the kindest/haproxy image configurable #4950

Closed
joshrosso opened this issue Jul 15, 2021 · 11 comments · Fixed by #4951
Closed

CAPD: make the kindest/haproxy image configurable #4950

joshrosso opened this issue Jul 15, 2021 · 11 comments · Fixed by #4951
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@joshrosso
Copy link
Contributor

User Story

As an operator, I'd like to use a custom HA proxy image for my CAPD-based cluster's API server loadbalancer.

Detailed Description

Today, the haproxy image is hard coded and not configurable:

// Image defines the loadbalancer image:tag
const Image = "kindest/haproxy:2.1.1-alpine"

Making this configurable would allow CAPD users to reference custom images and/or registries where they can pull this image from.

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 15, 2021
@vincepri
Copy link
Member

/area provider/docker
/milestone v0.4

@fabriziopandini
Copy link
Member

What about implementing something like

// DockerClusterSpec defines the desired state of DockerCluster.
type DockerClusterSpec struct {
	...

	// LoadBalancer allows to define configurations for the cluster load balancer.
	// +optional
	LoadBalancer DockerLoadBalancer `json:"loadBalancer,omitempty"`
}

// DockerLoadBalancer allows to define configurations for the cluster load balancer.
type DockerLoadBalancer struct {
	// ImageMeta allows to customize the image used for image used for the cluster load balancer
	ImageMeta `json:",inline"`
}

// ImageMeta allows to customize the image used for components that are not
// originated from the Kubernetes/Kubernetes release process.
type ImageMeta struct {
	// ImageRepository sets the container registry to pull the haproxy image from.
	// if not set, "kindest" will be used instead.
	// +optional
	ImageRepository string `json:"imageRepository,omitempty"`

	// ImageTag allows to specify a tag for the haproxy image.
	// if not set, "2.1.1-alpine" will be used instead.
	// +optional
	ImageTag string `json:"imageTag,omitempty"`
}

So we have something that mimics what we have in kubeadm config types?

@joshrosso
Copy link
Contributor Author

joshrosso commented Jul 15, 2021

@fabriziopandini should ImageMeta actually contain 3 fields, as follows?

// ImageMeta allows to customize the image used for components that are not
// originated from the Kubernetes/Kubernetes release process.
type ImageMeta struct {
	// ImageRepository sets the container registry to pull the haproxy image from.
	// if not set, "kindest" will be used instead.
	// +optional
	ImageRepository string `json:"imageRepository,omitempty"`

        // Image specifies the image name inside the ImageRepository
        // if not set, "haproxy" will be used instead.
        Image string

	// ImageTag allows to specify a tag for the haproxy image.
	// if not set, "2.1.1-alpine" will be used instead.
	// +optional
	ImageTag string `json:"imageTag,omitempty"`
}

if your suggestion is more "standard" in how kubeadm/kubernetes/etc does things, i'm OK with your suggestion.

@stmcginnis
Copy link
Contributor

/assign

Will take a crack at the suggested change.

@vincepri
Copy link
Member

We usually don't allow overwriting the ImageName, keeping that fixed and consistent with all the other places where ImageMeta is used

@joshrosso
Copy link
Contributor Author

cool, makes sense -- thanks 👍

@vincepri
Copy link
Member

@fabriziopandini One thing to note though is that we're not calling out anywhere in the proposed API addition that the load balancer is haproxy

@stmcginnis
Copy link
Contributor

@vincepri should that be in a code comment calling that out, or are you thinking somewhere more visible?

@vincepri
Copy link
Member

Code comment could be good, I'm wondering though if we should have Haproxy as a specific field under the DockerLoadBalancer struct

@stmcginnis
Copy link
Contributor

So like @joshrosso was suggesting, but instead of having the field in ImageMeta, move it up a level to the DockerLoadBalancer struct?

@vincepri
Copy link
Member

vincepri commented Jul 15, 2021

Not the imageName per se, I was more suggesting something like these structs/fields
DockerLoadBalancer -> DockerHaproxyLoadBalancer -> ImageMeta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants