From 50041b4db335305385c823dbc16433aafe280c20 Mon Sep 17 00:00:00 2001 From: Devon Bautista Date: Mon, 23 Oct 2023 14:49:39 -0600 Subject: [PATCH 1/2] Store MAC addresses lower case; convert when comparing To avoid having MAC addresses of different case being stored separately, MAC addresses are stored in their lower-case form and any time MAC address comparison occurs (e.g. for deletion/update/retrieval), the passed MAC address is converted to lower case before the comparison is done, effectively ignoring case. This ensures that a difference in case does not result in duplicate MAC addresses. --- internal/postgres/postgres.go | 44 +++++- .../postgres/tests/mac/04-bss-add-macs.hurl | 5 +- ...mac.hurl => 05-bss-add-duplicate-mac.hurl} | 19 ++- .../postgres/tests/mac/06-bss-delete-mac.hurl | 141 ++++++++++++++++++ ....hurl => 07-bss-get-boot-script-mac1.hurl} | 0 ....hurl => 08-bss-get-boot-script-mac2.hurl} | 0 ...hurl => 09-bss-delete-by-boot-config.hurl} | 0 ...l => 10-smd-delete-redfish-endpoints.hurl} | 0 ...nts.hurl => 11-smd-delete-components.hurl} | 0 ...=> 12-smd-delete-component-endpoints.hurl} | 0 10 files changed, 194 insertions(+), 15 deletions(-) rename test/ct/postgres/tests/mac/{05-bss-delete-mac.hurl => 05-bss-add-duplicate-mac.hurl} (62%) create mode 100644 test/ct/postgres/tests/mac/06-bss-delete-mac.hurl rename test/ct/postgres/tests/mac/{06-bss-get-boot-script-mac1.hurl => 07-bss-get-boot-script-mac1.hurl} (100%) rename test/ct/postgres/tests/mac/{07-bss-get-boot-script-mac2.hurl => 08-bss-get-boot-script-mac2.hurl} (100%) rename test/ct/postgres/tests/mac/{08-bss-delete-by-boot-config.hurl => 09-bss-delete-by-boot-config.hurl} (100%) rename test/ct/postgres/tests/mac/{09-smd-delete-redfish-endpoints.hurl => 10-smd-delete-redfish-endpoints.hurl} (100%) rename test/ct/postgres/tests/mac/{10-smd-delete-components.hurl => 11-smd-delete-components.hurl} (100%) rename test/ct/postgres/tests/mac/{11-smd-delete-component-endpoints.hurl => 12-smd-delete-component-endpoints.hurl} (100%) diff --git a/internal/postgres/postgres.go b/internal/postgres/postgres.go index 300cbac..d2d0745 100644 --- a/internal/postgres/postgres.go +++ b/internal/postgres/postgres.go @@ -112,11 +112,12 @@ func Connect(host string, port uint, user, password string, ssl bool) (BootDataD return bddb, err } -// NewNode creates a new Node and populates it with the boot MAC address, XName, and NID specified. -// Before returning the Node, its ID is populated with a new unique identifier. +// NewNode creates a new Node and populates it with the boot MAC address (converts to lower case), +// XName, and NID specified. Before returning the Node, its ID is populated with a new unique +// identifier. func NewNode(mac, xname string, nid int32) (n Node) { n.Id = uuid.Generate().String() - n.BootMac = mac + n.BootMac = strings.ToLower(mac) n.Xname = xname n.Nid = nid return n @@ -267,7 +268,12 @@ func (bddb BootDataDatabase) GetNodesByItems(macs, xnames []string, nids []int32 } switch i { case 0: - qstr += fmt.Sprintf(` boot_mac IN %s`, stringSliceToSql(macs)) + // Ignore case when searching by MAC. + var macsLower []string + for _, mac := range macs { + macsLower = append(macsLower, strings.ToLower(mac)) + } + qstr += fmt.Sprintf(` boot_mac IN %s`, stringSliceToSql(macsLower)) case 1: qstr += fmt.Sprintf(` xname IN %s`, stringSliceToSql(xnames)) case 2: @@ -1156,7 +1162,12 @@ func (bddb BootDataDatabase) deleteNodesByItems(hosts, macs []string, nids []int case 0: qstr += fmt.Sprintf(` xname IN %s`, stringSliceToSql(hosts)) case 1: - qstr += fmt.Sprintf(` boot_mac IN %s`, stringSliceToSql(macs)) + // Ignore case when matching MAC addresses. + var macsLower []string + for _, mac := range macs { + macsLower = append(macsLower, strings.ToLower(mac)) + } + qstr += fmt.Sprintf(` boot_mac IN %s`, stringSliceToSql(macsLower)) case 2: qstr += fmt.Sprintf(` nid IN %s`, int32SliceToSql(nids)) } @@ -1257,6 +1268,7 @@ func (bddb BootDataDatabase) deleteBootConfigByGroup(groupNames []string) (nodeL // BootConfig are also deleted. A slice of deleted Node items and a slice of deleted BootConfig // items are returned. If an error occurs with any of the SQL queries, an error is returned. func (bddb BootDataDatabase) deleteNodesWithBootConfigs(hosts, macs []string, nids []int32) (nodeList []Node, bcList []BootConfig, err error) { + // MAC address comparison is case-insensitive. nodeList, err = bddb.deleteNodesByItems(hosts, macs, nids) if err != nil { err = fmt.Errorf("Error deleting Node(s): %v", err) @@ -1404,7 +1416,12 @@ func (bddb BootDataDatabase) Add(bp bssTypes.BootParams) (result map[string]stri // Store list of nodes to add. for _, mac := range bp.Macs { - if existingNodeMap[mac] == (Node{}) { + if _, ok := existingNodeMap[strings.ToLower(mac)]; !ok { + // Store using lower case version of MAC address. When the MAC + // address is compared for retrieval/deletion/update, it will be + // converted into lower case before comparison, effectively ignoring + // case. This will prevent duplicate MAC addresses due to case + // difference. nodesToAdd = append(nodesToAdd, NewNode(mac, "", 0)) } } @@ -1423,6 +1440,11 @@ func (bddb BootDataDatabase) Add(bp bssTypes.BootParams) (result map[string]stri } } + if len(nodesToAdd) == 0 { + err = fmt.Errorf("postgres.Add: No nodes to add (possible duplicate(s))") + return result, err + } + // Add any nonexisting nodes, plus their boot config as needed. result, err = bddb.addBootConfigByNode(nodesToAdd, bp.Kernel, bp.Initrd, bp.Params) if err != nil { @@ -1480,6 +1502,8 @@ func (bddb BootDataDatabase) Delete(bp bssTypes.BootParams) (nodesDeleted, bcsDe } } case len(bp.Macs) > 0: + // This deletion function will ignore the case of the passed MAC addresses by first + // converting them to lower case before comparison. delNodes, delBcs, err = bddb.deleteNodesWithBootConfigs([]string{}, bp.Macs, []int32{}) if err != nil { err = fmt.Errorf("postgres.Delete: %v", err) @@ -1666,11 +1690,17 @@ func (bddb BootDataDatabase) GetBootParamsByMac(macs []string) ([]bssTypes.BootP return results, nil } + // Ignore case for MAC addresses. + var macsLower []string + for _, mac := range macs { + macsLower = append(macsLower, strings.ToLower(mac)) + } + qstr := "SELECT n.boot_mac, bc.kernel_uri, bc.initrd_uri, bc.cmdline FROM nodes AS n" + " LEFT JOIN boot_group_assignments AS bga ON n.id=bga.node_id" + " JOIN boot_groups AS bg on bga.boot_group_id=bg.id" + " JOIN boot_configs AS bc ON bg.boot_config_id=bc.id" + - " WHERE n.boot_mac IN " + stringSliceToSql(macs) + + " WHERE n.boot_mac IN " + stringSliceToSql(macsLower) + ";" rows, err := bddb.DB.Query(qstr) if err != nil { diff --git a/test/ct/postgres/tests/mac/04-bss-add-macs.hurl b/test/ct/postgres/tests/mac/04-bss-add-macs.hurl index 75eed6c..be7a822 100644 --- a/test/ct/postgres/tests/mac/04-bss-add-macs.hurl +++ b/test/ct/postgres/tests/mac/04-bss-add-macs.hurl @@ -10,12 +10,13 @@ POST http://bss:27778/boot/v1/bootparameters HTTP 201 # Add another MAC address to BSS (to see if same boot config got used instead of -# creating another). +# creating another). Also, use an upper-case MAC address. BSS should store this +# as lower case. POST http://bss:27778/boot/v1/bootparameters { "kernel": "https://testkerneluri2.tld", "initrd": "https://testinitrduri2.tld", - "macs": ["02:0b:b8:00:30:04"], + "macs": ["02:0B:B8:00:30:04"], "params": "param3,param4" } diff --git a/test/ct/postgres/tests/mac/05-bss-delete-mac.hurl b/test/ct/postgres/tests/mac/05-bss-add-duplicate-mac.hurl similarity index 62% rename from test/ct/postgres/tests/mac/05-bss-delete-mac.hurl rename to test/ct/postgres/tests/mac/05-bss-add-duplicate-mac.hurl index afc582d..3764921 100644 --- a/test/ct/postgres/tests/mac/05-bss-delete-mac.hurl +++ b/test/ct/postgres/tests/mac/05-bss-add-duplicate-mac.hurl @@ -1,10 +1,15 @@ -# Delete a MAC address. -DELETE http://bss:27778/boot/v1/bootparameters +# Try to add a duplicate MAC address using upper case. This should not add anything. +POST http://bss:27778/boot/v1/bootparameters { - "macs": ["02:0b:b8:00:30:02"] + "kernel": "https://testkerneluri0.tld", + "initrd": "https://testinitrduri0.tld", + "macs": ["02:0B:B8:00:30:04"], + "params": "param0" } -HTTP 200 +HTTP 400 +[Asserts] +jsonpath "$.detail" == "Bad Request: postgres.Add: No nodes to add (possible duplicate(s))" GET http://bss:27778/boot/v1/bootparameters @@ -12,8 +17,9 @@ GET http://bss:27778/boot/v1/bootparameters # # [ # { -# "hosts": [ +# "macs": [ # "02:0b:b8:00:30:00", +# "02:0b:b8:00:30:02", # "02:0b:b8:00:30:04" # ], # "params": "param3,param4", @@ -36,8 +42,9 @@ GET http://bss:27778/boot/v1/bootparameters HTTP 200 Content-Type: application/json; charset=UTF-8 [Asserts] -jsonpath "$[0].macs" count == 2 +jsonpath "$[0].macs" count == 3 jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:00" +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:02" jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:04" jsonpath "$[0].params" == "param3,param4" jsonpath "$[0].kernel" == "https://testkerneluri2.tld" diff --git a/test/ct/postgres/tests/mac/06-bss-delete-mac.hurl b/test/ct/postgres/tests/mac/06-bss-delete-mac.hurl new file mode 100644 index 0000000..2bfd1ef --- /dev/null +++ b/test/ct/postgres/tests/mac/06-bss-delete-mac.hurl @@ -0,0 +1,141 @@ +# Delete a MAC address. Test using an upper-case MAC address to see if comparing +# MAC addresses is case-insensitive. +DELETE http://bss:27778/boot/v1/bootparameters +{ + "macs": ["02:0B:B8:00:30:02"] +} + +HTTP 200 + +GET http://bss:27778/boot/v1/bootparameters + +# Response should be: +# +# [ +# { +# "hosts": [ +# "02:0b:b8:00:30:00", +# "02:0b:b8:00:30:04" +# ], +# "params": "param3,param4", +# "kernel": "https://testkerneluri2.tld", +# "initrd": "https://testinitrduri2.tld", +# "cloud-init": { +# "meta-data": null, +# "user-data": null, +# "phone-home": { +# "pub_key_dsa": "", +# "pub_key_rsa": "", +# "pub_key_ecdsa": "", +# "instance_id": "", +# "hostname": "", +# "fqdn": "" +# } +# } +# } +# ] +HTTP 200 +Content-Type: application/json; charset=UTF-8 +[Asserts] +jsonpath "$[0].macs" count == 2 +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:00" +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:04" +jsonpath "$[0].params" == "param3,param4" +jsonpath "$[0].kernel" == "https://testkerneluri2.tld" +jsonpath "$[0].initrd" == "https://testinitrduri2.tld" + +# Add the MAC address again to test deletion using upper-case MAC address. +POST http://bss:27778/boot/v1/bootparameters +{ + "kernel": "https://testkerneluri2.tld", + "initrd": "https://testinitrduri2.tld", + "macs": ["02:0b:b8:00:30:02"], + "params": "param3,param4" +} + +HTTP 201 + +GET http://bss:27778/boot/v1/bootparameters + +# Response should be: +# +# [ +# { +# "hosts": [ +# "02:0b:b8:00:30:00", +# "02:0b:b8:00:30:02", +# "02:0b:b8:00:30:04" +# ], +# "params": "param3,param4", +# "kernel": "https://testkerneluri2.tld", +# "initrd": "https://testinitrduri2.tld", +# "cloud-init": { +# "meta-data": null, +# "user-data": null, +# "phone-home": { +# "pub_key_dsa": "", +# "pub_key_rsa": "", +# "pub_key_ecdsa": "", +# "instance_id": "", +# "hostname": "", +# "fqdn": "" +# } +# } +# } +# ] +HTTP 200 +Content-Type: application/json; charset=UTF-8 +[Asserts] +jsonpath "$[0].macs" count == 3 +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:00" +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:02" +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:04" +jsonpath "$[0].params" == "param3,param4" +jsonpath "$[0].kernel" == "https://testkerneluri2.tld" +jsonpath "$[0].initrd" == "https://testinitrduri2.tld" + +# Delete the MAC address again, using an upper-case MAC address to see if comparing +# MAC addresses is case-insensitive. +DELETE http://bss:27778/boot/v1/bootparameters +{ + "macs": ["02:0B:B8:00:30:02"] +} + +HTTP 200 + +GET http://bss:27778/boot/v1/bootparameters + +# Response should be: +# +# [ +# { +# "hosts": [ +# "02:0b:b8:00:30:00", +# "02:0b:b8:00:30:04" +# ], +# "params": "param3,param4", +# "kernel": "https://testkerneluri2.tld", +# "initrd": "https://testinitrduri2.tld", +# "cloud-init": { +# "meta-data": null, +# "user-data": null, +# "phone-home": { +# "pub_key_dsa": "", +# "pub_key_rsa": "", +# "pub_key_ecdsa": "", +# "instance_id": "", +# "hostname": "", +# "fqdn": "" +# } +# } +# } +# ] +HTTP 200 +Content-Type: application/json; charset=UTF-8 +[Asserts] +jsonpath "$[0].macs" count == 2 +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:00" +jsonpath "$[0].macs[*]" includes "02:0b:b8:00:30:04" +jsonpath "$[0].params" == "param3,param4" +jsonpath "$[0].kernel" == "https://testkerneluri2.tld" +jsonpath "$[0].initrd" == "https://testinitrduri2.tld" diff --git a/test/ct/postgres/tests/mac/06-bss-get-boot-script-mac1.hurl b/test/ct/postgres/tests/mac/07-bss-get-boot-script-mac1.hurl similarity index 100% rename from test/ct/postgres/tests/mac/06-bss-get-boot-script-mac1.hurl rename to test/ct/postgres/tests/mac/07-bss-get-boot-script-mac1.hurl diff --git a/test/ct/postgres/tests/mac/07-bss-get-boot-script-mac2.hurl b/test/ct/postgres/tests/mac/08-bss-get-boot-script-mac2.hurl similarity index 100% rename from test/ct/postgres/tests/mac/07-bss-get-boot-script-mac2.hurl rename to test/ct/postgres/tests/mac/08-bss-get-boot-script-mac2.hurl diff --git a/test/ct/postgres/tests/mac/08-bss-delete-by-boot-config.hurl b/test/ct/postgres/tests/mac/09-bss-delete-by-boot-config.hurl similarity index 100% rename from test/ct/postgres/tests/mac/08-bss-delete-by-boot-config.hurl rename to test/ct/postgres/tests/mac/09-bss-delete-by-boot-config.hurl diff --git a/test/ct/postgres/tests/mac/09-smd-delete-redfish-endpoints.hurl b/test/ct/postgres/tests/mac/10-smd-delete-redfish-endpoints.hurl similarity index 100% rename from test/ct/postgres/tests/mac/09-smd-delete-redfish-endpoints.hurl rename to test/ct/postgres/tests/mac/10-smd-delete-redfish-endpoints.hurl diff --git a/test/ct/postgres/tests/mac/10-smd-delete-components.hurl b/test/ct/postgres/tests/mac/11-smd-delete-components.hurl similarity index 100% rename from test/ct/postgres/tests/mac/10-smd-delete-components.hurl rename to test/ct/postgres/tests/mac/11-smd-delete-components.hurl diff --git a/test/ct/postgres/tests/mac/11-smd-delete-component-endpoints.hurl b/test/ct/postgres/tests/mac/12-smd-delete-component-endpoints.hurl similarity index 100% rename from test/ct/postgres/tests/mac/11-smd-delete-component-endpoints.hurl rename to test/ct/postgres/tests/mac/12-smd-delete-component-endpoints.hurl From bb0901aa607faa5b145520251511b735e3f13212 Mon Sep 17 00:00:00 2001 From: Devon Bautista Date: Mon, 23 Oct 2023 16:12:04 -0600 Subject: [PATCH 2/2] Remove period from error messages; fix comment typo --- internal/postgres/postgres.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/postgres/postgres.go b/internal/postgres/postgres.go index d2d0745..7819c53 100644 --- a/internal/postgres/postgres.go +++ b/internal/postgres/postgres.go @@ -585,7 +585,7 @@ func (bddb BootDataDatabase) addBootConfigByGroup(groupNames []string, kernelUri results := make(map[string]string) if len(groupNames) == 0 { - return results, fmt.Errorf("No group names specified to add.") + return results, fmt.Errorf("No group names specified to add") } // See if group name exists, if passed. @@ -713,7 +713,7 @@ func (bddb BootDataDatabase) addBootConfigByNode(nodeList []Node, kernelUri, ini bgaList []BootGroupAssignment ) if len(nodeList) == 0 { - return result, fmt.Errorf("No nodes specified to add boot configurations for.") + return result, fmt.Errorf("No nodes specified to add boot configurations for") } existingBgList, existingBcList, numResults, err = bddb.GetBootConfigsByItems(kernelUri, initrdUri, cmdline) if err != nil { @@ -804,7 +804,7 @@ func (bddb BootDataDatabase) addBootConfigByNode(nodeList []Node, kernelUri, ini // with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteBootGroupsByName(names []string) (bgList []BootGroup, err error) { if len(names) == 0 { - err = fmt.Errorf("No boot group names specified to delete.") + err = fmt.Errorf("No boot group names specified to delete") return bgList, err } // "RETURNING *" is Postgres-specific. @@ -840,7 +840,7 @@ func (bddb BootDataDatabase) deleteBootGroupsByName(names []string) (bgList []Bo // an error occurs with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteBootGroupsById(bgIds []string) (bgList []BootGroup, err error) { if len(bgIds) == 0 { - err = fmt.Errorf("No boot group IDs specified to delete.") + err = fmt.Errorf("No boot group IDs specified to delete") return bgList, err } // "RETURNING *" is Postgres-specific. @@ -876,7 +876,7 @@ func (bddb BootDataDatabase) deleteBootGroupsById(bgIds []string) (bgList []Boot // with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteBootConfigsById(bcIds []string) (bcList []BootConfig, err error) { if len(bcIds) == 0 { - err = fmt.Errorf("No boot config IDs specified to delete.") + err = fmt.Errorf("No boot config IDs specified to delete") return bcList, err } // "RETURNING *" is Postgres-specific. @@ -1041,7 +1041,7 @@ func (bddb BootDataDatabase) deleteBootConfigsByItems(kernelUri, initrdUri, cmdl // were deleted. If an error occurs with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteBootGroupAssignmentsByGroupId(bgIds []string) (bgaList []BootGroupAssignment, err error) { if len(bgIds) == 0 { - err = fmt.Errorf("No boot group IDs specified for deleting boot group assignments.") + err = fmt.Errorf("No boot group IDs specified for deleting boot group assignments") return bgaList, err } // "RETURNING *" is Postgres-specific. @@ -1077,7 +1077,7 @@ func (bddb BootDataDatabase) deleteBootGroupAssignmentsByGroupId(bgIds []string) // occurs with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteBootGroupAssignmentsByNodeId(nodeIds []string) (bgaList []BootGroupAssignment, err error) { if len(nodeIds) == 0 { - err = fmt.Errorf("No node IDs specified for deleting boot group assignments.") + err = fmt.Errorf("No node IDs specified for deleting boot group assignments") return bgaList, err } // "RETURNING *" is Postgres-specific. @@ -1112,7 +1112,7 @@ func (bddb BootDataDatabase) deleteBootGroupAssignmentsByNodeId(nodeIds []string // an error occurs with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteNodesById(nodeIds []string) (nodeList []Node, err error) { if len(nodeIds) == 0 { - err = fmt.Errorf("No node IDs specified for deletion.") + err = fmt.Errorf("No node IDs specified for deletion") return nodeList, err } // "RETURNING *" is Postgres-specific. @@ -1148,7 +1148,7 @@ func (bddb BootDataDatabase) deleteNodesById(nodeIds []string) (nodeList []Node, // nodes is returned. If an error occurs with any of the SQL queries, it is returned. func (bddb BootDataDatabase) deleteNodesByItems(hosts, macs []string, nids []int32) (nodeList []Node, err error) { if len(hosts) == 0 && len(macs) == 0 && len(nids) == 0 { - err = fmt.Errorf("No hosts, MAC addresses, or NIDs specified to delete nodes.") + err = fmt.Errorf("No hosts, MAC addresses, or NIDs specified to delete nodes") return nodeList, err } qstr := `DELETE FROM nodes WHERE` @@ -1208,7 +1208,7 @@ func (bddb BootDataDatabase) deleteNodesByItems(hosts, macs []string, nids []int // SQL queries occurs, it is returned. func (bddb BootDataDatabase) deleteBootConfigByGroup(groupNames []string) (nodeList []Node, bcList []BootConfig, err error) { if len(groupNames) == 0 { - return nodeList, bcList, fmt.Errorf("No group names specified for deletion.") + return nodeList, bcList, fmt.Errorf("No group names specified for deletion") } // Delete matching boot groups, store deleted ones. @@ -1490,7 +1490,7 @@ func (bddb BootDataDatabase) Delete(bp bssTypes.BootParams) (nodesDeleted, bcsDe // Group name(s) specified, add boot config by group. delNodes, delBcs, err = bddb.deleteBootConfigByGroup(groupNames) if err != nil { - err = fmt.Errorf("postgres.Add: %v", err) + err = fmt.Errorf("postgres.Delete: %v", err) return nodesDeleted, bcsDeleted, err } } else if len(xNames) > 0 { @@ -1685,7 +1685,7 @@ func (bddb BootDataDatabase) GetBootParamsByName(names []string) ([]bssTypes.Boo func (bddb BootDataDatabase) GetBootParamsByMac(macs []string) ([]bssTypes.BootParams, error) { var results []bssTypes.BootParams - // If inout is empty, so is the output. + // If input is empty, so is the output. if len(macs) == 0 { return results, nil }