Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support ConsumerGroups in KongRawStateToKongState #5438

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

programmer04
Copy link
Member

What this PR does / why we need it:

Support properly ConsumerGroups when fallback to the last valid configuration. Function configfetcher.KongRawStateToKongState now supports ConsumerGroup entities. It means that if KIC fetches the last valid configuration from Gateways, it will include all ConsumerGroups that a Gateway may return.

Which issue this PR fixes:

Fixes #5377

Special notes for your reviewer:

Having explicitly omitted checking this in internal/dataplane/configfetcher/kongrawstate_test.go:318 caused that the test hasn't caught it during the work on ConsumerGroup. I thing the same may happen for Vault 🤔

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@programmer04 programmer04 added this to the KIC v3.1.x milestone Jan 15, 2024
@programmer04 programmer04 self-assigned this Jan 15, 2024
@programmer04 programmer04 requested a review from a team as a code owner January 15, 2024 12:51
Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9e95e9) 69.3% compared to head (cc7d0f7) 69.4%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #5438   +/-   ##
=====================================
  Coverage   69.3%   69.4%           
=====================================
  Files        176     176           
  Lines      22379   22386    +7     
=====================================
+ Hits       15529   15556   +27     
+ Misses      5925    5911   -14     
+ Partials     925     919    -6     

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

@czeslavo
Copy link
Contributor

Having explicitly omitted checking this in internal/dataplane/configfetcher/kongrawstate_test.go:318 caused that the test hasn't caught it during the work on ConsumerGroup. I thing the same may happen for Vault 🤔

Yup, the intention was that once implementing a new field in KongState we will remove the exclude from the test. But I think we might improve that and instead of asserting all of KongRawState - excluded fields to be covered in tests to assert all of kongstate.KongState fields to be covered as that's the struct KIC defines and once we add anything there, it should be also covered in the mapping from KongRawState. That would be something like this:

--- a/internal/dataplane/configfetcher/kongrawstate_test.go
+++ b/internal/dataplane/configfetcher/kongrawstate_test.go
@@ -277,7 +277,7 @@ func TestKongRawStateToKongState(t *testing.T) {
 
                        // Collect all fields that are tested in this test case.
                        if tt.kongRawState != nil {
-                               testedKongRawStateFields.Insert(extractNotEmptyFieldNames(*tt.kongRawState)...)
+                               testedKongRawStateFields.Insert(extractNotEmptyFieldNames(*tt.expectedKongState)...)
                        }
 
                        var state *kongstate.KongState
@@ -290,12 +290,12 @@ func TestKongRawStateToKongState(t *testing.T) {
                })
        }
 
-       ensureAllKongRawStateFieldsAreTested(t, testedKongRawStateFields.UnsortedList())
+       ensureAllKongStateFieldsAreTested(t, testedKongRawStateFields.UnsortedList())
 }
 
-// extractNotEmptyFieldNames returns the names of all non-empty fields in the given KongRawState.
+// extractNotEmptyFieldNames returns the names of all non-empty fields in the given KongState.
 // This is to programmatically find out what fields are used in a test case.
-func extractNotEmptyFieldNames(s utils.KongRawState) []string {
+func extractNotEmptyFieldNames(s kongstate.KongState) []string {
        var fields []string
        typ := reflect.ValueOf(s).Type()
        for i := 0; i < typ.NumField(); i++ {
@@ -308,22 +308,13 @@ func extractNotEmptyFieldNames(s utils.KongRawState) []string {
        return fields
 }
 
-// ensureAllKongRawStateFieldsAreTested verifies that all fields in KongRawState are tested.
+// ensureAllKongStateFieldsAreTested verifies that all fields in KongState are tested.
 // It uses the testedFields slice to determine what fields were actually tested and compares
-// it to the list of all fields in KongRawState, excluding fields that KIC doesn't support.
-func ensureAllKongRawStateFieldsAreTested(t *testing.T, testedFields []string) {
-       kongRawStateFieldsKICDoesntSupport := []string{
-               // These are fields that KIC explicitly doesn't support.
-               "SNIs",
-               "ConsumerGroups",
-               "CustomEntities",
-               "Vaults",
-               "RBACRoles",
-               "RBACEndpointPermissions",
-       }
-       allKongRawStateFields := func() []string {
+// it to the list of all fields in KongState, excluding fields that KIC doesn't support.
+func ensureAllKongStateFieldsAreTested(t *testing.T, testedFields []string) {
+       allKongStateFields := func() []string {
                var fields []string
-               typ := reflect.ValueOf(utils.KongRawState{}).Type()
+               typ := reflect.ValueOf(kongstate.KongState{}).Type()
                for i := 0; i < typ.NumField(); i++ {
                        fields = append(fields, typ.Field(i).Name)
                }
@@ -331,11 +322,7 @@ func ensureAllKongRawStateFieldsAreTested(t *testing.T, testedFields []string) {
        }()
 
        // Meta test - ensure we have testcases covering all fields in KongRawState.
-       for _, field := range allKongRawStateFields {
-               if lo.Contains(kongRawStateFieldsKICDoesntSupport, field) {
-                       t.Logf("skipping field %s - explicitly unsupported", field)
-                       continue
-               }
+       for _, field := range allKongStateFields {
                assert.True(t, lo.Contains(testedFields, field), "field %s not tested", field)
        }
 }

This catches Vaults, ConsumerGroups and Licenses as not tested. 👍

@programmer04 programmer04 merged commit b8b543c into main Jan 15, 2024
37 checks passed
@programmer04 programmer04 deleted the consumergroup-raw-state branch January 15, 2024 14:56
@programmer04
Copy link
Member Author

Neat idea @czeslavo. I checked it and it detects the field Plugins too as false-positive I think. The option is to explicitly ignore it... Also, I discovered that KongRawState doesn't contain the field Licenses, it has to be added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KongRawStateToKongState doesn't support ConsumerGroups
2 participants