Fix/kong cluster plugin

[tharun208]

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1361

Special notes for your reviewer:

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

[rainest]

Doesn't look like this cleared the clientset generation issue I ran into previously.

https://github.com/Kong/kubernetes-ingress-controller/blob/d3dbeee0d7e31a6378884730cc34a817c58dee76/railgun/pkg/clientset/typed/configuration/v1/kongclusterplugin.go#L58-L77 still has namespace information.

Suppose this is as good enough venue to discuss as any other, since the root of the needed change (the scope annotation in railgun/apis/configuration/v1/kongclusterplugin_types.go) is present: @shaneutt do you know what's actually needed to make kubebuilder regenerate the clientset properly/what I did wrong in the steps described in #1361 ?

[shaneutt]

@shaneutt do you know what's actually needed to make kubebuilder regenerate the clientset properly

Kubebuilder isn't responsible for the clientset, we generate this separately from Kubebuilder using client-gen, in theory just running make generate.clientsets should be enough to straighten things out.

[tharun208]

Kubebuilder

@shaneutt I used this https://github.com/kubernetes/code-generator/tree/master/cmd/client-gen to re-generate the client-sets but no effect

[rainest]

Project names are meaningless to me, it's all kubebuilder :D noted though, I'll try to remember which is which in the future.

After some more searching, that does indicate why my original attempt didn't work: kubebuilder and client-gen both have their own (different) metadata tags, and there's an additional tag needed for client-gen to do its half of the non-namespaced change:

git diff apis/configuration/v1/kongclusterplugin_types.go 
diff --git a/railgun/apis/configuration/v1/kongclusterplugin_types.go b/railgun/apis/configuration/v1/kongclusterplugin_types.go
index dea8b10d..01c3723c 100644
--- a/railgun/apis/configuration/v1/kongclusterplugin_types.go
+++ b/railgun/apis/configuration/v1/kongclusterplugin_types.go
@@ -22,6 +22,7 @@ import (
 )
 
 //+genclient
+//+genclient:nonNamespaced
 //+k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 //+kubebuilder:object:root=true
 //+kubebuilder:subresource:status

@tharun208 if you add that tag and then run make generate.clientsets again it will properly add the updated clientset code.

[tharun208]

https://stupefied-goodall-e282f7.netlify.app/contributors/devel/generating-clientset/ This document says we need to tag a separate tag for cluster scoped crd generation.

[tharun208]

and also the makefile is not having a command to install the client-gen tool. will try to add it in a separate PR.

[codecov]

Codecov Report

Merging #1418 (c62ce6f) into next (60c106a) will increase coverage by 2.38%.
The diff coverage is 35.15%.

@@            Coverage Diff             @@
##             next    #1418      +/-   ##
==========================================
+ Coverage   52.90%   55.28%   +2.38%     
==========================================
  Files          35       42       +7     
  Lines        3272     3661     +389     
==========================================
+ Hits         1731     2024     +293     
- Misses       1408     1489      +81     
- Partials      133      148      +15     

Impacted Files	Coverage Δ
cli/ingress-controller/main.go	0.24% <0.00%> (+0.01%)	⬆️
cli/ingress-controller/util.go	40.74% <0.00%> (+2.80%)	⬆️
internal/ingress/status/status.go	40.56% <ø> (ø)
pkg/kongstate/consumer.go	100.00% <ø> (ø)
pkg/kongstate/kongstate.go	37.64% <ø> (+12.94%)	⬆️
pkg/kongstate/service.go	80.00% <ø> (ø)
pkg/kongstate/upstream.go	55.00% <ø> (ø)
pkg/sendconfig/common_workflows.go	0.00% <0.00%> (ø)
pkg/util/logging.go	0.00% <0.00%> (ø)
pkg/util/secretgetter.go	0.00% <0.00%> (ø)
... and 32 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 93833d7...c62ce6f. Read the comment docs.

[rainest]

There are two outstanding issues. One is that I forgot the ingress class annotation on my original test object. That's easy to fix:

diff --git a/railgun/test/integration/plugin_test.go b/railgun/test/integration/plugin_test.go
index b439d77b..482774b4 100644
--- a/railgun/test/integration/plugin_test.go
+++ b/railgun/test/integration/plugin_test.go
@@ -95,6 +95,9 @@ func TestMinimalPlugin(t *testing.T) {
        kongclusterplugin := &kongv1.KongClusterPlugin{
                ObjectMeta: metav1.ObjectMeta{
                        Name: "legal",
+                       Annotations: map[string]string{
+                               annotations.IngressClassKey: ingressClass,
+                       },
                },
                PluginName: "request-termination",
                Config: apiextensionsv1.JSON{

The second part is weirder. For some reason the store isn't being constructed correctly:

> github.com/kong/kubernetes-ingress-controller/pkg/store.Store.GetKongClusterPlugin() /tmp/ingress-controller/pkg/store/store.go:483 (PC: 0x12d8746)
   478:		return p.(*kongv1.KongPlugin), nil
   479:	}
   480:	
   481:	// GetKongClusterPlugin returns the 'name' KongClusterPlugin resource.
   482:	func (s Store) GetKongClusterPlugin(name string) (*kongv1.KongClusterPlugin, error) {
=> 483:		p, exists, err := s.stores.ClusterPlugin.GetByKey(name)
   484:		if err != nil {
   485:			return nil, err
   486:		}
   487:		if !exists {
   488:			return nil, ErrNotFound{fmt.Sprintf("KongClusterPlugin %v not found", name)}

(dlv) p s.stores.ClusterPlugin.cacheStorage.items
map[string]interface {} [
	"/legal": *github.com/kong/kubernetes-ingress-controller/railgun/apis/configuration/v1.KongClusterPlugin {
		TypeMeta: (*"k8s.io/apimachinery/pkg/apis/meta/v1.TypeMeta")(0xc0004b7800),
		ObjectMeta: (*"k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta")(0xc0004b7820),
		ConsumerRef: "",
		Disabled: false,
		Config: (*"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON")(0xc0004b7930),
		ConfigFrom: (*"github.com/kong/kubernetes-ingress-controller/railgun/apis/configuration/v1.NamespacedConfigSource")(0xc0004b7948),
		PluginName: "request-termination",
		RunOn: "",
		Protocols: []string len: 0, cap: 0, nil,}, 
]

(dlv) call s.stores.ClusterPlugin.GetByKey("/legal")
> github.com/kong/kubernetes-ingress-controller/pkg/store.Store.GetKongClusterPlugin() /tmp/ingress-controller/pkg/store/store.go:483 (PC: 0x12d8746)
Values returned:
	item: interface {}(*github.com/kong/kubernetes-ingress-controller/railgun/apis/configuration/v1.KongClusterPlugin) *{
		TypeMeta: k8s.io/apimachinery/pkg/apis/meta/v1.TypeMeta {
			Kind: "KongClusterPlugin",
			APIVersion: "configuration.konghq.com/v1",},
		ObjectMeta: k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta {
			Name: "legal",

Looks like wherever we insert this into the store is still treating this like a namespaced object (a KongPlugin would have a key like namespace/name, but since there's no actual namespace, it just gets the empty string before the slash. Need to track down where that gets inserted so we can prune the slash.

[rainest]

Alright, looks like the second part is simple if more annoying to track down:

 // NewCacheStores is a convenience function for CacheStores to initialize all attributes with new cache stores
 func NewCacheStores() (c CacheStores) {
-       c.ClusterPlugin = cache.NewStore(keyFunc)
+       c.ClusterPlugin = cache.NewStore(clusterResourceKeyFunc)

This was new for 2.x only. Not entirely sure how 1.x store worked. Apparently it just uses the stuff in fake_store.go (which had the correct key function)? Not worth investigating too much.

@tharun208 with cluster.diff.txt on top of the previous commits here tests will pass. You'll want to pull the latest upstream copy of next and rebase to that also.

Edit: well, maybe not. Something weird happening where they said they passed but then claimed the condition never satisfied after? Maybe just a flake, since the existing KongPlugin test had issues in my run as well.

Edit 2: subsequent run looks fine, so apparently indeed a flake.