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
7 changes: 2 additions & 5 deletions cmd/kind/create/cluster/createcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +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")
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 @@ -88,10 +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)
if flags.Network != "" {
ctx.SetNetwork(flags.Network)
}
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
}
}
10 changes: 4 additions & 6 deletions pkg/cluster/internal/meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ type ClusterMeta struct {
}

// 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 @@ -45,14 +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
}

func (c *ClusterMeta) SetNetwork(network string) {
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
2 changes: 0 additions & 2 deletions pkg/cluster/nodes/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ func CreateControlPlaneNode(name, image, network, clusterLabel string) (node *No
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 Down Expand Up @@ -92,7 +91,6 @@ func CreateExternalLoadBalancerNode(name, image, network, clusterLabel string) (
if network != "" {
extraArgs = append(extraArgs, "--network", network)
}

node, err = createNode(name, image, clusterLabel, config.ExternalLoadBalancerRole, extraArgs...)
if err != nil {
return node, err
Expand Down