Feat/ingress class driven configuration #1586

larribas · 2021-07-24T19:04:54Z

This is a proposal to allow cluster operators to control certain settings of an ingress controller through a new CR named IngressClassParams, which must be associated to the controller through an IngressClass.

The motivation behind this ticket and the choice to do it through the IngressClass is explained in detail in issue #1131.

The IngressClassParams CR has been introduced at version v1alpha1, and covering only one setting (serviceUpstream). The idea is that the same CR can be extended to support all existing controller-level configuration options.

Which issue this PR fixes:

Closes #774
Closes #1131

Special notes for your reviewer:

This is my first time contributing to the codebase. I have a few follow-up questions, mostly about conventions.

Should we also listen to IngressClass resources from versions other than "v1"? (1.18 introduced it as v1beta1)
Should we control the scenario where serviceUpstream is set to true in the IngressClassParameters but the ingress.kubernetes.io/service-upstream annotation is explicitly set to false?
Is there anything else we should do in terms of RBAC permissions for the new resources we're controlling?
Should we prefix IngressClassParams with Kong? Why is there a convention to prefix resources with Kong when they're already in kong's api group? Does the convention apply in this case?
At the moment, the codebase places responsibility on the controllers to filter out the Ingress resources based on the class name. Intuitively, I think the responsibility should be equivalent for IngressClass resources (and even IngressClassParameters). However, I don't feel very comfortable changing go code as a big multi-line string, so I opted for doing it at the store level. Happy to discuss about it and change it.

PR Readiness Checklist:

The following tasks are left unfinished (to be done after the first review):

there is an integration test covering the new functionality
the documentation has been updated
the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

…tion tests

…setting the upstream for an endpoint

CLAassistant · 2021-07-24T19:05:03Z

All committers have signed the CLA.

mflendrich · 2021-07-26T17:19:07Z

"Do not merge" yet because this will not go to KIC 2.0, and next is our 2.0 mainline at the moment. We're aiming for KIC 2.1 with this feature, so we'll merge this PR later (and after review)

codecov · 2021-07-27T11:38:31Z

Codecov Report

Merging #1586 (f1f963a) into next (6c77c91) will decrease coverage by 12.93%.
The diff coverage is 17.32%.

@@             Coverage Diff             @@
##             next    #1586       +/-   ##
===========================================
- Coverage   50.47%   37.54%   -12.94%     
===========================================
  Files          92       96        +4     
  Lines        6437     6438        +1     
===========================================
- Hits         3249     2417      -832     
- Misses       2880     3854      +974     
+ Partials      308      167      -141

Flag	Coverage Δ
integration-test	`?`
unit-test	`37.54% <17.32%> (-1.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/annotations/annotations.go	`96.87% <ø> (ø)`
pkg/sendconfig/common_workflows.go	`0.00% <ø> (ø)`
...n/apis/configuration/v1alpha1/groupversion_info.go	`0.00% <0.00%> (ø)`
...trollers/configuration/zz_generated_controllers.go	`0.00% <0.00%> (-30.14%)`	⬇️
...gun/hack/generators/controllers/networking/main.go	`0.00% <ø> (ø)`
railgun/manager/controllerdef.go	`0.00% <0.00%> (-88.97%)`	⬇️
railgun/pkg/clientset/clientset.go	`0.00% <0.00%> (-28.13%)`	⬇️
railgun/pkg/clientset/fake/register.go	`100.00% <ø> (ø)`
railgun/pkg/clientset/scheme/register.go	`100.00% <ø> (ø)`
...ped/configuration/v1alpha1/configuration_client.go	`0.00% <0.00%> (ø)`
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c77c91...f1f963a. Read the comment docs.

mflendrich

The approach seems good 👍. First round of comments follows.

mflendrich · 2021-07-26T14:54:21Z

pkg/parser/parser.go

@@ -384,12 +399,11 @@ func getEndpoints(
 			Port:    fmt.Sprintf("%v", targetPort),
 		})
 	}
-	if annotations.HasServiceUpstreamAnnotation(s.Annotations) {
+	if annotations.HasServiceUpstreamAnnotation(s.Annotations) || ingressClassParams.ServiceUpstream {


I recommend that we isolate the logic of "evaluating whether a service should be treated as service-upstream or not" in a separate, unit-tested function. That would mean putting the condition of this if expression in a function.

Note that this comment interacts with the comment about the signature of getEndpoints above.

mflendrich · 2021-07-27T11:48:30Z

railgun/manager/controllerdef.go

@@ -102,6 +102,15 @@ func setupControllers(logger logr.Logger, mgr manager.Manager, proxy proxy.Proxy
 				Proxy:  proxy,
 			},
 		},
+		{
+			IsEnabled: &alwaysEnabled,


This won't work for kubernetes <1.18 because they don't support the IngressClass kind.
Please target not breaking compatibility with k8s 1.16+.

The simplest way to go about that would be by exposing a flag to control whether KIC shall try listening to IngressClasses.
Then (outside of the scope of this PR) we could implement a mechanism (an AutoHandler most likely - there is some work in progress on it, see #1585) - that would tell whether the apiserver supports IngressClasses and react dynamically. But, again, this would be outside of the scope of this PR.

mflendrich · 2021-07-27T11:49:50Z

railgun/manager/controllerdef.go

@@ -153,6 +162,15 @@ func setupControllers(logger logr.Logger, mgr manager.Manager, proxy proxy.Proxy
 				IngressClassName: c.IngressClassName,
 			},
 		},
+		{
+			IsEnabled: &c.IngressClassParamsEnabled,


non-blocking, open question sparked by my comment above: I'm wondering whether two different flags for IngressClass and IngressClassParams make sense, or should it be just one.

mflendrich · 2021-07-27T11:52:59Z

pkg/annotations/annotations.go

@@ -35,6 +35,7 @@ const (
 const (
 	IngressClassKey        = "kubernetes.io/ingress.class"
 	KnativeIngressClassKey = "networking.knative.dev/ingress.class"
+	KongIngressClassKey    = "ingress.konghq.com/ingress.class"


Following an out-of-band discussion: the controller name in the IngressClass resource is unused by KIC. Please use an "unused" const in tests instead, because a value like this may bring confusion to users, and also it's very similar to the name of the ingress class annotation.

mflendrich · 2021-07-27T11:55:23Z

pkg/parser/parser.go

@@ -330,7 +331,8 @@ func getServiceEndpoints(log logrus.FieldLogger, s store.Storer, svc corev1.Serv
 	// check all protocols for associated endpoints
 	endpoints := []util.Endpoint{}
 	for protocol := range protocols {
-		newEndpoints := getEndpoints(log, &svc, servicePort, protocol, s.GetEndpointsForService)
+		ingressClassParams := getIngressClassParamsOrDefault(s)
+		newEndpoints := getEndpoints(log, &svc, servicePort, protocol, s.GetEndpointsForService, ingressClassParams)


I think that a serviceUpstream bool parameter would produce a cleaner solution than plumbing the ingress class params through. Also it would allow for better separation of concerns between "ingress class parameterization" and "extracting info from a service".

Suggestion: replace IngressClassParams with a bool saying whether getEndpoints shall yield a single service upstream endpoint, or the real service endpoints.

mflendrich · 2021-07-27T12:27:11Z

pkg/store/fake_store_test.go

+			ingressClassParams: []*kongv1alpha1.IngressClassParams{},
+			ingressClasses:     []*networkingv1.IngressClass{},


Suggested change

ingressClassParams: []*kongv1alpha1.IngressClassParams{},

ingressClasses: []*networkingv1.IngressClass{},

two comments here:

nil slices are better than empty literal slices, because the latter occupy memory unnecessarily

there's no need to initialize empty slices explicitly

https://medium.com/@habibridho/golang-nil-vs-empty-slice-87fd51c0a4d

Please follow this style throughout the PR.

mflendrich · 2021-07-27T12:28:03Z

pkg/store/fake_store_test.go

+		shouldExist        bool
+	}{
+		{
+			name:               "When there are no ingress classes defined",


"When there is" is superfluous - keep the descriptions simple: e.g. no ingress class defined

mflendrich · 2021-07-27T12:30:19Z

pkg/store/fake_store_test.go

+						Name: "meant-for-a-different-ingress-controller",
+					},
+					Spec: networkingv1.IngressClassSpec{
+						Controller: "some/ingress-controller",


please say "unused" here - like explained in one of the comments above

mflendrich · 2021-07-27T12:32:01Z

pkg/store/store_test.go

+metadata:
+  name: foo
+spec:
+  controller: "some/controller"


"unused" - as said in other comments

mflendrich · 2021-07-27T12:34:29Z

pkg/store/store.go

+// by the IngressClass resource associated with this controller, if it exists.
+// Otherwise, it returns an error
+func (s Store) GetIngressClassParams() (*kongv1alpha1.IngressClassParams, error) {
+	class, exists, err := s.stores.IngressClass.GetByKey(s.ingressClass)


There's a NPE in the tests on this line - please ensure that all the CI tests for this PR pass.

shaneutt · 2021-07-29T13:00:44Z

@larribas so that you're aware we made some structural changes in next/ which removed older v1 code (see #1591) I was going around proactively rebasing PRs but yours being from a fork I wont be able to do it directly.

I've created the following branch: larribas-rebase which rebases your fork onto current next and you should be able to use that directly, let me know if you run into any trouble or need any assistance getting that working.

mflendrich · 2021-08-06T18:38:12Z

@larribas

GH has automatically closed this PR because we've deleted the next branch.

Could you please change the target branch of this PR from next to main and reopen it? 🙏 Only the author can do it.
The closure of this PR is NOT because we don't want to merge this PR.

edit: apparently, in this case GH doesn't allow for changing the target branch. If you are unable to set the target branch to main, could you please recreate this PR, targetting main this time? Thank you!

larribas added 6 commits July 24, 2021 17:54

chore(make): allow selecting specific test cases when running integra…

0d8c369

…tion tests

chore: fix typo on common_workflows

bf7b030

feat: reconcile and store ingressclass resources

ad54911

feat: reconcile and store custom ingressclassparams resources

24aecd3

feat: use serviceUpstream attribute from IngressClassParameters when …

3c0081e

…setting the upstream for an endpoint

test: strengthen parser getEndpoints() test suite

f1f963a

larribas requested a review from a team as a code owner July 24, 2021 19:04

larribas changed the base branch from main to next July 24, 2021 19:05

larribas marked this pull request as draft July 24, 2021 19:06

mflendrich requested review from mflendrich and removed request for a team July 24, 2021 19:30

mflendrich added the do not merge let the author merge this, don't merge for them. label Jul 26, 2021

mflendrich reviewed Jul 27, 2021

View reviewed changes

mflendrich deleted the branch Kong:next August 6, 2021 18:30

mflendrich closed this Aug 6, 2021

RobSerafini mentioned this pull request Mar 16, 2022

Enable service-upstream for an entire ingress class #774

Closed

This was referenced Jun 3, 2022

feat: add IngressClassParameters CRD and ServiceUpstream field #2535

Merged

docs: add IngressClassParams KEP #2536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ingress class driven configuration #1586

Feat/ingress class driven configuration #1586

larribas commented Jul 24, 2021

CLAassistant commented Jul 24, 2021 •

edited

Loading

mflendrich commented Jul 26, 2021

codecov bot commented Jul 27, 2021

mflendrich left a comment

mflendrich Jul 26, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

mflendrich Jul 27, 2021

shaneutt commented Jul 29, 2021

mflendrich commented Aug 6, 2021 •

edited

Loading

		ingressClassParams: []*kongv1alpha1.IngressClassParams{},
		ingressClasses: []*networkingv1.IngressClass{},

Feat/ingress class driven configuration #1586

Feat/ingress class driven configuration #1586

Conversation

larribas commented Jul 24, 2021

CLAassistant commented Jul 24, 2021 • edited Loading

mflendrich commented Jul 26, 2021

codecov bot commented Jul 27, 2021

Codecov Report

mflendrich left a 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

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

shaneutt commented Jul 29, 2021

mflendrich commented Aug 6, 2021 • edited Loading

CLAassistant commented Jul 24, 2021 •

edited

Loading

mflendrich commented Aug 6, 2021 •

edited

Loading