Repository: metallb/metallb
Pull request #2041 (2023-08-14 15:03:18)

Fixed    #2039 Allows for configmaptocrs to modify address pools and BFD profiles if…
… they would not have valid resource names once converted

Thanks for sending a pull request! A few things before we get started:

1. If this is your first time, please read the [contributing guide](https://metallb.universe.tf/community/#contributing)
:heavy_check_mark: 
2. For non-trivial pull requests, please [file an
   issue](https://github.com/metallb/metallb/issues/new) first, and get
   agreement that the change is a good idea, and a general guideline
   for how it should be implemented, before sending code. Large PRs
   that weren't first discussed and agreed upon in an issue won't be
   accepted.
https://github.com/metallb/metallb/issues/2039
3. If the PR fixes a particular bug, please include the words "Fixed
   #<issue number>" in the PR text, so that the bug auto-closes when
   the PR is merged.
Fixed  #2039


Sebastian-RG: @oribon Hi! If you have the time, could you approve the workflow so the automated tests can run?
oribon: > @oribon Hi! If you have the time, could you approve the workflow so the automated tests can run?

sorry was away for a while, done and will review it soon I hope
Sebastian-RG: Fixed an issue with how the `convertNames` flag was passed that was causing CI to fail. I'm not sure how to fix the [Implicit memory aliasing in for loop](https://github.com/metallb/metallb/actions/runs/5857332084/job/16059582956#step:4:162) one. I don't think I modified those lines.
Sebastian-RG: Sorry about the mess :sweat:, I'm new to Golang and i was checking for an empty list/string the wrong way. This should work now
fedepaol: > Sorry about the mess 😓, I'm new to Golang and i was checking for an empty list/string the wrong way. This should work now

no problem, and thanks a lot for the contribution! I will go over it soon
Sebastian-RG: At least this time the actual unit tests passed :smiling_face_with_tear:. I'll correct what the linter mentions in a minute.
Sebastian-RG: `gofmt -s` done
liornoy: @Sebastian-RG we just merged a fix for the static-security-analysis, so can you please rebase this PR?
Also, in order to have an organized and unified git log in this repo we have guidelines for the commit messages format.
The full details can be found in here: https://cbea.ms/git-commit/
But if you don't want to read all of it, the basics are: 
Have a title up to 50 characters, starting with a capital letter,
separate the title from body with a blank line
and wrap the body at 72 characters
liornoy: @fedepaol I think it's better to have this conversion happen by default. wdyt?
fedepaol: > @fedepaol I think it's better to have this conversion happen by default. wdyt?

+1 , the old version is useless is the generated name is not valid.

liornoy: @Sebastian-RG thanks for the fix, looks good, just left one comment from my side
Sebastian-RG: Regex added as requested
liornoy: lgtm, thanks!
fedepaol: sent to merge queue
Sebastian-RG: Fixed a golangci-lint error, sorry for not checking before pushing

Patch:
diff --git a/configmaptocrs/README.md b/configmaptocrs/README.md
index 231e1ef4..15eee23e 100644
--- a/configmaptocrs/README.md
+++ b/configmaptocrs/README.md
@@ -95,6 +95,90 @@ status: {}
 ---
 ```
 
+## Notes
+
+Since `v0.13.11`:
+Provided configmaps can contain resource names that are not compatible with 
+[K8S object names](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names) 
+such as underscores or upper-case characters. They will be modified respectively to a dash and the
+corresponding lowercase letter.
+
+### Example
+
+For this MetalLB configmap named config.yaml the address pool `My_Ip_Space` is not a valid 
+resource name in K8S:
+
+```yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  namespace: metallb-system
+  name: config
+data:
+  config: |
+    peers:
+    - my-asn: 64512
+      peer-asn: 64512
+      peer-address: 10.96.0.100
+    address-pools:
+    - name: My_Ip_Space
+      protocol: bgp
+      addresses:
+      - 198.51.100.0/24
+      bgp-advertisements:
+      - aggregation-length: 32
+        localpref: 100
+        communities:
+        - 64512:1234
+```
+
+The generator will generate a `resources.yaml` file that looks like:
+
+```yaml
+# This was autogenerated by MetalLB's custom resource generator.
+apiVersion: metallb.io/v1beta2
+kind: BGPPeer
+metadata:
+  creationTimestamp: null
+  name: peer1
+  namespace: metallb-system
+spec:
+  holdTime: 1m30s
+  keepaliveTime: 0s
+  myASN: 64512
+  passwordSecret: {}
+  peerASN: 64512
+  peerAddress: 10.96.0.100
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: IPAddressPool
+metadata:
+  creationTimestamp: null
+  name: my-ip-space
+  namespace: metallb-system
+spec:
+  addresses:
+  - 198.51.100.0/24
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BGPAdvertisement
+metadata:
+  creationTimestamp: null
+  name: bgpadvertisement1
+  namespace: metallb-system
+spec:
+  aggregationLength: 32
+  communities:
+  - 64512:1234
+  ipAddressPools:
+  - my-ip-space
+  localPref: 100
+status: {}
+---
+```
+
 ## Running directly against a cluster
 
 Configmaptocrs tool can also run directly against a cluster,
diff --git a/configmaptocrs/main.go b/configmaptocrs/main.go
index f307c316..7032e231 100644
--- a/configmaptocrs/main.go
+++ b/configmaptocrs/main.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strings"
 	"time"
@@ -83,6 +84,12 @@ func generate(w io.Writer, origin string) error {
 		return err
 	}
 
+	log.Println("Converting configmap resources to K8S-compliant names")
+	err = convertNamesToK8S(cf)
+	if err != nil {
+		return err
+	}
+
 	log.Println("Creating custom resources")
 	resources, err := resourcesFor(cf)
 	if err != nil {
@@ -149,6 +156,80 @@ func decodeConfigFile(raw []byte) (*configFile, error) {
 	return cf, nil
 }
 
+// convertNamesToK8S gets a configFile object and converts all names
+// in it to names compatible with K8S resources, if necessary.
+func convertNamesToK8S(cf *configFile) error {
+	var err error
+	var validName bool
+
+	r := regexp.MustCompile("^[a-z][-a-z0-9]{0,61}[a-z0-9]{1}$")
+
+	for i := 0; i < len(cf.Pools); i++ {
+		validName = r.MatchString(cf.Pools[i].Name)
+		if validName {
+			continue
+		}
+		cf.Pools[i].Name, err = formatToK8S(cf.Pools[i].Name, "IpAddressPool")
+		if err != nil {
+			return err
+		}
+	}
+
+	for i := 0; i < len(cf.BFDProfiles); i++ {
+		validName = r.MatchString(cf.BFDProfiles[i].Name)
+		if validName {
+			continue
+		}
+		cf.BFDProfiles[i].Name, err = formatToK8S(cf.BFDProfiles[i].Name, "BFDProfile")
+		if err != nil {
+			return err
+		}
+	}
+
+	for i := 0; i < len(cf.Peers); i++ {
+		if len(cf.Peers[i].BFDProfile) == 0 {
+			continue
+		}
+		validName = r.MatchString(cf.Peers[i].BFDProfile)
+		if validName {
+			continue
+		}
+		cf.Peers[i].BFDProfile, err = formatToK8S(cf.Peers[i].BFDProfile, "BFDProfile")
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func formatToK8S(name string, kind string) (string, error) {
+	var truncated string
+
+	if len(name) > 63 {
+		truncated = name[:63]
+	} else {
+		truncated = name
+	}
+
+	lowercase := strings.ToLower(truncated)
+	noUnderscore := strings.ReplaceAll(lowercase, "_", "-")
+
+	firstLetterRegex := regexp.MustCompile("[a-z]")
+	firstLetter := firstLetterRegex.FindStringIndex(noUnderscore)
+	if len(firstLetter) == 0 {
+		return "", fmt.Errorf("failed to make %s K8S compatible: %s", kind, name)
+	}
+	final := noUnderscore[firstLetter[0]:]
+
+	if strings.Compare(name, final) != 0 {
+		log.Printf("Changing %s name from: %s to: %s",
+			kind, name, final)
+	}
+
+	return final, nil
+}
+
 // getConfigMapData gets raw bytes representing a ConfigMap and returns the
 // data of the configmap.
 func getConfigMapData(raw []byte) ([]byte, error) {
diff --git a/website/content/configuration/_advanced_ipaddresspool_configuration.md b/website/content/configuration/_advanced_ipaddresspool_configuration.md
index ed505987..18c15853 100644
--- a/website/content/configuration/_advanced_ipaddresspool_configuration.md
+++ b/website/content/configuration/_advanced_ipaddresspool_configuration.md
@@ -84,14 +84,15 @@ matching with expression `key: app, operator: In, values: [bar]` created
 either in `namespace-a` or `namespace-b` or any namespace has a label
 `foo:bar`.
 
-Given a service, if multiple matching IPAddressPool are available MetalLB
-will check for the availability of IPs sorting the matching IPAddressPool
-by priority, starting from the highest to the lowest. A lower number for
-priority field equals a higher priority. If multiple IPAddressPool have
-the same priority, the choice will be random.
-When not specifying a priority / setting priority 0 is considered as lowest
-priority and will be used for assignment only if the pools with priority
-can't be used.
+If multiple `IPAddressPool` objects are available to a `Service`, MetalLB will
+check for the availability of IPs by sorting the matching `IPAddressPool`
+objects by priority.
+It will first select the `IPAddressPool` with the lowest `priority` number
+(i.e., `priority=1` is the highest priority).
+If the `priority` field is unset or set to `0`, it will have the lowest
+priority (i.e., it will be the last pool to be used).
+If multiple `IPAddressPool` objects have the same priority, the choice will be
+random.
 
 {{% notice note %}}
 When a service explicitly chooses an IPAddressPool via `metallb.universe.tf/address-pool`


Test patch:
diff --git a/configmaptocrs/testdata/configmap-data/config-data-convert-names.golden b/configmaptocrs/testdata/configmap-data/config-data-convert-names.golden
new file mode 100644
index 00000000..9bd7b7af
--- /dev/null
+++ b/configmaptocrs/testdata/configmap-data/config-data-convert-names.golden
@@ -0,0 +1,80 @@
+# This was autogenerated by MetalLB's custom resource generator.
+apiVersion: metallb.io/v1beta1
+kind: IPAddressPool
+metadata:
+  creationTimestamp: null
+  name: numbered-addresspool
+  namespace: metallb-system
+spec:
+  addresses:
+  - 198.51.100.0/24
+  - fc00:f853:0ccd:e799::/124
+  autoAssign: true
+  avoidBuggyIPs: true
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: IPAddressPool
+metadata:
+  creationTimestamp: null
+  name: capitalized-addresspool
+  namespace: metallb-system
+spec:
+  addresses:
+  - 192.168.1.240/28
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BGPAdvertisement
+metadata:
+  creationTimestamp: null
+  name: bgpadvertisement1
+  namespace: metallb-system
+spec:
+  ipAddressPools:
+  - numbered-addresspool
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: L2Advertisement
+metadata:
+  creationTimestamp: null
+  name: l2advertisement1
+  namespace: metallb-system
+spec:
+  ipAddressPools:
+  - capitalized-addresspool
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BFDProfile
+metadata:
+  creationTimestamp: null
+  name: underscored-bfd-profile
+  namespace: metallb-system
+spec:
+  detectMultiplier: 200
+  echoInterval: 62
+  echoMode: false
+  minimumTtl: 254
+  passiveMode: false
+  receiveInterval: 280
+  transmitInterval: 270
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BFDProfile
+metadata:
+  creationTimestamp: null
+  name: toolongbfdprofilename01-toolongbfdprofilename02-toolongbfdprofi
+  namespace: metallb-system
+spec:
+  detectMultiplier: 150
+  echoInterval: 80
+  echoMode: true
+  minimumTtl: 150
+  passiveMode: true
+  receiveInterval: 150
+  transmitInterval: 150
+status: {}
+---
diff --git a/configmaptocrs/testdata/configmap-data/config-data-convert-names.yaml b/configmaptocrs/testdata/configmap-data/config-data-convert-names.yaml
new file mode 100644
index 00000000..deae613a
--- /dev/null
+++ b/configmaptocrs/testdata/configmap-data/config-data-convert-names.yaml
@@ -0,0 +1,29 @@
+address-pools:
+- name: 01-numbered-addresspool
+  protocol: bgp
+  addresses:
+  - 198.51.100.0/24
+  - fc00:f853:0ccd:e799::/124
+  avoid-buggy-ips: true
+  auto-assign: true
+- name: Capitalized-AddressPool
+  protocol: layer2
+  addresses:
+  - 192.168.1.240/28
+bfd-profiles:
+- name: underscored_bfd_profile
+  receive-interval: 280
+  transmit-interval: 270
+  echo-interval: 62
+  echo-mode: false
+  passive-mode: false
+  minimum-ttl: 254
+  detect-multiplier: 200
+- name: tooLongBFDProfileName01-tooLongBFDProfileName02-tooLongBFDProfileName03-tooLongBFDProfileName04
+  receive-interval: 150
+  transmit-interval: 150
+  echo-interval: 80
+  echo-mode: true
+  passive-mode: true
+  minimum-ttl: 150
+  detect-multiplier: 150
diff --git a/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.golden b/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.golden
new file mode 100644
index 00000000..9bd7b7af
--- /dev/null
+++ b/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.golden
@@ -0,0 +1,80 @@
+# This was autogenerated by MetalLB's custom resource generator.
+apiVersion: metallb.io/v1beta1
+kind: IPAddressPool
+metadata:
+  creationTimestamp: null
+  name: numbered-addresspool
+  namespace: metallb-system
+spec:
+  addresses:
+  - 198.51.100.0/24
+  - fc00:f853:0ccd:e799::/124
+  autoAssign: true
+  avoidBuggyIPs: true
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: IPAddressPool
+metadata:
+  creationTimestamp: null
+  name: capitalized-addresspool
+  namespace: metallb-system
+spec:
+  addresses:
+  - 192.168.1.240/28
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BGPAdvertisement
+metadata:
+  creationTimestamp: null
+  name: bgpadvertisement1
+  namespace: metallb-system
+spec:
+  ipAddressPools:
+  - numbered-addresspool
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: L2Advertisement
+metadata:
+  creationTimestamp: null
+  name: l2advertisement1
+  namespace: metallb-system
+spec:
+  ipAddressPools:
+  - capitalized-addresspool
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BFDProfile
+metadata:
+  creationTimestamp: null
+  name: underscored-bfd-profile
+  namespace: metallb-system
+spec:
+  detectMultiplier: 200
+  echoInterval: 62
+  echoMode: false
+  minimumTtl: 254
+  passiveMode: false
+  receiveInterval: 280
+  transmitInterval: 270
+status: {}
+---
+apiVersion: metallb.io/v1beta1
+kind: BFDProfile
+metadata:
+  creationTimestamp: null
+  name: toolongbfdprofilename01-toolongbfdprofilename02-toolongbfdprofi
+  namespace: metallb-system
+spec:
+  detectMultiplier: 150
+  echoInterval: 80
+  echoMode: true
+  minimumTtl: 150
+  passiveMode: true
+  receiveInterval: 150
+  transmitInterval: 150
+status: {}
+---
diff --git a/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.yaml b/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.yaml
new file mode 100644
index 00000000..4b93556c
--- /dev/null
+++ b/configmaptocrs/testdata/configmap-yaml/config-yaml-convert-names.yaml
@@ -0,0 +1,36 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  namespace: metallb-system
+  name: config
+data:
+  config: |
+    address-pools:
+    - name: 01-numbered-addresspool
+      protocol: bgp
+      addresses:
+      - 198.51.100.0/24
+      - fc00:f853:0ccd:e799::/124
+      avoid-buggy-ips: true
+      auto-assign: true
+    - name: Capitalized-AddressPool
+      protocol: layer2
+      addresses:
+      - 192.168.1.240/28
+    bfd-profiles:
+    - name: underscored_bfd_profile
+      receive-interval: 280
+      transmit-interval: 270
+      echo-interval: 62
+      echo-mode: false
+      passive-mode: false
+      minimum-ttl: 254
+      detect-multiplier: 200
+    - name: tooLongBFDProfileName01-tooLongBFDProfileName02-tooLongBFDProfileName03-tooLongBFDProfileName04
+      receive-interval: 150
+      transmit-interval: 150
+      echo-interval: 80
+      echo-mode: true
+      passive-mode: true
+      minimum-ttl: 150
+      detect-multiplier: 150