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

ensure ingress-nginx node label is applied #377

Merged
merged 2 commits into from
Aug 29, 2024
Merged

ensure ingress-nginx node label is applied #377

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
9beb84a
ensure ingress-nginx node label is applied
nabuskey Aug 29, 2024
4891f53
fix typto
nabuskey Aug 29, 2024
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
44 changes: 41 additions & 3 deletions pkg/kind/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ import (
"sigs.k8s.io/yaml"
)

const (
ingressNginxNodeLabelKey = "ingress-ready"
ingressNginxNodeLabelValue = "true"
)

var (
setupLog = log.Log.WithName("setup")
)
Expand Down Expand Up @@ -241,28 +246,61 @@ func (c *Cluster) ensureCorrectConfig(in []byte) (kindv1alpha4.Cluster, error) {
if err != nil {
return kindv1alpha4.Cluster{}, fmt.Errorf("parsing kind config: %w", err)
}

// the port and ingress-nginx label must be on the same node to ensure nginx runs on the node with the right port.
appendNecessaryPort := true
appendIngresNodeLabel := true
nabuskey marked this conversation as resolved.
Show resolved Hide resolved
// pick the first node for the ingress-nginx if we need to configure node port.
nodePosition := 0

if parsedCluster.Nodes == nil || len(parsedCluster.Nodes) == 0 {
return kindv1alpha4.Cluster{}, fmt.Errorf("provided kind config does not have the node field defined")
}

nodes:
for i := range parsedCluster.Nodes {
node := parsedCluster.Nodes[i]
for _, pm := range node.ExtraPortMappings {
if strconv.Itoa(int(pm.HostPort)) == c.cfg.Port {
appendNecessaryPort = false
nodePosition = i
if node.Labels != nil {
v, ok := node.Labels[ingressNginxNodeLabelKey]
if ok && v == ingressNginxNodeLabelValue {
appendIngresNodeLabel = false
}
}
break nodes
}
}
if node.Labels != nil {
v, ok := node.Labels[ingressNginxNodeLabelKey]
if ok && v == ingressNginxNodeLabelValue {
appendIngresNodeLabel = false
nodePosition = i
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this can be further simplified. I am not fond of label breaks in go, but leave it up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's really just comes down to last one winning or the first one winning. I chose first one winning. If we do last one wins, we can avoid using label breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

i like first one winning too.

Here is an alternative way of writing it (thanks ChatGPT). I dont think it is any easier tho. let's leave it.

for i, node := range parsedCluster.Nodes {
    checkNodeLabels := func() {
        if node.Labels != nil {
            if v, ok := node.Labels[ingressNginxNodeLabelKey]; ok && v == ingressNginxNodeLabelValue {
                appendIngresNodeLabel = false
            }
        }
    }

    for _, pm := range node.ExtraPortMappings {
        if strconv.Itoa(int(pm.HostPort)) == c.cfg.Port {
            appendNecessaryPort = false
            nodePosition = i
            checkNodeLabels()
            break
        }
    }

    if appendNecessaryPort { // If the port was not matched, check labels
        checkNodeLabels()
        if !appendIngresNodeLabel {
            nodePosition = i
            break
        }
    }
}

break nodes
}
}
}

if appendNecessaryPort && len(parsedCluster.Nodes) != 0 {
if appendNecessaryPort {
hp, err := strconv.Atoi(c.cfg.Port)
if err != nil {
return kindv1alpha4.Cluster{}, fmt.Errorf("converting port, %s, to int: %w", c.cfg.Port, err)
}
// either "80" or "443". No need to check for err
cp, _ := strconv.Atoi(containerPort)

parsedCluster.Nodes[0].ExtraPortMappings = append(parsedCluster.Nodes[0].ExtraPortMappings, kindv1alpha4.PortMapping{ContainerPort: int32(cp), HostPort: int32(hp)})
if parsedCluster.Nodes[nodePosition].ExtraPortMappings == nil {
parsedCluster.Nodes[nodePosition].ExtraPortMappings = make([]kindv1alpha4.PortMapping, 0, 1)
}
parsedCluster.Nodes[nodePosition].ExtraPortMappings =
append(parsedCluster.Nodes[nodePosition].ExtraPortMappings, kindv1alpha4.PortMapping{ContainerPort: int32(cp), HostPort: int32(hp), Protocol: "TCP"})
}
if appendIngresNodeLabel {
if parsedCluster.Nodes[nodePosition].Labels == nil {
parsedCluster.Nodes[nodePosition].Labels = make(map[string]string)
}
parsedCluster.Nodes[nodePosition].Labels[ingressNginxNodeLabelKey] = ingressNginxNodeLabelValue
}

return parsedCluster, nil
Expand Down
38 changes: 29 additions & 9 deletions pkg/kind/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,37 +106,57 @@ func TestGetConfigCustom(t *testing.T) {
inputPath string
outputPath string
hostPort string
Protocol string
protocol string
error bool
}

cases := []testCase{
{
inputPath: "testdata/no-necessary-port.yaml",
outputPath: "testdata/expected/no-necessary-port.yaml",
inputPath: "testdata/no-port.yaml",
outputPath: "testdata/expected/no-port.yaml",
hostPort: "8443",
Protocol: "https",
protocol: "https",
},
{
inputPath: "testdata/necessary-port-present.yaml",
outputPath: "testdata/expected/necessary-port-present.yaml",
inputPath: "testdata/port-only.yaml",
outputPath: "testdata/expected/port-only.yaml",
hostPort: "80",
Protocol: "http",
protocol: "http",
},
{
inputPath: "testdata/no-port-multi.yaml",
outputPath: "testdata/expected/no-port-multi.yaml",
hostPort: "8443",
protocol: "https",
},
{
inputPath: "testdata/label-only.yaml",
outputPath: "testdata/expected/label-only.yaml",
hostPort: "8443",
protocol: "https",
},
{
inputPath: "testdata/no-node",
error: true,
},
}

for _, v := range cases {
c, _ := NewCluster("testcase", "v1.26.3", "", v.inputPath, "", util.CorePackageTemplateConfig{
Host: "cnoe.localtest.me",
Port: v.hostPort,
Protocol: v.Protocol,
Protocol: v.protocol,
})

b, err := c.getConfig()
if v.error {
assert.Error(t, err)
continue
}
assert.NoError(t, err)
expected, _ := os.ReadFile(v.outputPath)
assert.YAMLEq(t, string(expected), string(b))
}

}

// Mock provider for testing
Expand Down
21 changes: 21 additions & 0 deletions pkg/kind/testdata/expected/label-only.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
networking: {}
nodes:
- role: control-plane
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- role: worker
image: "abc"
labels:
ingress-ready: "true"
extraPortMappings:
- containerPort: 443
hostPort: 8443
protocol: TCP

19 changes: 19 additions & 0 deletions pkg/kind/testdata/expected/no-port-multi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
networking: {}
nodes:
- role: control-plane
labels:
ingress-ready: "true"
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- containerPort: 443
hostPort: 8443
protocol: TCP
- role: worker
image: "abc"
5 changes: 4 additions & 1 deletion .../testdata/expected/no-necessary-port.yaml → pkg/kind/testdata/expected/no-port.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ apiVersion: kind.x-k8s.io/v1alpha4
networking: {}
nodes:
- role: control-plane
labels:
ingress-ready: "true"
extraMounts:
- containerPath: /var/lib/kubelet/config.json
hostPath: ~/.docker/config.json
Expand All @@ -14,4 +16,5 @@ nodes:
- containerPort: 31333
hostPort: 31333
- containerPort: 443
hostPort: 8443
hostPort: 8443
protocol: TCP
8 changes: 5 additions & 3 deletions ...data/expected/necessary-port-present.yaml → pkg/kind/testdata/expected/port-only.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@ apiVersion: kind.x-k8s.io/v1alpha4
networking: {}
nodes:
- role: control-plane
extraMounts:
- containerPath: /var/lib/kubelet/config.json
hostPath: ~/.docker/config.json
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- role: worker
labels:
ingress-ready: "true"
extraPortMappings:
- containerPort: 80
hostPort: 80
protocol: TCP
15 changes: 15 additions & 0 deletions pkg/kind/testdata/label-only.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- role: worker
image: "abc"
labels:
ingress-ready: "true"
2 changes: 2 additions & 0 deletions pkg/kind/testdata/no-node.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
13 changes: 13 additions & 0 deletions pkg/kind/testdata/no-port-multi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- role: worker
image: "abc"
0 pkg/kind/testdata/no-necessary-port.yaml → pkg/kind/testdata/no-port.yaml
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions ...kind/testdata/necessary-port-present.yaml → pkg/kind/testdata/port-only.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
extraMounts:
- containerPath: /var/lib/kubelet/config.json
hostPath: ~/.docker/config.json
extraPortMappings:
- containerPort: 31337
hostPort: 31337
- containerPort: 31340
hostPort: 31340
- containerPort: 31333
hostPort: 31333
- role: worker
extraPortMappings:
- containerPort: 80
hostPort: 80
protocol: TCP
Loading