object.ID: sortedTags contains empty elements

[oxzi]

The ID function in internal/object/object.go creates the sortedTags slice with len(tags) empty fields, before adding len(tags) populated fields.

icinga-notifications/internal/object/object.go

Lines 205 to 216 in 710c818

	sortedTags := make([]KV, len(tags))
	for k, v := range tags {
	sortedTags = append(sortedTags, KV{k, v})
	}
	sort.Slice(sortedTags, func(i, j int) bool { return sortedTags[i].K < sortedTags[j].K })
	for _, kv := range sortedTags {
	h.Write([]byte(kv.K))
	h.Write([]byte{0})
	h.Write([]byte(kv.V))
	h.Write([]byte{0})
	}

This is most likely a bug and the code should be make([]KV, 0, len(tags)) to create the slice with a predefined capacity. However, due to the function warning this is maybe not something we can just fix.

icinga-notifications/internal/object/object.go

Lines 187 to 189 in 710c818

	// ID generates a stable identifier based on a source ID and tags.
	//
	// TODO: the return value of this function must be stable like forever

A simplified demo is available on the Go Playground: https://go.dev/play/p/TtBg3FsIG0e

Thanks to @BastianLedererIcinga for finding this issue while working on a reimplementation on the web side.

[yhabteab]

However, due to the function warning this is maybe not something we can just fix.

Why not? Shouldn't be possible to drop all these null bytes from the database in the upgrade script?

[julianbrost]

If you change that function, all objects change their ID and basically get a new identity. It's possible but either means you need to clear the database on upgrade, or need to implement a database migration that recomputes all IDs and changes them consistently in the database (doing that im SQL-only will be painful).

Shouldn't be possible to drop all these null bytes from the database in the upgrade script?

The database only stores a hash, so from that one you don't see what went into it.

[yhabteab]

It's possible but either means you need to clear the database on upgrade, or need to implement a database migration that recomputes all IDs and changes them consistently in the database (doing that im SQL-only will be painful).

In PostgreSQL, this is kind of trivial and can be done in a single transaction like this (note: I've omitted some tables here that are going to be dropped in the v1.0 release, so the tx only contains the tables that are going to be kept.):

START TRANSACTION ISOLATION LEVEL SERIALIZABLE;
CREATE EXTENSION IF NOT EXISTS pgcrypto;

ALTER TABLE object ADD COLUMN id_tmp bytea;

UPDATE object SET id_tmp = digest(source_id::bytea || id_tags.tags, 'sha256') FROM (
  -- We need to aggregate the tags for each object_id into a single bytea value, ordered by tag name,
  -- and separated by null bytes just like in the original (Go-code) hash computation.
  SELECT object_id, string_agg(tag::bytea || '\x00'::bytea || value::bytea || '\x00'::bytea, ''::bytea ORDER BY tag COLLATE "C") AS tags
    FROM object_id_tag AS id_tag
  GROUP BY object_id
) AS id_tags
WHERE object.id = id_tags.object_id;

ALTER TABLE object_id_tag DROP CONSTRAINT fk_object_id_tag_object;
ALTER TABLE incident DROP CONSTRAINT fk_incident_object;

UPDATE object_id_tag SET object_id = (SELECT id_tmp FROM object WHERE object.id = object_id_tag.object_id);
UPDATE incident SET object_id = (SELECT id_tmp FROM object WHERE object.id = incident.object_id);
UPDATE object SET id = id_tmp;

ALTER TABLE object_id_tag ADD CONSTRAINT fk_object_id_tag_object FOREIGN KEY(object_id) REFERENCES object(id);
ALTER TABLE incident ADD CONSTRAINT fk_incident_object FOREIGN KEY(object_id) REFERENCES object(id);

ALTER TABLE object DROP COLUMN id_tmp;

DROP EXTENSION pgcrypto;
COMMIT;

I have tested it and produces the same IDs as the fixed Go code. The pgcrypto extension isn't even needed if we would require PostgreSQL 11 or later, since it includes a built-in sha256 function.

For MySQL/MariaDB, however, I didn't manage to get it working but thinking more about it, we should just clear the object and all related tables on upgrade as the data recorded prior to v1.0 isn't really critical, instead of keeping this buggy code around forever.