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 1 commit
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
5 changes: 5 additions & 0 deletions 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", "", "network to use for cluster")
Copy link
Member

@neolit123 neolit123 Feb 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

could we use something more descriptive here, e.g. outlining that this network will be used for the containers.
e.g. container network to use for the cluster

  • listing possible values?

also, what is the state of other container runtimes in terms of supporting such a flag; what are the alternatives in naming and values?
e.g. do CRI-O / containerd have a --network where the Docker values such as "bridge" will make sense?

Copy link
Author

Choose a reason for hiding this comment

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

I checked out a few of the other container runtimes.

rkt has a --net flag that you can pass one or more networks I believe it uses CNI.
docker seems to also support the shorter --net flag

CRI-O, containerd do not support networking directly, you must configure it through runc or CNI.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for checking.
CRI-O and containerd are the 2 more active CR after docker in the k8s space, while rkt is not used a lot.

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 @@ -87,6 +89,9 @@ func runE(flags *flagpole, cmd *cobra.Command, args []string) error {

// create a cluster context and create the cluster
ctx := cluster.NewContext(flags.Name)
if flags.Network != "" {
ctx.SetNetwork(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
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
11 changes: 10 additions & 1 deletion pkg/cluster/internal/meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import (
// cluster metadata
// See: NewClusterMeta
type ClusterMeta struct {
name string
name string
network string
}

// NewClusterMeta returns a new cluster meta
Expand All @@ -44,6 +45,14 @@ func (c *ClusterMeta) Name() string {
return c.name
}

func (c *ClusterMeta) Network() string {
foolusion marked this conversation as resolved.
Show resolved Hide resolved
return c.network
}

func (c *ClusterMeta) SetNetwork(network string) {
Copy link
Member

Choose a reason for hiding this comment

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

cluster meta is not supposed to be mutable.

I should really clean up how create works :|

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't want to add anything to the NewClusterMeta func yet since it would require more changes. I can add it as a variadic param to NewClusterMeta since network is not required and existing code doesn't break.

Copy link
Member

Choose a reason for hiding this comment

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

er what I really meant by this, is that clusterMeta is a terrible name, this is really just the cluster key (name) and some utils right now, I suspect Create(...) is the right place to plumb this through.

c.network = 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
30 changes: 22 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,23 @@ 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.

}

foolusion marked this conversation as resolved.
Show resolved Hide resolved
node, err = createNode(name, image, clusterLabel, config.ControlPlaneRole, extraArgs...)
if err != nil {
return node, err
}
Expand All @@ -72,18 +77,23 @@ 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)
}

foolusion marked this conversation as resolved.
Show resolved Hide resolved
node, err = createNode(name, image, clusterLabel, config.ExternalLoadBalancerRole, extraArgs...)
if err != nil {
return node, err
}
Expand All @@ -95,8 +105,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