Skip to content

Commit 4245edd

Browse files
yakomyakom
authored
feat(#1570): introducing the --port-delete flag; (#1609)
* feat(#1570): introducing the --port-delete flag; * feat(#1570): small issues from the CR addressed; * feat(#1570): refactoring; NodeEditChangeset introduced; * feat(#1570): node edit --port-delete implemented; * feat(#1570): node edit E2E test extended;
1 parent 8ca0ccc commit 4245edd

File tree

9 files changed

+169
-49
lines changed

9 files changed

+169
-49
lines changed

‎cmd/cluster/clusterEdit.go‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ func NewCmdClusterEdit() *cobra.Command {
5757
// add subcommands
5858

5959
// add flags
60-
cmd.Flags().StringArray("port-add", nil, "[EXPERIMENTAL] Map ports from the node containers (via the serverlb) to the host (Format: `[HOST:][HOSTPORT:]CONTAINERPORT[/PROTOCOL][@NODEFILTER]`)\n - Example: `k3d node edit k3d-mycluster-serverlb --port-add 8080:80`")
60+
cmd.Flags().StringArray("port-add", nil, "Map ports from the node containers (via the serverlb) to the host (Format: `[HOST:][HOSTPORT:]CONTAINERPORT[/PROTOCOL][@NODEFILTER]`)\n - Example: `k3d cluster edit k3d-mycluster-serverlb --port-add 8080:80`")
61+
cmd.Flags().StringArray("port-delete", nil, "[EXPERIMENTAL] Delete a port mapping with the given format\nThe mapping spec needs to be exactly the same as the one used during creation\n - Example: `k3d cluster edit k3d-mycluster-serverlb --port-delete 8080:80`")
6162

6263
// done
6364
return cmd
@@ -76,18 +77,24 @@ func parseEditClusterCmd(cmd *cobra.Command, args []string) (*k3d.Cluster, *conf
7677
}
7778

7879
changeset := conf.SimpleConfig{}
80+
changeset.Ports = []conf.PortWithNodeFilters{}
7981

80-
/*
81-
* --port-add
82-
*/
83-
portFlags, err := cmd.Flags().GetStringArray("port-add")
84-
if err != nil {
85-
l.Log().Errorln(err)
86-
return nil, nil
82+
portsAdded := parsePortChangeFlag(cmd, &changeset, "port-add", false)
83+
84+
portsDeleted := parsePortChangeFlag(cmd, &changeset, "port-delete", true)
85+
86+
if portsAdded && portsDeleted {
87+
l.Log().Fatalln("Cannot combine port addition and deletion")
8788
}
8889

89-
// init portmap
90-
changeset.Ports = []conf.PortWithNodeFilters{}
90+
return existingCluster, &changeset
91+
}
92+
93+
func parsePortChangeFlag(cmd *cobra.Command, changeset *conf.SimpleConfig, flagName string, isRemoval bool) bool {
94+
portFlags, err := cmd.Flags().GetStringArray(flagName)
95+
if err != nil {
96+
l.Log().Fatalln(err)
97+
}
9198

9299
portFilterMap := make(map[string][]string, 1)
93100
for _, portFlag := range portFlags {
@@ -104,15 +111,14 @@ func parseEditClusterCmd(cmd *cobra.Command, args []string) (*k3d.Cluster, *conf
104111
portFilterMap[portmap] = filters
105112
}
106113
}
114+
l.Log().Tracef("PortFilterMap: %+v", portFilterMap)
107115

108116
for port, nodeFilters := range portFilterMap {
109117
changeset.Ports = append(changeset.Ports, conf.PortWithNodeFilters{
110118
Port: port,
111119
NodeFilters: nodeFilters,
120+
Removal: isRemoval,
112121
})
113122
}
114-
115-
l.Log().Tracef("PortFilterMap: %+v", portFilterMap)
116-
117-
return existingCluster, &changeset
123+
return len(portFilterMap) > 0
118124
}

‎cmd/node/nodeEdit.go‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,15 @@ func NewCmdNodeEdit() *cobra.Command {
5858

5959
// add flags
6060
cmd.Flags().StringArray("port-add", nil, "[EXPERIMENTAL] (serverlb only!) Map ports from the node container to the host (Format: `[HOST:][HOSTPORT:]CONTAINERPORT[/PROTOCOL][@NODEFILTER]`)\n - Example: `k3d node edit k3d-mycluster-serverlb --port-add 8080:80`")
61+
cmd.Flags().StringArray("port-delete", nil, "[EXPERIMENTAL] (serverlb only!) Remove port mappings between a node and the host (Format: `[HOST:][HOSTPORT:]CONTAINERPORT[/PROTOCOL][@NODEFILTER]`)\n - Example: `k3d node edit k3d-mycluster-serverlb --port-delete 8080:80`")
6162

6263
// done
6364
return cmd
6465
}
6566

6667
// parseEditNodeCmd parses the command input into variables required to delete nodes
67-
func parseEditNodeCmd(cmd *cobra.Command, args []string) (*k3d.Node, *k3d.Node) {
68+
func parseEditNodeCmd(cmd *cobra.Command, args []string) (*k3d.Node, *client.NodeEditChangeset) {
69+
6870
existingNode, err := client.NodeGet(cmd.Context(), runtimes.SelectedRuntime, &k3d.Node{Name: args[0]})
6971
if err != nil {
7072
l.Log().Fatalln(err)
@@ -79,19 +81,26 @@ func parseEditNodeCmd(cmd *cobra.Command, args []string) (*k3d.Node, *k3d.Node)
7981
l.Log().Fatalln("Currently only the loadbalancer can be updated!")
8082
}
8183

82-
changeset := &k3d.Node{}
84+
changeset := &client.NodeEditChangeset{}
85+
changeset.Ports = make(map[nat.Port][]client.NodeEditPortBinding)
8386

84-
/*
85-
* --port-add
86-
*/
87-
portFlags, err := cmd.Flags().GetStringArray("port-add")
88-
if err != nil {
89-
l.Log().Errorln(err)
90-
return nil, nil
87+
portsAdded := parsePortChangeFlag(cmd, changeset, "port-add", false)
88+
89+
portsDeleted := parsePortChangeFlag(cmd, changeset, "port-delete", true)
90+
91+
if portsAdded && portsDeleted {
92+
l.Log().Fatalln("Cannot combine port addition and deletion")
9193
}
9294

93-
// init portmap
94-
changeset.Ports = nat.PortMap{}
95+
return existingNode, changeset
96+
}
97+
98+
func parsePortChangeFlag(cmd *cobra.Command, changeset *client.NodeEditChangeset, flagName string, isRemoval bool) bool {
99+
100+
portFlags, err := cmd.Flags().GetStringArray(flagName)
101+
if err != nil {
102+
l.Log().Fatalln(err)
103+
}
95104

96105
for _, flag := range portFlags {
97106
portmappings, err := nat.ParsePortSpec(flag)
@@ -100,9 +109,10 @@ func parseEditNodeCmd(cmd *cobra.Command, args []string) (*k3d.Node, *k3d.Node)
100109
}
101110

102111
for _, pm := range portmappings {
103-
changeset.Ports[pm.Port] = append(changeset.Ports[pm.Port], pm.Binding)
112+
changeset.Ports[pm.Port] = append(changeset.Ports[pm.Port],
113+
client.NodeEditPortBinding{PortBinding: pm.Binding, RemovalFlag: isRemoval})
104114
}
105115
}
106116

107-
return existingNode, changeset
117+
return len(portFlags) > 0
108118
}

‎pkg/client/cluster.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1239,7 +1239,7 @@ func ClusterEditChangesetSimple(ctx context.Context, runtime k3drt.Runtime, clus
12391239
// 2. transform
12401240
cluster.ServerLoadBalancer = lbChangeset // we're working with pointers, so let's point to the changeset here to not update the original that we keep as a reference
12411241
if err := TransformPorts(ctx, runtime, cluster, changeset.Ports); err != nil {
1242-
return fmt.Errorf("error transforming port config %s: %w", changeset.Ports, err)
1242+
return fmt.Errorf("error transforming port config %+v: %w", changeset.Ports, err)
12431243
}
12441244
}
12451245

‎pkg/client/loadbalancer.go‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
l "github.com/k3d-io/k3d/v5/pkg/logger"
4040
"github.com/k3d-io/k3d/v5/pkg/runtimes"
4141
k3d "github.com/k3d-io/k3d/v5/pkg/types"
42+
"github.com/k3d-io/k3d/v5/pkg/util"
4243
)
4344

4445
var (
@@ -228,7 +229,7 @@ func loadbalancerAddPortConfigs(loadbalancer *k3d.Loadbalancer, portmapping nat.
228229
nodenames := []string{}
229230
for _, node := range targetNodes {
230231
if node.Role == k3d.LoadBalancerRole {
231-
return fmt.Errorf("cannot add port config referencing the loadbalancer itself (loop)")
232+
return fmt.Errorf("cannot add a port config referencing the loadbalancer itself (loop)")
232233
}
233234
nodenames = append(nodenames, node.Name)
234235
}
@@ -241,8 +242,8 @@ func loadbalancerAddPortConfigs(loadbalancer *k3d.Loadbalancer, portmapping nat.
241242

242243
nodenameLoop:
243244
for _, nodename := range nodenames {
244-
for _, existingNames := range loadbalancer.Config.Ports[portconfig] {
245-
if nodename == existingNames {
245+
for _, existingName := range loadbalancer.Config.Ports[portconfig] {
246+
if nodename == existingName {
246247
continue nodenameLoop
247248
}
248249
loadbalancer.Config.Ports[portconfig] = append(loadbalancer.Config.Ports[portconfig], nodename)
@@ -251,3 +252,29 @@ nodenameLoop:
251252

252253
return nil
253254
}
255+
256+
func loadbalancerRemovePortConfigs(loadbalancer *k3d.Loadbalancer, portmapping nat.PortMapping, targetNodes []*k3d.Node) error {
257+
portconfig := fmt.Sprintf("%s.%s", portmapping.Port.Port(), portmapping.Port.Proto())
258+
// entry for that port doesn't exist
259+
if _, ok := loadbalancer.Config.Ports[portconfig]; !ok {
260+
return nil
261+
}
262+
263+
nodenames := []string{}
264+
for _, node := range targetNodes {
265+
if node.Role == k3d.LoadBalancerRole {
266+
return fmt.Errorf("cannot have a port config referencing the loadbalancer itself (loop)")
267+
}
268+
nodenames = append(nodenames, node.Name)
269+
}
270+
271+
for _, nodename := range nodenames {
272+
loadbalancer.Config.Ports[portconfig] = util.RemoveFirst(loadbalancer.Config.Ports[portconfig], nodename)
273+
if 0 == len(loadbalancer.Config.Ports[portconfig]) {
274+
// deleting the empty map entry to get rid of an invalid Docker-level port mapping it leaves
275+
delete(loadbalancer.Config.Ports, portconfig)
276+
}
277+
}
278+
279+
return nil
280+
}

‎pkg/client/node.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -933,8 +933,17 @@ nodeLoop:
933933
return resultList
934934
}
935935

936+
type NodeEditPortBinding struct {
937+
nat.PortBinding
938+
RemovalFlag bool
939+
}
940+
941+
type NodeEditChangeset struct {
942+
Ports map[nat.Port][]NodeEditPortBinding
943+
}
944+
936945
// NodeEdit let's you update an existing node
937-
func NodeEdit(ctx context.Context, runtime runtimes.Runtime, existingNode, changeset *k3d.Node) error {
946+
func NodeEdit(ctx context.Context, runtime runtimes.Runtime, existingNode *k3d.Node, changeset *NodeEditChangeset) error {
938947
/*
939948
* Make a deep copy of the existing node
940949
*/
@@ -955,15 +964,29 @@ func NodeEdit(ctx context.Context, runtime runtimes.Runtime, existingNode, chang
955964
for port, portbindings := range changeset.Ports {
956965
loopChangesetPortbindings:
957966
for _, portbinding := range portbindings {
958-
// loop over existing portbindings to avoid port collisions (docker doesn't check for it)
959-
for _, existingPB := range result.Ports[port] {
960-
if util.IsPortBindingEqual(portbinding, existingPB) { // also matches on "equal" HostIPs (127.0.0.1, "", 0.0.0.0)
961-
l.Log().Tracef("Skipping existing PortBinding: %+v", existingPB)
967+
for i, existingPB := range result.Ports[port] {
968+
if util.IsPortBindingEqual(portbinding.PortBinding, existingPB) { // also matches on "equal" HostIPs (127.0.0.1, "", 0.0.0.0)
969+
if portbinding.RemovalFlag {
970+
// the mapping exists, needs to be removed
971+
l.Log().Tracef("Removing PortBinding: %+v", existingPB)
972+
result.Ports[port] = util.RemoveByIndex(result.Ports[port], i)
973+
if 0 == len(result.Ports[port]) {
974+
// deleting the empty map entry to get rid of an invalid Docker-level port mapping it leaves
975+
delete(result.Ports, port)
976+
}
977+
} else {
978+
// the mapping already exists, nothing to be added
979+
// Docker doesn't check for port collisions resulting from duplicates
980+
l.Log().Tracef("Skipping existing PortBinding: %+v", existingPB)
981+
}
962982
continue loopChangesetPortbindings
963983
}
964984
}
965-
l.Log().Tracef("Adding portbinding %+v for port %s", portbinding, port.Port())
966-
result.Ports[port] = append(result.Ports[port], portbinding)
985+
// the mapping doesn't exist already and we need to add it
986+
if !portbinding.RemovalFlag {
987+
l.Log().Tracef("Adding portbinding %+v for port %s", portbinding.PortBinding, port.Port())
988+
result.Ports[port] = append(result.Ports[port], portbinding.PortBinding)
989+
}
967990
}
968991
}
969992

‎pkg/client/ports.go‎

Lines changed: 40 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ func TransformPorts(ctx context.Context, runtime runtimes.Runtime, cluster *k3d.
6767
return err
6868
}
6969

70+
removalFlag := portWithNodeFilters.Removal
71+
7072
for suffix, nodes := range filteredNodes {
7173
// skip, if no nodes in filtered set, so we don't add portmappings with no targets in the backend
7274
if len(nodes) == 0 {
@@ -81,22 +83,18 @@ func TransformPorts(ctx context.Context, runtime runtimes.Runtime, cluster *k3d.
8183
if cluster.ServerLoadBalancer == nil {
8284
return fmt.Errorf("port-mapping of type 'proxy' specified, but loadbalancer is disabled")
8385
}
84-
if err := addPortMappings(cluster.ServerLoadBalancer.Node, portmappings); err != nil {
85-
return err
86-
}
86+
changePortMappings(cluster.ServerLoadBalancer.Node, portmappings, removalFlag)
8787
for _, pm := range portmappings {
88-
if err := loadbalancerAddPortConfigs(cluster.ServerLoadBalancer, pm, nodes); err != nil {
89-
return fmt.Errorf("error adding port config to loadbalancer: %w", err)
88+
if err := changeLBPortConfigs(cluster.ServerLoadBalancer, pm, nodes, removalFlag); err != nil {
89+
return fmt.Errorf("error modifying loadbalancer port config : %w", err)
9090
}
9191
}
9292
} else if suffix == "direct" {
9393
if len(nodes) > 1 {
9494
return fmt.Errorf("error: cannot apply a direct port-mapping (%s) to more than one node", portmappings)
9595
}
9696
for _, node := range nodes {
97-
if err := addPortMappings(node, portmappings); err != nil {
98-
return err
99-
}
97+
changePortMappings(node, portmappings, removalFlag)
10098
}
10199
} else if suffix != util.NodeFilterMapKeyAll {
102100
return fmt.Errorf("error adding port mappings: unknown suffix %s", suffix)
@@ -117,12 +115,44 @@ func TransformPorts(ctx context.Context, runtime runtimes.Runtime, cluster *k3d.
117115
return nil
118116
}
119117

120-
func addPortMappings(node *k3d.Node, portmappings []nat.PortMapping) error {
118+
func changeLBPortConfigs(loadbalancer *k3d.Loadbalancer, portmapping nat.PortMapping, targetNodes []*k3d.Node, remove bool) error {
119+
if remove {
120+
return loadbalancerRemovePortConfigs(loadbalancer, portmapping, targetNodes)
121+
} else {
122+
return loadbalancerAddPortConfigs(loadbalancer, portmapping, targetNodes)
123+
}
124+
}
125+
126+
func changePortMappings(node *k3d.Node, portmappings []nat.PortMapping, remove bool) {
127+
if remove {
128+
removePortMappings(node, portmappings)
129+
} else {
130+
addPortMappings(node, portmappings)
131+
}
132+
}
133+
134+
func addPortMappings(node *k3d.Node, portmappings []nat.PortMapping) {
121135
if node.Ports == nil {
122136
node.Ports = nat.PortMap{}
123137
}
124138
for _, pm := range portmappings {
125139
node.Ports[pm.Port] = append(node.Ports[pm.Port], pm.Binding)
126140
}
127-
return nil
141+
}
142+
143+
func removePortMappings(node *k3d.Node, portmappings []nat.PortMapping) {
144+
if node.Ports == nil {
145+
return
146+
}
147+
for _, pm := range portmappings {
148+
bindings, found := node.Ports[pm.Port]
149+
if !found {
150+
continue
151+
}
152+
node.Ports[pm.Port] = util.RemoveFirst(bindings, pm.Binding)
153+
if 0 == len(node.Ports[pm.Port]) {
154+
// deleting the empty map entry to get rid of an invalid Docker-level port mapping it leaves
155+
delete(node.Ports, pm.Port)
156+
}
157+
}
128158
}

‎pkg/config/v1alpha5/types.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ type VolumeWithNodeFilters struct {
6666
type PortWithNodeFilters struct {
6767
Port string `mapstructure:"port" json:"port,omitempty"`
6868
NodeFilters []string `mapstructure:"nodeFilters" json:"nodeFilters,omitempty"`
69+
Removal bool `mapstructure:"removal" json:"removal,omitempty"`
6970
}
7071

7172
type LabelWithNodeFilters struct {

‎pkg/util/util.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,16 @@ func ReplaceInAllElements(replacer *strings.Replacer, arr []string) []string {
3737
}
3838
return arr
3939
}
40+
41+
func RemoveByIndex[T comparable](slice []T, index int) []T {
42+
return append(slice[:index], slice[index+1:]...)
43+
}
44+
45+
func RemoveFirst[T comparable](slice []T, element T) []T {
46+
for i, v := range slice {
47+
if v == element {
48+
return RemoveByIndex(slice, i)
49+
}
50+
}
51+
return slice
52+
}

‎tests/test_node_edit.sh‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,17 @@ $EXE node edit k3d-$clustername-serverlb --port-add $existingPortMappingHostPort
3939

4040
info "Checking port-mappings..."
4141
docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$existingPortMappingContainerPort" || failed "failed to verify pre-existing port-mapping"
42-
docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$newPortMappingContainerPort" || failed "failed to verify pre-existing port-mapping"
42+
docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$newPortMappingContainerPort" || failed "failed to verify new port-mapping"
43+
44+
info "Checking cluster access..."
45+
check_clusters "$clustername" || failed "error checking cluster access"
46+
47+
info "Removing port-mapping from loadbalancer..."
48+
$EXE node edit k3d-$clustername-serverlb --port-delete $existingPortMappingHostPort:$existingPortMappingContainerPort || failed "failed to delete port-mapping from serverlb in $clustername"
49+
50+
info "Checking port-mappings..."
51+
docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qvE "^$existingPortMappingContainerPort" || failed "failed to verify deleted port-mapping"
52+
docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$newPortMappingContainerPort" || failed "failed to verify retained port-mapping"
4353

4454
info "Checking cluster access..."
4555
check_clusters "$clustername" || failed "error checking cluster access"

0 commit comments

Comments
 (0)