From c22d357f2075401e3a844ebbdb2104114c1e14ff Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Aug 2024 15:18:33 +0200 Subject: [PATCH 1/9] operator: move label templating functions to separate file no code changes, just label templating functions moved to labeltmpl.go. Signed-off-by: Francesco Giudici --- pkg/server/api_registration.go | 162 ---------------------------- pkg/server/labeltmpl.go | 187 +++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 162 deletions(-) create mode 100644 pkg/server/labeltmpl.go diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index eed8a161..52f5691d 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -24,16 +24,13 @@ import ( "net/http" "path" "regexp" - "strings" "github.com/gorilla/websocket" - values "github.com/rancher/wrangler/v2/pkg/data" "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" - "github.com/rancher/elemental-operator/pkg/hostinfo" "github.com/rancher/elemental-operator/pkg/log" "github.com/rancher/elemental-operator/pkg/register" ) @@ -217,34 +214,6 @@ func (i *InventoryServer) getRancherCACert() string { return cacert } -func replaceStringData(data map[string]interface{}, name string) (string, error) { - str := name - result := &strings.Builder{} - for { - i := strings.Index(str, "${") - if i == -1 { - result.WriteString(str) - break - } - j := strings.Index(str[i:], "}") - if j == -1 { - result.WriteString(str) - break - } - - result.WriteString(str[:i]) - obj := values.GetValueN(data, strings.Split(str[i+2:j+i], "/")...) - if str, ok := obj.(string); ok { - result.WriteString(str) - } else { - return "", errValueNotFound - } - str = str[j+i+1:] - } - - return result.String(), nil -} - func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration) error { protoVersion := register.MsgUndefined @@ -381,137 +350,6 @@ func decodeProtocolVersion(data []byte) (register.MessageType, error) { return protoVersion, nil } -func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { - annotations := map[string]string{} - if err := json.Unmarshal(data, &annotations); err != nil { - return err - } - log.Debug("Adding annotations from client data") - if mInventory.Annotations == nil { - mInventory.Annotations = map[string]string{} - } - for key, val := range annotations { - mInventory.Annotations[fmt.Sprintf("elemental.cattle.io/%s", sanitizeUserInput(key))] = sanitizeUserInput(val) - } - return nil -} - -// updateInventoryFromSMBIOSData() updates mInventory Name and Labels from the MachineRegistration and the SMBIOS data -func updateInventoryFromSMBIOSData(data []byte, mInventory *elementalv1.MachineInventory, mRegistration *elementalv1.MachineRegistration) error { - smbiosData := map[string]interface{}{} - if err := json.Unmarshal(data, &smbiosData); err != nil { - return err - } - // Sanitize any lower dashes into dashes as hostnames cannot have lower dashes, and we use the inventory name - // to set the machine hostname. Also set it to lowercase - name, err := replaceStringData(smbiosData, mInventory.Name) - if err == nil { - name = sanitizeStringHostname(name) - mInventory.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) - } else { - if errors.Is(err, errValueNotFound) { - // value not found, will be set in updateInventoryFromSystemData - log.Warningf("SMBIOS Value not found: %v", mInventory.Name) - } else { - return err - } - } - - log.Debugf("Adding labels from registration") - // Add extra label info from data coming from smbios and based on the registration data - if mInventory.Labels == nil { - mInventory.Labels = map[string]string{} - } - for k, v := range mRegistration.Spec.MachineInventoryLabels { - parsedData, err := replaceStringData(smbiosData, v) - if err != nil { - if errors.Is(err, errValueNotFound) { - log.Debugf("Value not found: %v", v) - continue - } - log.Errorf("Failed parsing smbios data: %v", err.Error()) - return err - } - parsedData = sanitizeString(parsedData) - - log.Debugf("Parsed %s into %s with smbios data, setting it to label %s", v, parsedData, k) - mInventory.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") - } - return nil -} - -// updateInventoryFromSystemDataNG receives digested hardware labels from the client -func updateInventoryFromSystemDataNG(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - labels := map[string]interface{}{} - - if err := json.Unmarshal(data, &labels); err != nil { - return fmt.Errorf("unmarshalling system data labels payload: %w", err) - } - - return sanitizeSystemDataLabels(labels, inv, reg) -} - -// Deprecated. Remove me together with 'MsgSystemData' type. -// updateInventoryFromSystemData creates labels in the inventory based on the hardware information -func updateInventoryFromSystemData(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - log.Infof("Adding labels from system data") - - labels, err := hostinfo.FillData(data) - if err != nil { - return err - } - - return sanitizeSystemDataLabels(labels, inv, reg) -} - -func sanitizeSystemDataLabels(labels map[string]interface{}, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - // Also available but not used: - // systemData.Product -> name, vendor, serial,uuid,sku,version. Kind of smbios data - // systemData.BIOS -> info about the bios. Useless IMO - // systemData.Baseboard -> asset, serial, vendor,version,product. Kind of useless? - // systemData.Chassis -> asset, serial, vendor,version,product, type. Maybe be useful depending on the provider. - // systemData.Topology -> CPU/memory and cache topology. No idea if useful. - - name, err := replaceStringData(labels, inv.Name) - if err != nil { - if errors.Is(err, errValueNotFound) { - log.Warningf("System data value not found: %v", inv.Name) - name = "m" - } else { - return err - } - } - name = sanitizeStringHostname(name) - - inv.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) - - log.Debugf("Parsing labels from System Data") - - if inv.Labels == nil { - inv.Labels = map[string]string{} - } - - for k, v := range reg.Spec.MachineInventoryLabels { - log.Debugf("Parsing: %v : %v", k, v) - - parsedData, err := replaceStringData(labels, v) - if err != nil { - if errors.Is(err, errValueNotFound) { - log.Debugf("Value not found: %v", v) - continue - } - log.Errorf("Failed parsing system data: %v", err.Error()) - return err - } - parsedData = sanitizeString(parsedData) - - log.Debugf("Parsed %s into %s with system data, setting it to label %s", v, parsedData, k) - inv.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") - } - - return nil -} - // sanitizeString will sanitize a given string by: // replacing all invalid chars as set on the sanitize regex by dashes // removing any double dashes resulted from the above method diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go new file mode 100644 index 00000000..bf96b378 --- /dev/null +++ b/pkg/server/labeltmpl.go @@ -0,0 +1,187 @@ +/* +Copyright © 2022 - 2024 SUSE LLC + +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 server + +import ( + "encoding/json" + "errors" + "fmt" + "strings" + + elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" + "github.com/rancher/elemental-operator/pkg/hostinfo" + "github.com/rancher/elemental-operator/pkg/log" + values "github.com/rancher/wrangler/v2/pkg/data" +) + +func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { + annotations := map[string]string{} + if err := json.Unmarshal(data, &annotations); err != nil { + return err + } + log.Debug("Adding annotations from client data") + if mInventory.Annotations == nil { + mInventory.Annotations = map[string]string{} + } + for key, val := range annotations { + mInventory.Annotations[fmt.Sprintf("elemental.cattle.io/%s", sanitizeUserInput(key))] = sanitizeUserInput(val) + } + return nil +} + +// updateInventoryFromSMBIOSData() updates mInventory Name and Labels from the MachineRegistration and the SMBIOS data +func updateInventoryFromSMBIOSData(data []byte, mInventory *elementalv1.MachineInventory, mRegistration *elementalv1.MachineRegistration) error { + smbiosData := map[string]interface{}{} + if err := json.Unmarshal(data, &smbiosData); err != nil { + return err + } + // Sanitize any lower dashes into dashes as hostnames cannot have lower dashes, and we use the inventory name + // to set the machine hostname. Also set it to lowercase + name, err := replaceStringData(smbiosData, mInventory.Name) + if err == nil { + name = sanitizeStringHostname(name) + mInventory.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) + } else { + if errors.Is(err, errValueNotFound) { + // value not found, will be set in updateInventoryFromSystemData + log.Warningf("SMBIOS Value not found: %v", mInventory.Name) + } else { + return err + } + } + + log.Debugf("Adding labels from registration") + // Add extra label info from data coming from smbios and based on the registration data + if mInventory.Labels == nil { + mInventory.Labels = map[string]string{} + } + for k, v := range mRegistration.Spec.MachineInventoryLabels { + parsedData, err := replaceStringData(smbiosData, v) + if err != nil { + if errors.Is(err, errValueNotFound) { + log.Debugf("Value not found: %v", v) + continue + } + log.Errorf("Failed parsing smbios data: %v", err.Error()) + return err + } + parsedData = sanitizeString(parsedData) + + log.Debugf("Parsed %s into %s with smbios data, setting it to label %s", v, parsedData, k) + mInventory.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") + } + return nil +} + +// updateInventoryFromSystemDataNG receives digested hardware labels from the client +func updateInventoryFromSystemDataNG(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { + labels := map[string]interface{}{} + + if err := json.Unmarshal(data, &labels); err != nil { + return fmt.Errorf("unmarshalling system data labels payload: %w", err) + } + + return sanitizeSystemDataLabels(labels, inv, reg) +} + +// Deprecated. Remove me together with 'MsgSystemData' type. +// updateInventoryFromSystemData creates labels in the inventory based on the hardware information +func updateInventoryFromSystemData(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { + log.Infof("Adding labels from system data") + + labels, err := hostinfo.FillData(data) + if err != nil { + return err + } + + return sanitizeSystemDataLabels(labels, inv, reg) +} + +func sanitizeSystemDataLabels(labels map[string]interface{}, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { + // Also available but not used: + // systemData.Product -> name, vendor, serial,uuid,sku,version. Kind of smbios data + // systemData.BIOS -> info about the bios. Useless IMO + // systemData.Baseboard -> asset, serial, vendor,version,product. Kind of useless? + // systemData.Chassis -> asset, serial, vendor,version,product, type. Maybe be useful depending on the provider. + // systemData.Topology -> CPU/memory and cache topology. No idea if useful. + + name, err := replaceStringData(labels, inv.Name) + if err != nil { + if errors.Is(err, errValueNotFound) { + log.Warningf("System data value not found: %v", inv.Name) + name = "m" + } else { + return err + } + } + name = sanitizeStringHostname(name) + + inv.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) + + log.Debugf("Parsing labels from System Data") + + if inv.Labels == nil { + inv.Labels = map[string]string{} + } + + for k, v := range reg.Spec.MachineInventoryLabels { + log.Debugf("Parsing: %v : %v", k, v) + + parsedData, err := replaceStringData(labels, v) + if err != nil { + if errors.Is(err, errValueNotFound) { + log.Debugf("Value not found: %v", v) + continue + } + log.Errorf("Failed parsing system data: %v", err.Error()) + return err + } + parsedData = sanitizeString(parsedData) + + log.Debugf("Parsed %s into %s with system data, setting it to label %s", v, parsedData, k) + inv.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") + } + + return nil +} +func replaceStringData(data map[string]interface{}, name string) (string, error) { + str := name + result := &strings.Builder{} + for { + i := strings.Index(str, "${") + if i == -1 { + result.WriteString(str) + break + } + j := strings.Index(str[i:], "}") + if j == -1 { + result.WriteString(str) + break + } + + result.WriteString(str[:i]) + obj := values.GetValueN(data, strings.Split(str[i+2:j+i], "/")...) + if str, ok := obj.(string); ok { + result.WriteString(str) + } else { + return "", errValueNotFound + } + str = str[j+i+1:] + } + + return result.String(), nil +} From 07265ec6672b9f6b06377416325bf529d60bf0d8 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Aug 2024 18:35:01 +0200 Subject: [PATCH 2/9] operator: add pkg/templater Signed-off-by: Francesco Giudici --- pkg/templater/templater.go | 85 ++++++++++++++++++++ pkg/templater/templater_test.go | 132 ++++++++++++++++++++++++++++++++ 2 files changed, 217 insertions(+) create mode 100644 pkg/templater/templater.go create mode 100644 pkg/templater/templater_test.go diff --git a/pkg/templater/templater.go b/pkg/templater/templater.go new file mode 100644 index 00000000..425eee12 --- /dev/null +++ b/pkg/templater/templater.go @@ -0,0 +1,85 @@ +/* +Copyright © 2022 - 2024 SUSE LLC + +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 templater + +import ( + "errors" + "strings" + + values "github.com/rancher/wrangler/v2/pkg/data" +) + +var errValueNotFound = errors.New("value not found") + +func IsValueNotFoundError(err error) bool { + return errors.Is(err, errValueNotFound) +} + +type Templater interface { + Fill(data map[string]interface{}) + Decode(str string) (string, error) +} + +func NewTemplater() *templater { + return &templater{ + data: map[string]interface{}{}, + } +} + +var _ Templater = (*templater)(nil) + +type templater struct { + data map[string]interface{} +} + +func (t *templater) Fill(data map[string]interface{}) { + for k, v := range data { + t.data[k] = v + } +} + +func (t *templater) Decode(str string) (string, error) { + return replaceStringData(t.data, str) +} + +func replaceStringData(data map[string]interface{}, name string) (string, error) { + str := name + result := &strings.Builder{} + for { + i := strings.Index(str, "${") + if i == -1 { + result.WriteString(str) + break + } + j := strings.Index(str[i:], "}") + if j == -1 { + result.WriteString(str) + break + } + + result.WriteString(str[:i]) + obj := values.GetValueN(data, strings.Split(str[i+2:j+i], "/")...) + if str, ok := obj.(string); ok { + result.WriteString(str) + } else { + return "", errValueNotFound + } + str = str[j+i+1:] + } + + return result.String(), nil +} diff --git a/pkg/templater/templater_test.go b/pkg/templater/templater_test.go new file mode 100644 index 00000000..f643a3da --- /dev/null +++ b/pkg/templater/templater_test.go @@ -0,0 +1,132 @@ +/* +Copyright © 2022 - 2024 SUSE LLC + +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 templater + +import ( + "fmt" + "testing" + + "gotest.tools/v3/assert" +) + +func TestFillAndDecode(t *testing.T) { + data := map[string]interface{}{ + "level1A": map[string]interface{}{ + "level2A": "level2AValue", + "level2B": map[string]interface{}{ + "level3A": "level3AValue", + }, + }, + "level1B": "level1BValue", + "Troublesome": map[string]interface{}{ + "emptyData": "", + "noStringVal": 5, + }, + } + + testCase := []struct { + Format string + Output string + Error string + }{ + { + Format: "${level1B}", + Output: "level1BValue", + }, + { + Format: "${level1B", + Output: "${level1B", + }, + { + Format: "a${level1B", + Output: "a${level1B", + }, + { + Format: "${}", + Error: "value not found", + }, + { + Format: "${", + Output: "${", + }, + { + Format: "a${", + Output: "a${", + }, + { + Format: "${level1A}", + Error: "value not found", + }, + { + Format: "a${level1A}c", + Error: "value not found", + }, + { + Format: "a${level1A}", + Error: "value not found", + }, + { + Format: "${level1A}c", + Error: "value not found", + }, + { + Format: "a${level1A/level2A}c", + Output: "alevel2AValuec", + }, + { + Format: "a${level1A/level2B/level3A}c", + Output: "alevel3AValuec", + }, + { + Format: "a${level1A/level2B/level3A}c${level1B}", + Output: "alevel3AValueclevel1BValue", + }, + { + Format: "a${unknown}", + Error: "value not found", + }, + { + Format: "${Troublesome/emptyData}", + Output: "", + }, + { + Format: "${Troublesome/noStringVal}", + Error: "value not found", + }, + } + + tmpl := NewTemplater() + tmpl.Fill(data) + for _, testCase := range testCase { + t.Run(testCase.Format, func(t *testing.T) { + str, err := tmpl.Decode(testCase.Format) + if testCase.Error == "" { + assert.NilError(t, err) + assert.Equal(t, testCase.Output, str, "'%s' not equal to '%s'", testCase.Output, str) + } else { + assert.Equal(t, testCase.Error, err.Error()) + } + }) + } +} + +func TestIsValueNotFoundError(t *testing.T) { + errRandom := fmt.Errorf("random error") + + assert.Equal(t, IsValueNotFoundError(errValueNotFound), true) + assert.Equal(t, IsValueNotFoundError(errRandom), false) +} From 379d6bf1fc77fc40f8a1e857a163276cbafd0fd9 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 08:21:51 +0200 Subject: [PATCH 3/9] operator: add few comments Signed-off-by: Francesco Giudici --- pkg/hostinfo/hostinfo.go | 7 +++++++ pkg/register/websocket.go | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/hostinfo/hostinfo.go b/pkg/hostinfo/hostinfo.go index 2d06a3d3..9d3dd16a 100644 --- a/pkg/hostinfo/hostinfo.go +++ b/pkg/hostinfo/hostinfo.go @@ -137,6 +137,13 @@ func host(opts ...*ghw.WithOption) (HostInfo, error) { // Deprecated. Remove me together with 'MsgSystemData' type. func FillData(data []byte) (map[string]interface{}, error) { + // Also available but not used: + // systemData.Product -> name, vendor, serial,uuid,sku,version. Kind of smbios data + // systemData.BIOS -> info about the bios. Useless IMO + // systemData.Baseboard -> asset, serial, vendor,version,product. Kind of useless? + // systemData.Chassis -> asset, serial, vendor,version,product, type. Maybe be useful depending on the provider. + // systemData.Topology -> CPU/memory and cache topology. No idea if useful. + systemData := &HostInfo{} if err := json.Unmarshal(data, &systemData); err != nil { return nil, fmt.Errorf("unmarshalling system data payload: %w", err) diff --git a/pkg/register/websocket.go b/pkg/register/websocket.go index 49aab4a2..ae7bfdb6 100644 --- a/pkg/register/websocket.go +++ b/pkg/register/websocket.go @@ -35,7 +35,7 @@ const ( MsgLabels MsgGet // v0.5.0 MsgVersion // v1.1.0 - MsgSystemData // v1.1.1 + MsgSystemData // v1.1.1 deprecated by MsgSystemDataV2 MsgConfig // v1.1.1 MsgError // v1.1.1 MsgAnnotations // v1.1.4 From 065c6b14f3ac33609f53671602f39777a910a955 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 08:23:23 +0200 Subject: [PATCH 4/9] operator: rework label templating Fixes #807 Signed-off-by: Francesco Giudici --- pkg/server/api_registration.go | 78 ++++-------- pkg/server/labeltmpl.go | 210 +++++++++++++-------------------- pkg/server/server.go | 16 ++- 3 files changed, 111 insertions(+), 193 deletions(-) diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index 52f5691d..1c28d03d 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -31,8 +31,10 @@ import ( "k8s.io/apimachinery/pkg/types" elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" + "github.com/rancher/elemental-operator/pkg/hostinfo" "github.com/rancher/elemental-operator/pkg/log" "github.com/rancher/elemental-operator/pkg/register" + "github.com/rancher/elemental-operator/pkg/templater" ) type LegacyConfig struct { @@ -45,7 +47,6 @@ var ( sanitizeHostname = regexp.MustCompile("[^0-9a-zA-Z.]") doubleDash = regexp.MustCompile("--+") start = regexp.MustCompile("^[a-zA-Z0-9]") - errValueNotFound = errors.New("value not found") errInventoryNotFound = errors.New("MachineInventory not found") ) @@ -216,6 +217,7 @@ func (i *InventoryServer) getRancherCACert() string { func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration) error { protoVersion := register.MsgUndefined + tmpl := templater.NewTemplater() for { var data []byte @@ -237,13 +239,13 @@ func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1 replyMsgType = register.MsgVersion replyData = []byte{byte(protoVersion)} case register.MsgSmbios: - err = updateInventoryFromSMBIOSData(data, inventory, registration) - if err != nil { + smbiosData := map[string]interface{}{} + if err := json.Unmarshal(data, &smbiosData); err != nil { return fmt.Errorf("failed to extract labels from SMBIOS data: %w", err) } - log.Debugf("received SMBIOS data - generated machine name: %s", inventory.Name) + tmpl.Fill(smbiosData) case register.MsgLabels: - if err := mergeInventoryLabels(inventory, data); err != nil { + if err := updateInventoryWithLabels(inventory, data); err != nil { return err } case register.MsgAnnotations: @@ -252,6 +254,14 @@ func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1 return fmt.Errorf("failed to decode dynamic data: %w", err) } case register.MsgGet: + // Final call: here we commit the MachineInventory, send the Elemental config data + // and close the connection. + if err := updateInventoryName(tmpl, inventory); err != nil { + return fmt.Errorf("failed to resolve inventory name: %w", err) + } + if err := updateInventoryLabels(tmpl, inventory, registration); err != nil { + return fmt.Errorf("failed to update inventory labels: %w", err) + } return i.handleGet(conn, protoVersion, inventory, registration) case register.MsgUpdate: err = i.handleUpdate(conn, protoVersion, inventory) @@ -259,15 +269,17 @@ func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1 return fmt.Errorf("failed to negotiate registration update: %w", err) } case register.MsgSystemData: - err = updateInventoryFromSystemData(data, inventory, registration) + systemData, err := hostinfo.FillData(data) if err != nil { - return fmt.Errorf("failed to extract labels from system data: %w", err) + return fmt.Errorf("failed to parse system data: %w", err) } + tmpl.Fill(systemData) case register.MsgSystemDataV2: - err = updateInventoryFromSystemDataNG(data, inventory, registration) - if err != nil { - return fmt.Errorf("failed to extract labels from system data: %w", err) + systemData := map[string]interface{}{} + if err := json.Unmarshal(data, &systemData); err != nil { + return fmt.Errorf("failed to parse system data: %w", err) } + tmpl.Fill(systemData) default: return fmt.Errorf("got unexpected message: %s", msgType) } @@ -349,49 +361,3 @@ func decodeProtocolVersion(data []byte) (register.MessageType, error) { return protoVersion, nil } - -// sanitizeString will sanitize a given string by: -// replacing all invalid chars as set on the sanitize regex by dashes -// removing any double dashes resulted from the above method -// removing prefix+suffix if they are a dash -func sanitizeString(s string) string { - s1 := sanitize.ReplaceAllString(s, "-") - s2 := doubleDash.ReplaceAllString(s1, "-") - if !start.MatchString(s2) { - s2 = "m" + s2 - } - if len(s2) > 58 { - s2 = s2[:58] - } - return s2 -} - -// like sanitizeString but allows also '.' inside "s" -func sanitizeStringHostname(s string) string { - s1 := sanitizeHostname.ReplaceAllString(s, "-") - s2 := doubleDash.ReplaceAllLiteralString(s1, "-") - if !start.MatchString(s2) { - s2 = "m" + s2 - } - if len(s2) > 58 { - s2 = s2[:58] - } - return s2 -} - -func mergeInventoryLabels(inventory *elementalv1.MachineInventory, data []byte) error { - labels := map[string]string{} - if err := json.Unmarshal(data, &labels); err != nil { - return fmt.Errorf("cannot extract inventory labels: %w", err) - } - log.Debugf("received labels: %v", labels) - log.Warningf("received labels from registering client: no more supported, skipping") - if inventory.Labels == nil { - inventory.Labels = map[string]string{} - } - return nil -} - -func isNewInventory(inventory *elementalv1.MachineInventory) bool { - return inventory.CreationTimestamp.IsZero() -} diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go index bf96b378..aefd1dfe 100644 --- a/pkg/server/labeltmpl.go +++ b/pkg/server/labeltmpl.go @@ -18,170 +18,124 @@ package server import ( "encoding/json" - "errors" "fmt" "strings" elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" - "github.com/rancher/elemental-operator/pkg/hostinfo" "github.com/rancher/elemental-operator/pkg/log" - values "github.com/rancher/wrangler/v2/pkg/data" + "github.com/rancher/elemental-operator/pkg/templater" ) -func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { - annotations := map[string]string{} - if err := json.Unmarshal(data, &annotations); err != nil { - return err +func updateInventoryName(tmpl templater.Templater, inv *elementalv1.MachineInventory) error { + // Inventory Name should be set only on freshly created inventories. + if !isNewInventory(inv) { + return nil } - log.Debug("Adding annotations from client data") - if mInventory.Annotations == nil { - mInventory.Annotations = map[string]string{} + // Sanitize any lower dashes into dashes as hostnames cannot have lower dashes, and we use the inventory name + // to set the machine hostname. Also set it to lowercase. + name, err := tmpl.Decode(inv.Name) + if err != nil { + if templater.IsValueNotFoundError(err) { + name = generateInventoryName() + log.Warningf("Templater cannot decode MachineInventory name %q, fallback to %q.", inv.Name, name) + inv.Name = name + return nil + } + return fmt.Errorf("templater: cannot decode MachineInventory name %q: %w", inv.Name, err) } - for key, val := range annotations { - mInventory.Annotations[fmt.Sprintf("elemental.cattle.io/%s", sanitizeUserInput(key))] = sanitizeUserInput(val) + name = sanitizeStringHostname(name) + + // Something went wrong, decoding and sanitizing the hostname it got empty. + if name == "" { + return fmt.Errorf("invalid MachineInventory name: %q", name) } + inv.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) return nil } -// updateInventoryFromSMBIOSData() updates mInventory Name and Labels from the MachineRegistration and the SMBIOS data -func updateInventoryFromSMBIOSData(data []byte, mInventory *elementalv1.MachineInventory, mRegistration *elementalv1.MachineRegistration) error { - smbiosData := map[string]interface{}{} - if err := json.Unmarshal(data, &smbiosData); err != nil { - return err - } - // Sanitize any lower dashes into dashes as hostnames cannot have lower dashes, and we use the inventory name - // to set the machine hostname. Also set it to lowercase - name, err := replaceStringData(smbiosData, mInventory.Name) - if err == nil { - name = sanitizeStringHostname(name) - mInventory.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) - } else { - if errors.Is(err, errValueNotFound) { - // value not found, will be set in updateInventoryFromSystemData - log.Warningf("SMBIOS Value not found: %v", mInventory.Name) - } else { - return err - } - } - - log.Debugf("Adding labels from registration") - // Add extra label info from data coming from smbios and based on the registration data - if mInventory.Labels == nil { - mInventory.Labels = map[string]string{} +func updateInventoryLabels(tmpl templater.Templater, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { + log.Debugf("Adding registration labels") + if inv.Labels == nil { + inv.Labels = map[string]string{} } - for k, v := range mRegistration.Spec.MachineInventoryLabels { - parsedData, err := replaceStringData(smbiosData, v) + for k, v := range reg.Spec.MachineInventoryLabels { + decodedLabel, err := tmpl.Decode(v) if err != nil { - if errors.Is(err, errValueNotFound) { - log.Debugf("Value not found: %v", v) + if templater.IsValueNotFoundError(err) { + log.Warningf("Templater cannot decode label '%q': %s", v, err.Error()) continue } - log.Errorf("Failed parsing smbios data: %v", err.Error()) + log.Errorf("Templater failed decoding label '%q': %s", v, err.Error()) return err } - parsedData = sanitizeString(parsedData) + decodedLabel = sanitizeString(decodedLabel) - log.Debugf("Parsed %s into %s with smbios data, setting it to label %s", v, parsedData, k) - mInventory.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") + log.Debugf("Decoded %s into %s, setting it to label %s", v, decodedLabel, k) + inv.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(decodedLabel, "-"), "-") } return nil } -// updateInventoryFromSystemDataNG receives digested hardware labels from the client -func updateInventoryFromSystemDataNG(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - labels := map[string]interface{}{} - +func updateInventoryWithLabels(inventory *elementalv1.MachineInventory, data []byte) error { + labels := map[string]string{} if err := json.Unmarshal(data, &labels); err != nil { - return fmt.Errorf("unmarshalling system data labels payload: %w", err) + return fmt.Errorf("cannot extract inventory labels: %w", err) } - - return sanitizeSystemDataLabels(labels, inv, reg) + log.Debugf("received labels: %v", labels) + log.Errorf("received labels from registering client: no more supported, skipping") + if inventory.Labels == nil { + inventory.Labels = map[string]string{} + } + return nil } -// Deprecated. Remove me together with 'MsgSystemData' type. -// updateInventoryFromSystemData creates labels in the inventory based on the hardware information -func updateInventoryFromSystemData(data []byte, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - log.Infof("Adding labels from system data") - - labels, err := hostinfo.FillData(data) - if err != nil { - return err +func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { + annotations := map[string]string{} + if err := json.Unmarshal(data, &annotations); err != nil { + return fmt.Errorf("cannot extract inventory annotations: %w", err) } - - return sanitizeSystemDataLabels(labels, inv, reg) + log.Debug("Adding annotations from client data") + if mInventory.Annotations == nil { + mInventory.Annotations = map[string]string{} + } + for key, val := range annotations { + mInventory.Annotations[fmt.Sprintf("elemental.cattle.io/%s", sanitizeUserInput(key))] = sanitizeUserInput(val) + } + return nil } -func sanitizeSystemDataLabels(labels map[string]interface{}, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { - // Also available but not used: - // systemData.Product -> name, vendor, serial,uuid,sku,version. Kind of smbios data - // systemData.BIOS -> info about the bios. Useless IMO - // systemData.Baseboard -> asset, serial, vendor,version,product. Kind of useless? - // systemData.Chassis -> asset, serial, vendor,version,product, type. Maybe be useful depending on the provider. - // systemData.Topology -> CPU/memory and cache topology. No idea if useful. - - name, err := replaceStringData(labels, inv.Name) - if err != nil { - if errors.Is(err, errValueNotFound) { - log.Warningf("System data value not found: %v", inv.Name) - name = "m" - } else { - return err - } +// like sanitizeString but allows also '.' inside "s" +func sanitizeStringHostname(s string) string { + if s == "" { + return "" } - name = sanitizeStringHostname(name) - - inv.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) - - log.Debugf("Parsing labels from System Data") - - if inv.Labels == nil { - inv.Labels = map[string]string{} + s1 := sanitizeHostname.ReplaceAllString(s, "-") + s2 := doubleDash.ReplaceAllLiteralString(s1, "-") + if !start.MatchString(s2) { + s2 = "m" + s2 } - - for k, v := range reg.Spec.MachineInventoryLabels { - log.Debugf("Parsing: %v : %v", k, v) - - parsedData, err := replaceStringData(labels, v) - if err != nil { - if errors.Is(err, errValueNotFound) { - log.Debugf("Value not found: %v", v) - continue - } - log.Errorf("Failed parsing system data: %v", err.Error()) - return err - } - parsedData = sanitizeString(parsedData) - - log.Debugf("Parsed %s into %s with system data, setting it to label %s", v, parsedData, k) - inv.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(parsedData, "-"), "-") + if len(s2) > 58 { + s2 = s2[:58] } - - return nil + return s2 } -func replaceStringData(data map[string]interface{}, name string) (string, error) { - str := name - result := &strings.Builder{} - for { - i := strings.Index(str, "${") - if i == -1 { - result.WriteString(str) - break - } - j := strings.Index(str[i:], "}") - if j == -1 { - result.WriteString(str) - break - } - result.WriteString(str[:i]) - obj := values.GetValueN(data, strings.Split(str[i+2:j+i], "/")...) - if str, ok := obj.(string); ok { - result.WriteString(str) - } else { - return "", errValueNotFound - } - str = str[j+i+1:] +// sanitizeString will sanitize a given string by: +// replacing all invalid chars as set on the sanitize regex by dashes +// removing any double dashes resulted from the above method +// removing prefix+suffix if they are a dash +func sanitizeString(s string) string { + s1 := sanitize.ReplaceAllString(s, "-") + s2 := doubleDash.ReplaceAllString(s1, "-") + if !start.MatchString(s2) { + s2 = "m" + s2 } + if len(s2) > 58 { + s2 = s2[:58] + } + return s2 +} - return result.String(), nil +func isNewInventory(inventory *elementalv1.MachineInventory) bool { + return inventory.CreationTimestamp.IsZero() } diff --git a/pkg/server/server.go b/pkg/server/server.go index 09c5a30f..c3ec0006 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -141,27 +141,25 @@ func (i *InventoryServer) authMachine(conn *websocket.Conn, req *http.Request, r return nil, nil } -func initInventory(inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration) { +func generateInventoryName() string { const namePrefix = "m-" + return namePrefix + uuid.NewString() +} +func initInventory(inventory *elementalv1.MachineInventory, registration *elementalv1.MachineRegistration) { if registration.Spec.Config == nil { registration.Spec.Config = &elementalv1.Config{} } inventory.Name = registration.Spec.MachineName if inventory.Name == "" { - inventory.Name = namePrefix + uuid.NewString() + inventory.Name = generateInventoryName() } inventory.Namespace = registration.Namespace inventory.Annotations = registration.Spec.MachineInventoryAnnotations - // Set labels using replaceStringData to remove forbidden characters and - // default values for SMBIOS/System Data values until they are updated. + // Set the labels later as we may need to do some template decoding and we need + // to get data from the client first inventory.Labels = map[string]string{} - for k, v := range registration.Spec.MachineInventoryLabels { - value, _ := replaceStringData(map[string]interface{}{}, v) - value = sanitizeString(value) - inventory.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(value, "-"), "-") - } // Set resettable annotation on cascade from MachineRegistration spec if registration.Spec.Config.Elemental.Reset.Enabled { From 6c6b28356765e9c2f3f12ac4ceaa3956a5c0eda7 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 08:35:11 +0200 Subject: [PATCH 5/9] tests: update api_registration_test.go Signed-off-by: Francesco Giudici --- pkg/server/api_registration_test.go | 34 ++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/pkg/server/api_registration_test.go b/pkg/server/api_registration_test.go index 9b634476..f9e403dd 100644 --- a/pkg/server/api_registration_test.go +++ b/pkg/server/api_registration_test.go @@ -48,6 +48,7 @@ import ( "github.com/rancher/elemental-operator/pkg/hostinfo" "github.com/rancher/elemental-operator/pkg/register" elementalruntime "github.com/rancher/elemental-operator/pkg/runtime" + "github.com/rancher/elemental-operator/pkg/templater" ) var ( @@ -258,9 +259,11 @@ func TestBuildName(t *testing.T) { }, } + tmpl := templater.NewTemplater() + tmpl.Fill(data) for _, testCase := range testCase { t.Run(testCase.Format, func(t *testing.T) { - str, err := replaceStringData(data, testCase.Format) + str, err := tmpl.Decode(testCase.Format) if testCase.Error == "" { str = sanitizeString(str) assert.NilError(t, err) @@ -315,7 +318,7 @@ func TestMergeInventoryLabels(t *testing.T) { inventory := &elementalv1.MachineInventory{} inventory.Labels = test.labels - err := mergeInventoryLabels(inventory, test.data) + err := updateInventoryWithLabels(inventory, test.data) if test.fail { assert.Assert(t, err != nil) } else { @@ -332,21 +335,36 @@ func TestMergeInventoryLabels(t *testing.T) { func TestUpdateInventoryFromSystemData(t *testing.T) { inventory := &elementalv1.MachineInventory{} + tmpl := templater.NewTemplater() + encodedData, err := json.Marshal(hostInfoFixture) assert.NilError(t, err) - err = updateInventoryFromSystemData(encodedData, inventory, systemDataLabelsRegistrationFixture) + + systemData, err := hostinfo.FillData(encodedData) assert.NilError(t, err) + tmpl.Fill(systemData) + err = updateInventoryLabels(tmpl, inventory, systemDataLabelsRegistrationFixture) + assert.NilError(t, err) assertSystemDataLabels(t, inventory) } func TestUpdateInventoryFromSystemDataNG(t *testing.T) { inventory := &elementalv1.MachineInventory{} + tmpl := templater.NewTemplater() + data, err := hostinfo.ExtractLabels(hostInfoFixture) assert.NilError(t, err) + encodedData, err := json.Marshal(data) assert.NilError(t, err) - err = updateInventoryFromSystemDataNG(encodedData, inventory, systemDataLabelsRegistrationFixture) + + systemData := map[string]interface{}{} + err = json.Unmarshal(encodedData, &systemData) + assert.NilError(t, err) + + tmpl.Fill(systemData) + err = updateInventoryLabels(tmpl, inventory, systemDataLabelsRegistrationFixture) assert.NilError(t, err) assertSystemDataLabels(t, inventory) } @@ -444,7 +462,13 @@ func TestUpdateInventoryFromSystemDataSanitized(t *testing.T) { } encodedData, err := json.Marshal(data) assert.NilError(t, err) - err = updateInventoryFromSystemData(encodedData, inventory, registration) + systemData, err := hostinfo.FillData(encodedData) + assert.NilError(t, err) + tmpl := templater.NewTemplater() + tmpl.Fill(systemData) + err = updateInventoryName(tmpl, inventory) + assert.NilError(t, err) + err = updateInventoryLabels(tmpl, inventory, registration) assert.NilError(t, err) // Check that the labels we properly added to the inventory assert.Equal(t, inventory.Name, "machine-1") From ba44afebc20a0088edbef614ee7d82a18e46dd44 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 13:11:24 +0200 Subject: [PATCH 6/9] operator: move regexp to label templating file Signed-off-by: Francesco Giudici --- pkg/server/api_registration.go | 9 +-------- pkg/server/labeltmpl.go | 8 ++++++++ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index 1c28d03d..14b79f0d 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -23,7 +23,6 @@ import ( "io" "net/http" "path" - "regexp" "github.com/gorilla/websocket" "gopkg.in/yaml.v3" @@ -42,13 +41,7 @@ type LegacyConfig struct { CloudConfig map[string]interface{} `yaml:"cloud-config,omitempty"` } -var ( - sanitize = regexp.MustCompile("[^0-9a-zA-Z_]") - sanitizeHostname = regexp.MustCompile("[^0-9a-zA-Z.]") - doubleDash = regexp.MustCompile("--+") - start = regexp.MustCompile("^[a-zA-Z0-9]") - errInventoryNotFound = errors.New("MachineInventory not found") -) +var errInventoryNotFound = errors.New("MachineInventory not found") func (i *InventoryServer) apiRegistration(resp http.ResponseWriter, req *http.Request) error { var err error diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go index aefd1dfe..6d99284d 100644 --- a/pkg/server/labeltmpl.go +++ b/pkg/server/labeltmpl.go @@ -19,6 +19,7 @@ package server import ( "encoding/json" "fmt" + "regexp" "strings" elementalv1 "github.com/rancher/elemental-operator/api/v1beta1" @@ -26,6 +27,13 @@ import ( "github.com/rancher/elemental-operator/pkg/templater" ) +var ( + sanitize = regexp.MustCompile("[^0-9a-zA-Z_]") + sanitizeHostname = regexp.MustCompile("[^0-9a-zA-Z.]") + doubleDash = regexp.MustCompile("--+") + start = regexp.MustCompile("^[a-zA-Z0-9]") +) + func updateInventoryName(tmpl templater.Templater, inv *elementalv1.MachineInventory) error { // Inventory Name should be set only on freshly created inventories. if !isNewInventory(inv) { From 98444459ce825234f8b0ac1ae5030b6791a63eeb Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 13:45:14 +0200 Subject: [PATCH 7/9] operator: change template behavior in corner cases 1. '.' is added to the allowed characted in label values (previously was sobsituted with '-'). 2. when the first character of a label value is not alphanumeric ("-" or "_" or ".") we drop it (previously we prepended 'm'). 3. if the last characted of a label value is not alphanumeric ("-" or "_" or ".") we drop it (previously was not checked). Signed-off-by: Francesco Giudici --- pkg/server/api_registration_test.go | 8 ++-- pkg/server/labeltmpl.go | 64 ++++++++++++++++------------- 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/pkg/server/api_registration_test.go b/pkg/server/api_registration_test.go index f9e403dd..910ead0b 100644 --- a/pkg/server/api_registration_test.go +++ b/pkg/server/api_registration_test.go @@ -207,7 +207,7 @@ func TestBuildName(t *testing.T) { }, { Format: "${level1B", - Output: "m-level1B", + Output: "level1B", }, { Format: "a${level1B", @@ -219,11 +219,11 @@ func TestBuildName(t *testing.T) { }, { Format: "${", - Output: "m-", + Output: "", }, { Format: "a${", - Output: "a-", + Output: "a", }, { Format: "${level1A}", @@ -265,7 +265,7 @@ func TestBuildName(t *testing.T) { t.Run(testCase.Format, func(t *testing.T) { str, err := tmpl.Decode(testCase.Format) if testCase.Error == "" { - str = sanitizeString(str) + str = sanitizeLabel(str) assert.NilError(t, err) assert.Equal(t, testCase.Output, str, "'%s' not equal to '%s'", testCase.Output, str) } else { diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go index 6d99284d..8706b7e5 100644 --- a/pkg/server/labeltmpl.go +++ b/pkg/server/labeltmpl.go @@ -28,10 +28,9 @@ import ( ) var ( - sanitize = regexp.MustCompile("[^0-9a-zA-Z_]") - sanitizeHostname = regexp.MustCompile("[^0-9a-zA-Z.]") - doubleDash = regexp.MustCompile("--+") - start = regexp.MustCompile("^[a-zA-Z0-9]") + doubleDash = regexp.MustCompile("--+") + start = regexp.MustCompile("^[a-zA-Z0-9]") + end = regexp.MustCompile("[a-zA-Z0-9]$") ) func updateInventoryName(tmpl templater.Templater, inv *elementalv1.MachineInventory) error { @@ -51,13 +50,13 @@ func updateInventoryName(tmpl templater.Templater, inv *elementalv1.MachineInven } return fmt.Errorf("templater: cannot decode MachineInventory name %q: %w", inv.Name, err) } - name = sanitizeStringHostname(name) + name = sanitizeHostname(name) // Something went wrong, decoding and sanitizing the hostname it got empty. if name == "" { return fmt.Errorf("invalid MachineInventory name: %q", name) } - inv.Name = strings.ToLower(sanitizeHostname.ReplaceAllString(name, "-")) + inv.Name = strings.ToLower(name) return nil } @@ -76,10 +75,10 @@ func updateInventoryLabels(tmpl templater.Templater, inv *elementalv1.MachineInv log.Errorf("Templater failed decoding label '%q': %s", v, err.Error()) return err } - decodedLabel = sanitizeString(decodedLabel) + decodedLabel = sanitizeLabel(decodedLabel) log.Debugf("Decoded %s into %s, setting it to label %s", v, decodedLabel, k) - inv.Labels[k] = strings.TrimSuffix(strings.TrimPrefix(decodedLabel, "-"), "-") + inv.Labels[k] = decodedLabel } return nil } @@ -112,31 +111,40 @@ func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.Machine return nil } -// like sanitizeString but allows also '.' inside "s" -func sanitizeStringHostname(s string) string { - if s == "" { - return "" - } - s1 := sanitizeHostname.ReplaceAllString(s, "-") - s2 := doubleDash.ReplaceAllLiteralString(s1, "-") - if !start.MatchString(s2) { - s2 = "m" + s2 - } - if len(s2) > 58 { - s2 = s2[:58] - } - return s2 +// like sanitizeString but drops '_' inside "s" +func sanitizeHostname(s string) string { + regHostname := regexp.MustCompile("[^0-9a-zA-Z.]") + + return sanitize(s, regHostname) } -// sanitizeString will sanitize a given string by: +func sanitizeLabel(s string) string { + regLabels := regexp.MustCompile("[^0-9a-zA-Z._]") + + return sanitize(s, regLabels) +} + +// sanitize will sanitize a given string by: // replacing all invalid chars as set on the sanitize regex by dashes // removing any double dashes resulted from the above method -// removing prefix+suffix if they are a dash -func sanitizeString(s string) string { - s1 := sanitize.ReplaceAllString(s, "-") +// removing prefix+suffix if they are not alphanum characters +func sanitize(s string, reg *regexp.Regexp) string { + if s == "" { + return "" + } + s1 := reg.ReplaceAllString(s, "-") s2 := doubleDash.ReplaceAllString(s1, "-") - if !start.MatchString(s2) { - s2 = "m" + s2 + for !start.MatchString(s2) { + if len(s2) == 1 { + return "" + } + s2 = s2[1:] + } + for !end.MatchString(s2) { + if len(s2) == 1 { + return "" + } + s2 = s2[:len(s2)-1] } if len(s2) > 58 { s2 = s2[:58] From c053d383e0745afe2881845c870bfdba4e2c0253 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 2 Aug 2024 18:41:29 +0200 Subject: [PATCH 8/9] tests: improve coverage of label templating Signed-off-by: Francesco Giudici --- pkg/server/api_registration.go | 4 +- pkg/server/api_registration_test.go | 156 +++++++++++++++++++++++++++- pkg/server/labeltmpl.go | 8 +- 3 files changed, 162 insertions(+), 6 deletions(-) diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index 14b79f0d..1710061c 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -238,11 +238,11 @@ func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1 } tmpl.Fill(smbiosData) case register.MsgLabels: - if err := updateInventoryWithLabels(inventory, data); err != nil { + if err := mergeInventoryLabels(inventory, data); err != nil { return err } case register.MsgAnnotations: - err = updateInventoryWithAnnotations(data, inventory) + err = mergeInventoryAnnotations(data, inventory) if err != nil { return fmt.Errorf("failed to decode dynamic data: %w", err) } diff --git a/pkg/server/api_registration_test.go b/pkg/server/api_registration_test.go index 910ead0b..a5646a6b 100644 --- a/pkg/server/api_registration_test.go +++ b/pkg/server/api_registration_test.go @@ -27,7 +27,9 @@ import ( "path" "strings" "testing" + "time" + "github.com/google/uuid" "github.com/gorilla/websocket" "github.com/jaypipes/ghw/pkg/block" "github.com/jaypipes/ghw/pkg/cpu" @@ -257,6 +259,30 @@ func TestBuildName(t *testing.T) { Format: "a${unknown}", Error: "value not found", }, + { + Format: "+check-sanitize!", + Output: "check-sanitize", + }, + { + Format: "VeryVeryVeryLongLabelValueThatWillBeCutTo58Chars-xxxxx-End|CUTFROMHERE", + Output: "VeryVeryVeryLongLabelValueThatWillBeCutTo58Chars-xxxxx-End", + }, + { + Format: "double--dash--+-sanitized++--", + Output: "double-dash-sanitized", + }, + { + Format: "AllowedNotAlphaNumChars:_.NotTrailingBTW_.", + Output: "AllowedNotAlphaNumChars-_.NotTrailingBTW", + }, + { + Format: "+.!_-", + Output: "", + }, + { + Format: "", + Output: "", + }, } tmpl := templater.NewTemplater() @@ -318,7 +344,7 @@ func TestMergeInventoryLabels(t *testing.T) { inventory := &elementalv1.MachineInventory{} inventory.Labels = test.labels - err := updateInventoryWithLabels(inventory, test.data) + err := mergeInventoryLabels(inventory, test.data) if test.fail { assert.Assert(t, err != nil) } else { @@ -333,6 +359,63 @@ func TestMergeInventoryLabels(t *testing.T) { } } +func TestMergeInventoryAnnotations(t *testing.T) { + testCase := []struct { + data []byte // annotations to add to the inventory + annotations map[string]string // annotations already in the inventory + fail bool + expected map[string]string + }{ + { + []byte(`{"key2":"val2"}`), + map[string]string{"key1": "val1"}, + false, + map[string]string{"key1": "val1", "elemental.cattle.io/key2": "val2"}, + }, + { + []byte(`{"key2":2}`), + map[string]string{"key1": "val1"}, + true, + map[string]string{"key1": "val1"}, + }, + { + []byte(`{"key2":"val2", "key3":"val3"}`), + map[string]string{"key1": "val1", "elemental.cattle.io/key3": "previous_val"}, + false, + map[string]string{"key1": "val1", "elemental.cattle.io/key3": "val3", "elemental.cattle.io/key2": "val2"}, + }, + { + []byte{}, + map[string]string{"key1": "val1"}, + true, + map[string]string{"key1": "val1"}, + }, + { + []byte(`{"key2":"val2"}`), + nil, + false, + map[string]string{"elemental.cattle.io/key2": "val2"}, + }, + } + + for _, test := range testCase { + inventory := &elementalv1.MachineInventory{} + inventory.Annotations = test.annotations + + err := mergeInventoryAnnotations(test.data, inventory) + if test.fail { + assert.Assert(t, err != nil) + } else { + assert.NilError(t, err) + } + for k, v := range test.expected { + val, ok := inventory.Annotations[k] + assert.Equal(t, ok, true, "annotations: %v\nexpected: %v ", inventory.Annotations, test.expected) + assert.Equal(t, v, val, "annotations: %v\nexpected: %v ", inventory.Annotations, test.expected) + } + } +} + func TestUpdateInventoryFromSystemData(t *testing.T) { inventory := &elementalv1.MachineInventory{} tmpl := templater.NewTemplater() @@ -391,7 +474,7 @@ func assertSystemDataLabels(t *testing.T, inventory *elementalv1.MachineInventor assert.Equal(t, inventory.Labels["elemental.cattle.io/BlockDevice1-Removable"], "false") } -func TestUpdateInventoryFromSystemDataSanitized(t *testing.T) { +func TestUpdateInventoryLabels(t *testing.T) { inventory := &elementalv1.MachineInventory{} inventory.Name = "${System Data/Runtime/Hostname}" @@ -412,6 +495,7 @@ func TestUpdateInventoryFromSystemDataSanitized(t *testing.T) { "elemental.cattle.io/BlockDevice1-Size": "${System Data/Block Devices/testdisk2/Size}", "elemental.cattle.io/BlockDevice0-Removable": "${System Data/Block Devices/testdisk1/Removable}", "elemental.cattle.io/BlockDevice1-Removable": "${System Data/Block Devices/testdisk2/Removable}", + "elemental.cattle.io/UnexistingTemplate": "${System Data/Not Existing Value}", }, }, } @@ -490,6 +574,74 @@ func TestUpdateInventoryFromSystemDataSanitized(t *testing.T) { assert.Equal(t, len(validation.IsValidLabelValue(inventory.Labels["elemental.cattle.io/CpuVendor"])), 0) } +func TestUpdateInventoryName(t *testing.T) { + tmplData := map[string]interface{}{ + "Template Data": map[string]interface{}{ + "Data 1": "value1", + "Data 2": "value2", + "Empty data": "", + "Bad data": 0, + }, + } + tmpl := templater.NewTemplater() + tmpl.Fill(tmplData) + + testCases := []struct { + invName string // initial Inventory.Name + expName string // expected Inventory.Name + isOldInv bool // the inventory was created previously (do not update the name) + isError bool // the inventory name update will error out + isFallbk bool // the inventory name will be set to the fallback (UUID generated) + }{ + { + invName: "machine-${Template Data/Data 1}", + expName: "machine-value1", + }, + { + invName: "don't Touch if not new ${Template Data/Data 2}", + expName: "don't Touch if not new ${Template Data/Data 2}", + isOldInv: true, + }, + { + invName: "machine-${Template Data/Empty data}", + expName: "machine", + }, + { + invName: "machine-${Template Data/Dont Exists}", + isFallbk: true, + }, + { + invName: "$machine-${Template Data/Bad data}", + isFallbk: true, + }, + { + invName: "${Template Data/Empty data}.-", + isError: true, + }, + } + + for _, tc := range testCases { + inv := &elementalv1.MachineInventory{} + inv.Name = tc.invName + if tc.isOldInv { + inv.CreationTimestamp.Time = time.Now() + } + err := updateInventoryName(tmpl, inv) + if tc.isError { + assert.Assert(t, err != nil) + continue + } + if tc.isFallbk { + // Fallback is a m-UUID generated name + assert.Equal(t, inv.Name[:2], "m-") + _, err = uuid.Parse(inv.Name[2:]) + assert.NilError(t, err) + continue + } + assert.Equal(t, inv.Name, tc.expName) + } +} + func TestRegistrationMsgGet(t *testing.T) { testCases := []struct { name string diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go index 8706b7e5..e632d665 100644 --- a/pkg/server/labeltmpl.go +++ b/pkg/server/labeltmpl.go @@ -83,7 +83,9 @@ func updateInventoryLabels(tmpl templater.Templater, inv *elementalv1.MachineInv return nil } -func updateInventoryWithLabels(inventory *elementalv1.MachineInventory, data []byte) error { +// mergeInventoryLabels: DEPRECATED +// Used to merge client side labels, now deprecated would just skip and log an error. +func mergeInventoryLabels(inventory *elementalv1.MachineInventory, data []byte) error { labels := map[string]string{} if err := json.Unmarshal(data, &labels); err != nil { return fmt.Errorf("cannot extract inventory labels: %w", err) @@ -96,7 +98,9 @@ func updateInventoryWithLabels(inventory *elementalv1.MachineInventory, data []b return nil } -func updateInventoryWithAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { +// mergeInventoryAnnotations: merge annotations from the client, which include dynamic data, +// e.g., the IP address. All annotation keys are prepended with "elemental.cattle.io/". +func mergeInventoryAnnotations(data []byte, mInventory *elementalv1.MachineInventory) error { annotations := map[string]string{} if err := json.Unmarshal(data, &annotations); err != nil { return fmt.Errorf("cannot extract inventory annotations: %w", err) From 3bd462b28ce12ad1fae30e379284ac83fb03a6ca Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Mon, 5 Aug 2024 11:51:50 +0200 Subject: [PATCH 9/9] make linter happy Signed-off-by: Francesco Giudici --- pkg/server/api_registration.go | 7 ++----- pkg/server/labeltmpl.go | 10 ++++++++++ pkg/templater/templater.go | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/server/api_registration.go b/pkg/server/api_registration.go index 1710061c..b1148fb8 100644 --- a/pkg/server/api_registration.go +++ b/pkg/server/api_registration.go @@ -249,11 +249,8 @@ func (i *InventoryServer) serveLoop(conn *websocket.Conn, inventory *elementalv1 case register.MsgGet: // Final call: here we commit the MachineInventory, send the Elemental config data // and close the connection. - if err := updateInventoryName(tmpl, inventory); err != nil { - return fmt.Errorf("failed to resolve inventory name: %w", err) - } - if err := updateInventoryLabels(tmpl, inventory, registration); err != nil { - return fmt.Errorf("failed to update inventory labels: %w", err) + if err := updateInventoryWithTemplates(tmpl, inventory, registration); err != nil { + return err } return i.handleGet(conn, protoVersion, inventory, registration) case register.MsgUpdate: diff --git a/pkg/server/labeltmpl.go b/pkg/server/labeltmpl.go index e632d665..8a6fbcb0 100644 --- a/pkg/server/labeltmpl.go +++ b/pkg/server/labeltmpl.go @@ -33,6 +33,16 @@ var ( end = regexp.MustCompile("[a-zA-Z0-9]$") ) +func updateInventoryWithTemplates(tmpl templater.Templater, inv *elementalv1.MachineInventory, reg *elementalv1.MachineRegistration) error { + if err := updateInventoryName(tmpl, inv); err != nil { + return fmt.Errorf("failed to resolve inventory name: %w", err) + } + if err := updateInventoryLabels(tmpl, inv, reg); err != nil { + return fmt.Errorf("failed to update inventory labels: %w", err) + } + return nil +} + func updateInventoryName(tmpl templater.Templater, inv *elementalv1.MachineInventory) error { // Inventory Name should be set only on freshly created inventories. if !isNewInventory(inv) { diff --git a/pkg/templater/templater.go b/pkg/templater/templater.go index 425eee12..cca29adb 100644 --- a/pkg/templater/templater.go +++ b/pkg/templater/templater.go @@ -34,7 +34,7 @@ type Templater interface { Decode(str string) (string, error) } -func NewTemplater() *templater { +func NewTemplater() Templater { return &templater{ data: map[string]interface{}{}, }