fix(dao) apply cache_key for target uniqueness detection

[yankun-li-kong]

Summary

Apply cache_key for target uniqueness detection
Improvement for #6483

Full changelog

• Add new cache_key(upstream, target) in targets table for uniqueness detection.
• Delete useless targets uniqueness detection functions.
• Targets API returns 409 when creating/updating delicate targets.
• Add migration functions to add cache_key column, delete duplicate targets, add cache_key for existing targets.

Issues resolved

CT-97

[yankun-li-kong]

Hi @locao would like to discuss below points

I have deleted the functions you wrote in kong/api/routes/upstreams.lua.
It will changed the behavior of POST target API.

Before:
POST target API is able to create new targets or update existing targets

After:
POST target API is only able to create new targets (See test case diff)
You have to use PATCH target API to update existing targets

Please let me know your opinion.
I think you may know the reason why we use POST target to update existing targets.

If we want to keep previous behavior(see Before), then I should not modify kong/api/routes/upstreams.lua

[dndx]

Related to #6483. Please see comments there for context.

[flrgh]

Question: Are there any safety/reentrantcy/atomicity concerns with performing these updates within the SELECT loop? Would it be any safer (or less safe?) to build a full list of rows (exhausting the SELECT iterator) and perform the updates in a separate loop, or is the difference negligible?

[yankun-li-kong]

Thanks for your suggestion!
I think it should not make much difference as the flow is still the same as below

SELECT from targets and pass targets list to rows
then execute UPDATE targets in a loop with rows

[locao]

Thanks for bringing this up @yankun-li-kong! That's something we've been waiting for 3.0 to do.

The code seems OK to me, just a few suggestions on the Cassandra migration. My main concern is that we should not merge this change to master at this moment. We are still having another release before 3.0.

[locao]

It makes sense to change the POST behaviour, but shouldn't we wait until 3.0 to merge this?

[yankun-li-kong]

Got it. Please let me know when we should merge it so I could rename 012_213_to_220.lua file

[locao]

Since the target history removal in 2.2 we don't keep deleted targets.

[yankun-li-kong]

Thanks! I saw kong has run c_remove_unused_targets in 012_213_to_220.lua to delete duplicate targets in 2.2.

But duplicate targets may still exist after that.
Regarding https://github.com/Kong/kong/blob/master/kong/db/dao/targets.lua#L50-L59
kong will do uniqueness detection and then insert targets.
It is not an atomic operation and a duplicate target may be added during the above process.

[locao]

(somehow this comment was not added to my review)

I haven't tested this suggestion, but it seems to me that it's possible to delete the duplicated targets in a single loop.

Suggested change

	upstream_targets[key] = { n = 0 }
	end
	upstream_targets[key].n = upstream_targets[key].n + 1
	upstream_targets[key][upstream_targets[key].n] = { row.id, row.created_at }
	end
	end
	local sort = function(a, b)
	return a[2] > b[2]
	end
	for _, targets in pairs(upstream_targets) do
	if targets.n > 1 then
	table.sort(targets, sort)
	for i = 2, targets.n do
	local _, err = coordinator:execute("DELETE FROM targets WHERE id = ?", {
	cassandra.uuid(targets[i][1])
	})
	upstream_targets[key] = {
	id = row.id,
	created_at = row.created_at,
	}
	else
	local to_remove
	if row.created_at > upstream_targets[key].created_at then
	to_remove = upstream_targets[key].id
	upstream_targets[key] = {
	id = row.id,
	created_at = row.created_at,
	}
	else
	to_remove = row.id
	end
	local _, err = coordinator:execute("DELETE FROM targets WHERE id = ?", {
	cassandra.uuid(to_remove)
	})

[yankun-li-kong]

Fixed and tested in my local.
It looks much simple than previous one!

[dndx]

Hello @yankun-li-kong ,

Could you rebase this onto the latest master so it can be ready for merge? Since this will be release in 3.0, a new migration file will be needed. We also need some entry inside the CHANGELOG.md file describing this change.