fix: correct tags filtering on consumer-group consumers #88

GGabriele · 2024-05-08T16:47:39Z

Due to the nature of the Consumer Group API, it's not possible
to natively filter its Consumers using tags:

$ http :8001/consumer_groups/foo_group_1/consumers\?tags="managed-by:deck"
HTTP/1.1 400 Bad Request

{
    "code": 11,
    "message": "invalid option (tags: cannot be used with 'consumer_group_consumers')",
    "name": "invalid options",
    "options": {
        "tags": "cannot be used with 'consumer_group_consumers'"
    }
}

So, in order to fetch ConsumerGroup:Consumer mappings, we need to rely
on the unfiltered Consumer Group API:

$ http :8001/consumer_groups/foo_group_1
HTTP/1.1 200 OK

{
    "consumer_group": {
        "created_at": 1715183554,
        "id": "e0e495f2-9acd-41f7-b4da-33659033af13",
        "name": "foo_group_1",
        "tags": [
            "managed-by-deck"
        ],
        "updated_at": 1715183554
    },
    "consumers": [
        {
            "created_at": 1715183563,
            "id": "a704b13e-1a9b-409b-9611-fd2bfcea63bb",
            "tags": [
                "managed-by:api"
            ],
            "type": 0,
            "updated_at": 1715183563,
            "username": "foo",
            "username_lower": "foo"
        }
    ]
}

This is a problem in cases when Consumer Groups are managed by decK
while their mappings with Consumers are managed directly via the API.
In such case, since the current library implementation doesn't allow
to filter mappings by tags, decK would wipe out the API managed
mappings when running against the specific tags it handles:

$ cat kong.yaml
_format_version: "3.0"
_info:
  select_tags:
  - managed-by:deck
consumer_groups:
- id: c0f6c818-470c-4df7-8515-c8e904765fcc
  name: foo_group_1
  tags:
  - managed-by:deck

$ deck gateway sync kong.yaml
deleting consumer-group-consumer foo
Summary:
  Created: 0
  Updated: 0
  Deleted: 1

This commit makes sure the correct filtering is "manually" done,
avoiding existing Consumers to be wiped out during syncs when using
Tags:

$ deck gateway sync kong.yaml
Summary:
  Created: 0
  Updated: 0
  Deleted: 0

$ http :8001/consumer_groups/foo_group_1
HTTP/1.1 200 OK

{
    "consumer_group": {
        "created_at": 1716450737,
        "id": "c0f6c818-470c-4df7-8515-c8e904765fcc",
        "name": "foo_group_1",
        "tags": [
            "managed-by:deck"
        ],
        "updated_at": 1716451431
    },
    "consumers": [
        {
            "created_at": 1716450861,
            "id": "bafd4f5d-3f4e-4c0f-8d6c-6901b0b8f483",
            "tags": [
                "managed-by:api"
            ],
            "type": 0,
            "updated_at": 1716450861,
            "username": "foo"
        }
    ]
}

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88

rainest

Can you clarify the specifics of

nature of the Consumer Group API

It looks like gateway has done something strange re the entity relationships and has added/already had an API endpoint that allows you to return the consumers that are (sort of, because of the weird many to many linking entity in the Kong DAO) nested under a consumer group, but that doesn't respect tag filters on the nested entities? For example:

15:19:15-0700 esenin $ http :8001/consumer_groups/fooga?tags=bagga
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 478
Content-Type: application/json; charset=utf-8
Date: Wed, 22 May 2024 22:19:20 GMT
Server: kong/3.6.1.4-enterprise-edition
X-Kong-Admin-Latency: 9
X-Kong-Admin-Request-ID: b99599102f1c77551a34f71ce694d62e

{
    "consumer_group": {
        "created_at": 1716416072,
        "id": "79d347c5-b83c-49ad-be5a-9f7458c31d3b",
        "name": "fooga",
        "tags": [
            "tagga"
        ],
        "updated_at": 1716416072
    },
    "consumers": [
        {
            "created_at": 1716416082,
            "id": "44d7ca02-684f-40bd-97d9-76ab26d8d5bd",
            "tags": [
                "bagga"
            ],
            "type": 0,
            "updated_at": 1716416082,
            "username": "bar",
            "username_lower": "bar"
        },
        {
            "created_at": 1716416053,
            "id": "9c23faee-3688-46dd-a964-13a0e99808b1",
            "tags": [
                "tagga"
            ],
            "type": 0,
            "updated_at": 1716416053,
            "username": "foo",
            "username_lower": "foo"
        }
    ]
}

"shouldn't" include the consumer with the tagga tag (though IDK how you really reconcile that with the same filter omitting the consumer_group in question).

I'm not entirely sure what the expected and broken behaviors are here; this should have a documented current behavior and expected behavior so reviewers can properly vet the change (and maybe the expected behavior).

Kong/kubernetes-ingress-controller#6072 indicates that this doesn't obviously break anything with respect to KIC's tag filtering use case. IDK if there are as-yet unknown breakages without the current/expected behavior context.

rainest · 2024-05-22T22:37:22Z

pkg/utils/tags.go

@@ -82,3 +82,33 @@ func RemoveTags(obj interface{}, tags []string) error {
 	structTags.Set(res)
 	return nil
 }
+
+func HasTags(obj interface{}, tags []string) bool {


The use of reflect here and throughout these utilities is rather hideous. We control these object definitions and really should have some proper Tag-related methods and interfaces defined in https://github.com/Kong/go-kong/blob/main/kong/consumer.go and such.

However, it probably gets a pass here due to the existing pattern ☹️

+1 to what @rainest said.

Also I'd add a godoc here so that we're clear if we're checking all tags or at least one of the provided tags.

Due to the nature of the Consumer Group API, it's not possible to natively filter its Consumers using tags: ``` $ http :8001/consumer_groups/foo_group_1/consumers\?tags="managed-by:deck" HTTP/1.1 400 Bad Request { "code": 11, "message": "invalid option (tags: cannot be used with 'consumer_group_consumers')", "name": "invalid options", "options": { "tags": "cannot be used with 'consumer_group_consumers'" } } ``` So, in order to fetch ConsumerGroup:Consumer mappings, we need to rely on the unfiltered Consumer Group API: ``` $ http :8001/consumer_groups/foo_group_1 HTTP/1.1 200 OK { "consumer_group": { "created_at": 1715183554, "id": "e0e495f2-9acd-41f7-b4da-33659033af13", "name": "foo_group_1", "tags": [ "managed-by-deck" ], "updated_at": 1715183554 }, "consumers": [ { "created_at": 1715183563, "id": "a704b13e-1a9b-409b-9611-fd2bfcea63bb", "tags": [ "managed-by:api" ], "type": 0, "updated_at": 1715183563, "username": "foo", "username_lower": "foo" } ] } ``` This is a problem in cases when Consumer Groups are managed by decK while their mappings with Consumers are managed directly via the API. In such case, since the current library implementation doesn't allow to filter mappings by tags, decK would wipe out the API managed mappings when running against the specific tags it handle: ``` $ cat kong.yaml _format_version: "3.0" _info: select_tags: - managed-by:deck consumer_groups: - id: c0f6c818-470c-4df7-8515-c8e904765fcc name: foo_group_1 tags: - managed-by:deck ``` ``` $ deck gateway sync kong.yaml deleting consumer-group-consumer foo Summary: Created: 0 Updated: 0 Deleted: 1 ``` This commit makes sure the correct filtering is "manually" done, avoiding existing Consumers to be wiped out during syncs when using Tags: ``` $ deck gateway sync kong.yaml Summary: Created: 0 Updated: 0 Deleted: 0 ``` ``` $ http :8001/consumer_groups/foo_group_1 HTTP/1.1 200 OK { "consumer_group": { "created_at": 1716450737, "id": "c0f6c818-470c-4df7-8515-c8e904765fcc", "name": "foo_group_1", "tags": [ "managed-by:deck" ], "updated_at": 1716451431 }, "consumers": [ { "created_at": 1716450861, "id": "bafd4f5d-3f4e-4c0f-8d6c-6901b0b8f483", "tags": [ "managed-by:api" ], "type": 0, "updated_at": 1716450861, "username": "foo" } ] } ```

GGabriele · 2024-05-23T08:10:11Z

@rainest I added more context in the PR description and commit message. Let me know if that helps.

codecov-commenter · 2024-05-23T08:11:01Z

Codecov Report

Attention: Patch coverage is 46.15385% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 41.02%. Comparing base (54a82f6) to head (2b0243b).
Report is 5 commits behind head on main.

Files	Patch %	Lines
pkg/dump/dump.go	0.00%	7 Missing ⚠️
pkg/utils/tags.go	63.15%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   40.89%   41.02%   +0.13%     
==========================================
  Files          73       74       +1     
  Lines       10202    10761     +559     
==========================================
+ Hits         4172     4415     +243     
- Misses       5612     5886     +274     
- Partials      418      460      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pmalek · 2024-05-23T09:42:12Z

pkg/utils/tags.go

+func HasTags(obj interface{}, tags []string) bool {
+	if len(tags) == 0 {
+		return true
+	}
+
+	m := make(map[string]bool)
+	for _, tag := range tags {
+		m[tag] = true
+	}
+
+	ptr := reflect.ValueOf(obj)
+	if ptr.Kind() != reflect.Ptr {
+		return false
+	}
+	v := reflect.Indirect(ptr)
+	structTags := v.FieldByName("Tags")
+	var zero reflect.Value
+	if structTags == zero {
+		return false
+	}
+
+	for i := 0; i < structTags.Len(); i++ {
+		tag := reflect.Indirect(structTags.Index(i)).String()
+		if m[tag] {
+			return true
+		}
+	}
+	return false
+}


Not using reflection but instead using generics not only will constrain the types at compilation but will also be faster (see my take on this at https://github.com/Kong/go-database-reconciler/compare/fix/consumer-group-consumers-tags...pmalek-consumer-group-tags?expand=1)

$ go test -run NONE -benchmem -bench . -count 5 ./pkg/utils/ | tee old goos: darwin goarch: arm64 pkg: github.com/kong/go-database-reconciler/pkg/utils BenchmarkHasTags-10 12694275 94.74 ns/op 8 B/op 1 allocs/op BenchmarkHasTags-10 12648442 94.51 ns/op 8 B/op 1 allocs/op BenchmarkHasTags-10 12533312 94.94 ns/op 8 B/op 1 allocs/op BenchmarkHasTags-10 12569464 94.53 ns/op 8 B/op 1 allocs/op BenchmarkHasTags-10 12564386 94.52 ns/op 8 B/op 1 allocs/op PASS ok github.com/kong/go-database-reconciler/pkg/utils 6.460s $ go test -run NONE -benchmem -bench . -count 5 ./pkg/utils/ | tee new goos: darwin goarch: arm64 pkg: github.com/kong/go-database-reconciler/pkg/utils BenchmarkHasTags-10 27593924 40.55 ns/op 0 B/op 0 allocs/op BenchmarkHasTags-10 29650494 40.54 ns/op 0 B/op 0 allocs/op BenchmarkHasTags-10 29476394 40.66 ns/op 0 B/op 0 allocs/op BenchmarkHasTags-10 29024270 40.47 ns/op 0 B/op 0 allocs/op BenchmarkHasTags-10 29232762 40.43 ns/op 0 B/op 0 allocs/op PASS ok github.com/kong/go-database-reconciler/pkg/utils 6.101s $ benchstat -confidence 0.6 old new goos: darwin goarch: arm64 pkg: github.com/kong/go-database-reconciler/pkg/utils │ old │ new │ │ sec/op │ sec/op vs base │ HasTags-10 94.53n ± 0% 40.54n ± 0% -57.11% (p=0.008 n=5) │ old │ new │ │ B/op │ B/op vs base │ HasTags-10 8.000 ± 0% 0.000 ± 0% -100.00% (p=0.008 n=5) │ old │ new │ │ allocs/op │ allocs/op vs base │ HasTags-10 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.008 n=5)

Nice! Please feel free to push your suggestions directly to this PR

Done, PTAL at https://github.com/Kong/go-database-reconciler/pull/88/files#r1611384232 and https://github.com/Kong/go-database-reconciler/pull/88/files#r1611385143

pkg/utils/tags.go

pkg/utils/tags_test.go

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>

pmalek · 2024-05-23T10:22:22Z

pkg/utils/tags.go

@@ -82,3 +82,29 @@ func RemoveTags(obj interface{}, tags []string) error {
 	structTags.Set(res)
 	return nil
 }
+
+func HasTags[T *kong.Consumer](obj T, tags []string) bool {


My changes didn't involve the kong import. Added that in a8d30fc

@GGabriele Can you please add the godoc to this func?

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

GGabriele requested a review from a team May 8, 2024 16:47

GGabriele added a commit to Kong/deck that referenced this pull request May 8, 2024

chore: bump go-database-reconciler dependency

eda4343

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88

GGabriele mentioned this pull request May 8, 2024

chore: bump go-database-reconciler dependency Kong/deck#1293

Merged

GGabriele added a commit to Kong/deck that referenced this pull request May 8, 2024

chore: bump go-database-reconciler dependency

9989a69

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88

rainest mentioned this pull request May 22, 2024

tmp: test unreleased GDR Kong/kubernetes-ingress-controller#6072

Closed

rainest reviewed May 22, 2024

View reviewed changes

GGabriele force-pushed the fix/consumer-group-consumers-tags branch from 9fc2338 to 917fcee Compare May 23, 2024 08:07

pmalek reviewed May 23, 2024

View reviewed changes

pkg/utils/tags.go Outdated Show resolved Hide resolved

pmalek reviewed May 23, 2024

View reviewed changes

pkg/utils/tags_test.go Outdated Show resolved Hide resolved

GGabriele and others added 3 commits May 23, 2024 12:02

Update pkg/utils/tags.go

5b9fc3d

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>

Update pkg/utils/tags_test.go

072acbd

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>

chore: add missing import

a8d30fc

pmalek reviewed May 23, 2024

View reviewed changes

chore: add docstring to HasTags

2b0243b

pmalek approved these changes May 23, 2024

View reviewed changes

GGabriele enabled auto-merge (squash) May 23, 2024 12:08

Merge branch 'main' into fix/consumer-group-consumers-tags

d85eb5c

GGabriele merged commit ff4cfa3 into main May 23, 2024
42 checks passed

GGabriele deleted the fix/consumer-group-consumers-tags branch May 23, 2024 18:15

rainest pushed a commit to Kong/deck that referenced this pull request May 23, 2024

chore: bump go-database-reconciler dependency

605697f

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

rainest pushed a commit to Kong/deck that referenced this pull request May 23, 2024

chore: bump go-database-reconciler dependency

f610064

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

GGabriele added a commit to Kong/deck that referenced this pull request May 27, 2024

chore: bump go-database-reconciler dependency

8ad1bb0

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

GGabriele added a commit to Kong/deck that referenced this pull request May 27, 2024

chore: bump go-database-reconciler dependency

fa598ae

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

rainest pushed a commit to Kong/deck that referenced this pull request May 27, 2024

chore: bump GDR (#1293)

5d3a1ee

Fixes filtering of consumer-group consumers when using tags Kong/go-database-reconciler#88 Add a deck-side test of the fixed behavior.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correct tags filtering on consumer-group consumers #88

fix: correct tags filtering on consumer-group consumers #88

GGabriele commented May 8, 2024 •

edited

Loading

rainest left a comment

rainest May 22, 2024

pmalek May 23, 2024

GGabriele commented May 23, 2024

codecov-commenter commented May 23, 2024 •

edited

Loading

pmalek May 23, 2024

GGabriele May 23, 2024

pmalek May 23, 2024

pmalek May 23, 2024

pmalek May 23, 2024

GGabriele May 23, 2024

fix: correct tags filtering on consumer-group consumers #88

fix: correct tags filtering on consumer-group consumers #88

Conversation

GGabriele commented May 8, 2024 • edited Loading

rainest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GGabriele commented May 23, 2024

codecov-commenter commented May 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GGabriele commented May 8, 2024 •

edited

Loading

codecov-commenter commented May 23, 2024 •

edited

Loading