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

add a flag to configure docker network #277

Closed
wants to merge 2 commits into from
Closed

add a flag to configure docker network #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
924c858
add a flag to configure docker network
foolusion Feb 6, 2019
0a6a592
pkg/cluster/context: move network config here.
foolusion Feb 7, 2019
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
4 changes: 3 additions & 1 deletion cmd/kind/create/cluster/createcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

type flagpole struct {
Name string
Network string
Config string
ImageName string
Retain bool
Expand All @@ -51,6 +52,7 @@ func NewCommand() *cobra.Command {
},
}
cmd.Flags().StringVar(&flags.Name, "name", cluster.DefaultName, "cluster context name")
cmd.Flags().StringVar(&flags.Network, "network", "", "container network to use for cluster")
cmd.Flags().StringVar(&flags.Config, "config", "", "path to a kind config file")
cmd.Flags().StringVar(&flags.ImageName, "image", "", "node docker image to use for booting the cluster")
cmd.Flags().BoolVar(&flags.Retain, "retain", false, "retain nodes for debugging when cluster creation fails")
Expand Down Expand Up @@ -86,7 +88,7 @@ func runE(flags *flagpole, cmd *cobra.Command, args []string) error {
}

// create a cluster context and create the cluster
ctx := cluster.NewContext(flags.Name)
ctx := cluster.NewContext(flags.Name, cluster.WithNetwork(flags.Network))
if flags.ImageName != "" {
// Apply image override to all the Nodes defined in Config
// TODO(fabrizio pandini): this should be reconsidered when implementing
Expand Down
22 changes: 20 additions & 2 deletions pkg/cluster/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,16 @@ const DefaultName = "kind"

// NewContext returns a new cluster management context
// if name is "" the default name will be used
func NewContext(name string) *Context {
func NewContext(name string, options ...Option) *Context {
if name == "" {
name = DefaultName
}
var c contextConfig
for _, opt := range options {
opt(&c)
}
return &Context{
ClusterMeta: meta.NewClusterMeta(name),
ClusterMeta: meta.NewClusterMeta(name, c.network),
}
}

Expand Down Expand Up @@ -207,3 +211,17 @@ func (c *Context) CollectLogs(dir string) error {
}
return logs.Collect(nodes, dir)
}

type contextConfig struct {
network string
}

// Option allows optional configuration of the cluster context.
type Option func(*contextConfig)

// WithNetwork sets the container network to be used when creating a cluster.
func WithNetwork(network string) func(*contextConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason this should be an option on context instead of Create()? do you forsee other calls needing this?

👍 for this pattern in general.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't know if anything else was calling Create so I didn't want to change the parameters. Either way works. I haven't dug into the code enough to see how/where it is used.

return func(c *contextConfig) {
c.network = network
}
}
6 changes: 3 additions & 3 deletions pkg/cluster/internal/create/createcontext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,11 @@ func (cc *Context) ProvisionNodes() (nodeList map[string]*nodes.Node, err error)

switch configNode.Role {
case config.ExternalLoadBalancerRole:
node, err = nodes.CreateExternalLoadBalancerNode(name, configNode.Image, cc.ClusterLabel())
node, err = nodes.CreateExternalLoadBalancerNode(name, configNode.Image, cc.Network(), cc.ClusterLabel())
case config.ControlPlaneRole:
node, err = nodes.CreateControlPlaneNode(name, configNode.Image, cc.ClusterLabel())
node, err = nodes.CreateControlPlaneNode(name, configNode.Image, cc.Network(), cc.ClusterLabel())
case config.WorkerRole:
node, err = nodes.CreateWorkerNode(name, configNode.Image, cc.ClusterLabel())
node, err = nodes.CreateWorkerNode(name, configNode.Image, cc.Network(), cc.ClusterLabel())
}
if err != nil {
return nodeList, err
Expand Down
13 changes: 10 additions & 3 deletions pkg/cluster/internal/meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ import (
// cluster metadata
// See: NewClusterMeta
type ClusterMeta struct {
name string
name string
network string
}

// NewClusterMeta returns a new cluster meta
func NewClusterMeta(name string) *ClusterMeta {
func NewClusterMeta(name, network string) *ClusterMeta {
return &ClusterMeta{
name: name,
name: name,
network: network,
}
}

Expand All @@ -44,6 +46,11 @@ func (c *ClusterMeta) Name() string {
return c.name
}

// Network returns the cluster's container network
func (c *ClusterMeta) Network() string {
foolusion marked this conversation as resolved.
Show resolved Hide resolved
return c.network
}

// KubeConfigPath returns the path to where the Kubeconfig would be placed
// by kind based on the configuration.
func (c *ClusterMeta) KubeConfigPath() string {
Expand Down
28 changes: 20 additions & 8 deletions pkg/cluster/nodes/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,22 @@ func getPort() (int, error) {

// CreateControlPlaneNode creates a contol-plane node
// and gets ready for exposing the the API server
func CreateControlPlaneNode(name, image, clusterLabel string) (node *Node, err error) {
func CreateControlPlaneNode(name, image, network, clusterLabel string) (node *Node, err error) {
// gets a random host port for the API server
port, err := getPort()
if err != nil {
return nil, errors.Wrap(err, "failed to get port for API server")
}

node, err = createNode(name, image, clusterLabel, config.ControlPlaneRole,
extraArgs := []string{
// publish selected port for the API server
"--expose", fmt.Sprintf("%d", port),
"-p", fmt.Sprintf("%d:%d", port, kubeadm.APIServerPort),
)
}
if network != "" {
extraArgs = append(extraArgs, "--network", network)
Copy link
Member

Choose a reason for hiding this comment

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

this binds a kind CLI flag to the underlying CR (docker).
i guess we need to evaluate if we want this coupling for the 1.0 release.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i'm iffy on this. 🤔
I this probably does make sense as a flag, I guess, but as a rule of thumb we should prefer config... 🤔

Copy link
Author

Choose a reason for hiding this comment

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

If it was in the config would it be at the top level?
Is there any setup where your nodes would be in different networks?

Copy link
Member

@BenTheElder BenTheElder Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, time flew by so fast :(

i'm not sure.. I could see this (EDIT: this = per node network options) maybe, we could start top level and do an override.

Either way we're mostly decoupled from docker at the user interface right now, and I do intend to support podman etc., I meant to investigate further if there was a better way to specify this or any other work arounds.

I do see how this is a feature we need to enable.

}
node, err = createNode(name, image, clusterLabel, config.ControlPlaneRole, extraArgs...)
if err != nil {
return node, err
}
Expand All @@ -72,18 +76,22 @@ func CreateControlPlaneNode(name, image, clusterLabel string) (node *Node, err e

// CreateExternalLoadBalancerNode creates an external loab balancer node
// and gets ready for exposing the the API server and the load balancer admin console
func CreateExternalLoadBalancerNode(name, image, clusterLabel string) (node *Node, err error) {
func CreateExternalLoadBalancerNode(name, image, network, clusterLabel string) (node *Node, err error) {
// gets a random host port for control-plane load balancer
port, err := getPort()
if err != nil {
return nil, errors.Wrap(err, "failed to get port for control-plane load balancer")
}

node, err = createNode(name, image, clusterLabel, config.ExternalLoadBalancerRole,
extraArgs := []string{
// publish selected port for the control plane
"--expose", fmt.Sprintf("%d", port),
"-p", fmt.Sprintf("%d:%d", port, haproxy.ControlPlanePort),
)
}
if network != "" {
extraArgs = append(extraArgs, "--network", network)
}
node, err = createNode(name, image, clusterLabel, config.ExternalLoadBalancerRole, extraArgs...)
if err != nil {
return node, err
}
Expand All @@ -95,8 +103,12 @@ func CreateExternalLoadBalancerNode(name, image, clusterLabel string) (node *Nod
}

// CreateWorkerNode creates a worker node
func CreateWorkerNode(name, image, clusterLabel string) (node *Node, err error) {
node, err = createNode(name, image, clusterLabel, config.WorkerRole)
func CreateWorkerNode(name, image, network, clusterLabel string) (node *Node, err error) {
var extraArgs []string
if network != "" {
extraArgs = []string{"--network", network}
}
node, err = createNode(name, image, clusterLabel, config.WorkerRole, extraArgs...)
if err != nil {
return node, err
}
Expand Down