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

Using bridge network for nodes #484

Closed
Closed
Show file tree
Hide file tree
Changes from 14 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
4 changes: 4 additions & 0 deletions cmd/kind/create/cluster/createcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ func runE(flags *flagpole, cmd *cobra.Command, args []string) error {

// create a cluster context and create the cluster
ctx := cluster.NewContext(flags.Name)
// using cluster name as bridge network name
if cfg.Networking.BridgeName == "" {
cfg.Networking.BridgeName = flags.Name
}
if flags.ImageName != "" {
// Apply image override to all the Nodes defined in Config
// TODO(fabrizio pandini): this should be reconsidered when implementing
Expand Down
3 changes: 3 additions & 0 deletions pkg/cluster/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,7 @@ type Networking struct {
// If DisableDefaultCNI is true, kind will not install the default CNI setup.
// Instead the user should install their own CNI after creating the cluster.
DisableDefaultCNI bool
// BridgeName is the bridge network name.
// Defaults to same as cluster name.
BridgeName string
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

(this config field name is not suitable, we need to separate docker specific functionality and make that clear)

}
3 changes: 3 additions & 0 deletions pkg/cluster/config/v1alpha3/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,7 @@ type Networking struct {
// If DisableDefaultCNI is true, kind will not install the default CNI setup.
// Instead the user should install their own CNI after creating the cluster.
DisableDefaultCNI bool `json:"disableDefaultCNI,omitempty"`
// Name is the bridge network name.
// Defaults to same as cluster name.
BridgeName string `json:"bridgeName,omitempty"`
}
2 changes: 2 additions & 0 deletions pkg/cluster/config/v1alpha3/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,7 @@ func (a *Action) Execute(ctx *actions.ActionContext) error {
return err
}
for _, n := range controlPlaneNodes {
controlPlaneIP, err := n.IP()
if err != nil {
return errors.Wrapf(err, "failed to get IP for node %s", n.Name())
}
backendServers[n.Name()] = fmt.Sprintf("%s:%d", controlPlaneIP, kubeadm.APIServerPort)
backendServers[n.Name()] = fmt.Sprintf("%s:%d", n.Name(), kubeadm.APIServerPort)
tao12345666333 marked this conversation as resolved.
Show resolved Hide resolved
}

// create loadbalancer config data
Expand Down
31 changes: 26 additions & 5 deletions pkg/cluster/internal/create/nodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"sigs.k8s.io/kind/pkg/cluster/nodes"
"sigs.k8s.io/kind/pkg/concurrent"
"sigs.k8s.io/kind/pkg/container/cri"
"sigs.k8s.io/kind/pkg/container/docker"
logutil "sigs.k8s.io/kind/pkg/log"
)

Expand Down Expand Up @@ -80,6 +81,14 @@ func provisionNodes(
) error {
defer status.End(false)

// check if the network is exist.
if !docker.IsNetworkExist(cfg.Networking.BridgeName) {
// create bridge network for nodes.
if err := createNetwork(status, cfg.Networking.BridgeName); err != nil {
return err
}
}

if err := createNodeContainers(status, cfg, clusterName, clusterLabel); err != nil {
return err
}
Expand All @@ -88,6 +97,18 @@ func provisionNodes(
return nil
}

func createNetwork(status *logutil.Status, clusterName string) error {
defer status.End(false)

status.Start("Creating bridge network 🌉")
tao12345666333 marked this conversation as resolved.
Show resolved Hide resolved
if err := docker.CreateNetwork(clusterName); err != nil {
tao12345666333 marked this conversation as resolved.
Show resolved Hide resolved
return err
}

status.End(true)
return nil
}

func createNodeContainers(
status *logutil.Status, cfg *config.Cluster, clusterName, clusterLabel string,
) error {
Expand All @@ -103,7 +124,7 @@ func createNodeContainers(
desiredNode := desiredNode // capture loop variable
fns = append(fns, func() error {
// create the node into a container (~= docker run -d)
_, err := desiredNode.Create(clusterLabel)
_, err := desiredNode.Create(cfg.Networking.BridgeName, clusterLabel)
return err
})
}
Expand Down Expand Up @@ -193,16 +214,16 @@ func nodesToCreate(cfg *config.Cluster, clusterName string) []nodeSpec {
}

// TODO(bentheelder): remove network in favor of []cri.PortMapping when that is in
func (d *nodeSpec) Create(clusterLabel string) (node *nodes.Node, err error) {
func (d *nodeSpec) Create(networkName, clusterLabel string) (node *nodes.Node, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

why isn't this in the nodeSpec?

// create the node into a container (docker run, but it is paused, see createNode)
// TODO(bentheelder): decouple from config objects further
switch d.Role {
case constants.ExternalLoadBalancerNodeRoleValue:
node, err = nodes.CreateExternalLoadBalancerNode(d.Name, d.Image, clusterLabel, d.APIServerAddress, d.APIServerPort)
node, err = nodes.CreateExternalLoadBalancerNode(d.Name, d.Image, networkName, clusterLabel, d.APIServerAddress, d.APIServerPort)
case constants.ControlPlaneNodeRoleValue:
node, err = nodes.CreateControlPlaneNode(d.Name, d.Image, clusterLabel, d.APIServerAddress, d.APIServerPort, d.ExtraMounts)
node, err = nodes.CreateControlPlaneNode(d.Name, d.Image, networkName, clusterLabel, d.APIServerAddress, d.APIServerPort, d.ExtraMounts)
case constants.WorkerNodeRoleValue:
node, err = nodes.CreateWorkerNode(d.Name, d.Image, clusterLabel, d.ExtraMounts)
node, err = nodes.CreateWorkerNode(d.Name, d.Image, networkName, clusterLabel, d.ExtraMounts)
default:
return nil, errors.Errorf("unknown node role: %s", d.Role)
}
Expand Down
13 changes: 12 additions & 1 deletion pkg/cluster/internal/delete/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"sigs.k8s.io/kind/pkg/cluster/internal/context"
"sigs.k8s.io/kind/pkg/cluster/nodes"
"sigs.k8s.io/kind/pkg/container/docker"
)

// Cluster deletes the cluster identified by ctx
Expand All @@ -46,5 +47,15 @@ func Cluster(c *context.Context) error {
fmt.Printf("$KUBECONFIG is still set to use %s even though that file has been deleted, remember to unset it\n", c.KubeConfigPath())
}

return nodes.Delete(n...)
// before delete node, we inspect it to get network name.
networkName, err := docker.Inspect(n[0].Name(), "{{range $name, $_:=.NetworkSettings.Networks}}{{$name}}{{end}}")
if err != nil {
BenTheElder marked this conversation as resolved.
Show resolved Hide resolved
return errors.Wrap(err, "error inspect nodes")
}

if err := nodes.Delete(n...); err != nil {
return errors.Wrap(err, "error deleting nodes")
}
// after all nodes have been deleted, we need delete the network
return docker.DeleteNetwork(networkName[0])
tao12345666333 marked this conversation as resolved.
Show resolved Hide resolved
}
3 changes: 3 additions & 0 deletions pkg/cluster/internal/kubeadm/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ kubeletConfiguration:
nodefs.available: "0%"
nodefs.inodesFree: "0%"
imagefs.available: "0%"
resolvConf: /kind/resolv.conf
controllerManagerExtraArgs:
enable-hostpath-provisioner: "true"
nodeRegistration:
Expand Down Expand Up @@ -181,6 +182,7 @@ evictionHard:
nodefs.available: "0%"
nodefs.inodesFree: "0%"
imagefs.available: "0%"
resolvConf: /kind/resolv.conf
Copy link
Member

Choose a reason for hiding this comment

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

this is not being done for all api versions

---
# no-op entry that exists solely so it can be patched
apiVersion: kubeproxy.config.k8s.io/v1alpha1
Expand Down Expand Up @@ -245,6 +247,7 @@ evictionHard:
nodefs.available: "0%"
nodefs.inodesFree: "0%"
imagefs.available: "0%"
resolvConf: /kind/resolv.conf
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the idea that containerd and debugging tools will have a different resolv conf than kubelet.

Copy link
Member Author

Choose a reason for hiding this comment

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

as you said

we could selectively merge the resolv.conf

Copy link
Member Author

Choose a reason for hiding this comment

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

After considering it, I think the current practice is relatively simple. If we do resolv.conf merge, we need a lot of testing. Why don't we put it later? 🤔

Keep this practice for a while, then change it after the verification is passed.

I hope we can move forward. ❤️

Copy link
Member

Choose a reason for hiding this comment

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

this means other components have an inconsistent resolv conf and we're not actually solving the issue. if we're going to use hostnames we need kubelet and everything else to have the same lookup.

Copy link
Contributor

Choose a reason for hiding this comment

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

technically having a different resolv.conf doesn't mean that have a different lookup, just that the pods will not be able to resolv the nodes names, the other requests will use the same dns server, however I agree that reality is always different and we can be broken because docker changes on the DNS behaviour, that will not be surprising 😅

Docker daemon runs an embedded DNS server which provides DNS resolution among containers connected to the same user-defined network, so that these containers can resolve container names to IP addresses. If the embedded DNS server is unable to resolve the request, it will be forwarded to any external DNS servers configured for the container. To facilitate this when the container is created, only the embedded DNS server reachable at 127.0.0.11 will be listed in the container’s resolv.conf file.

I agree with tao here, I think that we should left until last the DNS exercise

Copy link
Member

@neolit123 neolit123 May 9, 2019

Choose a reason for hiding this comment

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

+1 on merging the resolv.confs if feasable.

---
# no-op entry that exists solely so it can be patched
apiVersion: kubeproxy.config.k8s.io/v1alpha1
Expand Down
25 changes: 18 additions & 7 deletions pkg/cluster/nodes/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func getPort() (int32, error) {

// CreateControlPlaneNode creates a contol-plane node
// and gets ready for exposing the the API server
func CreateControlPlaneNode(name, image, clusterLabel, listenAddress string, port int32, mounts []cri.Mount) (node *Node, err error) {
func CreateControlPlaneNode(name, image, networkName, clusterLabel, listenAddress string, port int32, mounts []cri.Mount) (node *Node, err error) {
// gets a random host port for the API server
if port == 0 {
p, err := getPort()
Expand All @@ -60,7 +60,7 @@ func CreateControlPlaneNode(name, image, clusterLabel, listenAddress string, por
}

node, err = createNode(
name, image, clusterLabel, constants.ControlPlaneNodeRoleValue, mounts,
name, image, networkName, clusterLabel, constants.ControlPlaneNodeRoleValue, mounts,
// publish selected port for the API server
"--expose", fmt.Sprintf("%d", port),
"-p", fmt.Sprintf("%s:%d:%d", listenAddress, port, kubeadm.APIServerPort),
Expand All @@ -74,12 +74,16 @@ func CreateControlPlaneNode(name, image, clusterLabel, listenAddress string, por
cache.ports = map[int32]int32{kubeadm.APIServerPort: port}
})

if err := addResolve(node); err != nil {
return node, err
}

return node, nil
}

// 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, listenAddress string, port int32) (node *Node, err error) {
func CreateExternalLoadBalancerNode(name, image, networkName, clusterLabel, listenAddress string, port int32) (node *Node, err error) {
// gets a random host port for control-plane load balancer
// gets a random host port for the API server
if port == 0 {
Expand All @@ -90,7 +94,7 @@ func CreateExternalLoadBalancerNode(name, image, clusterLabel, listenAddress str
port = p
}

node, err = createNode(name, image, clusterLabel, constants.ExternalLoadBalancerNodeRoleValue,
node, err = createNode(name, image, networkName, clusterLabel, constants.ExternalLoadBalancerNodeRoleValue,
nil,
// publish selected port for the control plane
"--expose", fmt.Sprintf("%d", port),
Expand All @@ -109,19 +113,24 @@ func CreateExternalLoadBalancerNode(name, image, clusterLabel, listenAddress str
}

// CreateWorkerNode creates a worker node
func CreateWorkerNode(name, image, clusterLabel string, mounts []cri.Mount) (node *Node, err error) {
node, err = createNode(name, image, clusterLabel, constants.WorkerNodeRoleValue, mounts)
func CreateWorkerNode(name, image, networkName, clusterLabel string, mounts []cri.Mount) (node *Node, err error) {
node, err = createNode(name, image, networkName, clusterLabel, constants.WorkerNodeRoleValue, mounts)
if err != nil {
return node, err
}

if err = addResolve(node); err != nil {
return node, err
}

return node, nil
}

// TODO(bentheelder): refactor this to not have extraArgs
// createNode `docker run`s the node image, note that due to
// images/node/entrypoint being the entrypoint, this container will
// effectively be paused until we call actuallyStartNode(...)
func createNode(name, image, clusterLabel, role string, mounts []cri.Mount, extraArgs ...string) (handle *Node, err error) {
func createNode(name, image, networkName, clusterLabel, role string, mounts []cri.Mount, extraArgs ...string) (handle *Node, err error) {
runArgs := []string{
"-d", // run the container detached
"-t", // allocate a tty for entrypoint logs
Expand All @@ -142,6 +151,8 @@ func createNode(name, image, clusterLabel, role string, mounts []cri.Mount, extr
"--label", clusterLabel,
// label the node with the role ID
"--label", fmt.Sprintf("%s=%s", constants.NodeRoleKey, role),
// connect node to network
"--network", networkName,
}

// pass proxy environment variables to be used by node's docker deamon
Expand Down
28 changes: 28 additions & 0 deletions pkg/cluster/nodes/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ package nodes

import (
"fmt"
"io/ioutil"
"regexp"
"runtime"

"github.com/pkg/errors"
"sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer"
Expand All @@ -44,3 +47,28 @@ func GetControlPlaneEndpoint(allNodes []Node) (string, error) {

return fmt.Sprintf("%s:%d", loadBalancerIP, loadbalancer.ControlPlanePort), nil
}

// to avoid the DNS crash we need copy host's /etc/resolv.conf to node
// ref: https://github.com/kubernetes-sigs/kind/pull/484#issuecomment-489469044
func addResolve(node *Node) error {
// TODO(Jintao Zhang): I can't accurately judge the behavior of windows, so I skipped it for now.
// https://github.com/kubernetes-sigs/kind/pull/484#discussion_r284064671
if runtime.GOOS == "windows" {
return nil
}

resolv, err := ioutil.ReadFile("/etc/resolv.conf")
Copy link
Member

Choose a reason for hiding this comment

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

windows has no /etc/resolv.conf.

Copy link
Member

Choose a reason for hiding this comment

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

this is a blocker.
/hold

Copy link
Member Author

Choose a reason for hiding this comment

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

windows has no /etc/resolv.conf.

Then I will judge through the OS first, skip the behavior of windows. I can't accurately judge the behavior of windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

just skip Windows. 🙊

Copy link
Member

Choose a reason for hiding this comment

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

this is the first time kind does not behave the same in all environments, not really a fan of that.

also, I don't think we should be reading the host's resolv.conf at all

if err != nil {
return errors.Wrap(err, "failed to read /etc/resolv.conf")
}

// filter the loopback addresses
re := regexp.MustCompile("(?m)[\r\n]+^.*((127.([0-9]{1,3}.){2}[0-9]{1,3})|(::1)).*$")
Copy link
Member

@BenTheElder BenTheElder Jun 5, 2019

Choose a reason for hiding this comment

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

this shouldn't even be necessary in the hosts's resolv.conf ?

content := re.ReplaceAllString(string(resolv), "")

if err := node.WriteFile("/kind/resolv.conf", content); err != nil {
return errors.Wrap(err, "failed to write /kind/resolv.conf to node")
}

return nil
}
60 changes: 60 additions & 0 deletions pkg/container/docker/network.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
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.
*/

package docker

import (
"fmt"

"sigs.k8s.io/kind/pkg/cluster/constants"
"sigs.k8s.io/kind/pkg/exec"
)

// CreateNetwork create a bridge network
func CreateNetwork(networkName string) error {
cmd := exec.Command(
"docker", "network",
"create",
"--driver=bridge",
"--label="+fmt.Sprintf("%s=%s", constants.ClusterLabelKey, networkName),
Copy link
Member

Choose a reason for hiding this comment

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

this package should not be aware of the cluster label key. this package just implements docker functionality

networkName,
tao12345666333 marked this conversation as resolved.
Show resolved Hide resolved
)
return cmd.Run()
}

// DeleteNetwork delete the special network
func DeleteNetwork(networkName string) error {
cmd := exec.Command(
"docker", "network",
"rm",
networkName,
)
return cmd.Run()
}

// IsNetworkExist check if the network exist
func IsNetworkExist(networkName string) bool {
cmd := exec.Command(
"docker", "network",
"inspect",
networkName,
)
if err := cmd.Run(); err != nil {
return false
}

return true
}