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

Update remaining resource ingress.class behaviors #815

Merged
merged 8 commits into from
Aug 31, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Aug 26, 2020

What this PR does / why we need it:

  • Updates the last permissive legacy class matching logic (for Knative and CA certificate secrets) to their modern, exact match logic.
  • Updates CA certificate tests to include class annotations.
  • Use a reference to the default class exported by annotations rather than the literal "kong" string in tests.
  • Explicitly sets global KongPlugin matching to ExactOrEmptyMatching.

Which issue this PR fixes
fixes #739
fixes #731

Special notes for your reviewer:
Changes for #731 are a composite of this and the implementation for #732, which was merged in #767. See comments in #731 for additional details.

We had originally discussed also rolling removal of the ListGlobalKongPlugins() store function into this, but on further review, we should leave that in. We list them only to report that they're there and instruct users to convert them to KongClusterPlugins:

globalPlugins, err := p.store.ListGlobalKongPlugins()
if err != nil {
return nil, fmt.Errorf("error listing global KongPlugins: %w", err)
}
if len(globalPlugins) > 0 {
p.Logger.Warning("global KongPlugins found. These are no longer applied and",
" must be replaced with KongClusterPlugins.",
" Please run \"kubectl get kongplugin -l global=true --all-namespaces\" to list existing plugins")
}

ListGlobalKongPlugins() is still a bit odd as it sorta still used the legacy matching behavior: it tracked the Ingress matching behavior, but that has since changed. Tracking it now would omit warnings for global KongPlugins with no class annotation (which were previously processed if you used the default "kong" class). As such, this PR preserves the function, but always uses ExactOrEmptyMatching to try and match the old behavior as best possible.

Travis Raines added 4 commits August 26, 2020 12:46
Always list global KongPlugins without a class annotation. Previously,
the store listed these resources using its matching criteria for Ingress
objects.

This change more or less preserves the older global KongPlugin behavior,
which would match KongPlugins with a matching class annotation and
KongPlugins with no class annotation (if using the default ingress
class). Behavior is now slightly different as it will match classless
KongPlugins even if using a custom ingress class.

As this function is only used to warn users about these KongPlugins
(they are no longer supported), legacy-ish behavior is desirable here.
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for arranging the commits the way you have. That allows for rebasing and merging in.

Please add test cases in each commit for the change you have done (doesn't apply to test commit, but others do need additional tests).

internal/ingress/store/fake_store.go Outdated Show resolved Hide resolved
internal/ingress/controller/parser/parser_test.go Outdated Show resolved Hide resolved
internal/ingress/controller/parser/parser_test.go Outdated Show resolved Hide resolved
internal/ingress/store/fake_store_test.go Outdated Show resolved Hide resolved
@rainest rainest requested a review from hbagdi August 28, 2020 18:11
mflendrich and others added 2 commits August 28, 2020 20:16
Co-authored-by: Michał Flendrich <michal@flendrich.pro>
@hbagdi hbagdi merged commit dd75a96 into next Aug 31, 2020
@hbagdi hbagdi deleted the feat/class-handling branch August 31, 2020 18:12
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.

3 participants