Added annotation validation methods for Custom Resources #220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
=========================================
+ Coverage 21.85% 22.1% +0.24%
=========================================
Files 24 24
Lines 2375 2552 +177
=========================================
+ Hits 519 564 +45
- Misses 1812 1938 +126
- Partials 44 50 +6
Continue to review full report at Codecov.
|
| // 4 - ingress with specific class | different annotation on ingress | ||
| if ingress == "" && IngressClass == DefaultClass { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Let's extract the functionality out into a function rather than repeating it multiple times?
docs/custom-resources.md
Outdated
| @@ -15,6 +15,30 @@ Following CRDs enables users to declaratively configure all aspects of Kong: | |||
| - [**KongCredential**](#kongcredential): These resources map to | |||
| credentials (key-auth, basic-auth, etc) that belong to consumers. | |||
|
|
|||
| **IMPORTANT NOTE**: Kong Custom Resources are using the `kubernetes.io/ingress.class` annotation which defaults to `kong`. In | |||
| case you have more than one Kong Ingress deployed on your cluster, make sure you label all your ressources with the appropriate | |||
| annotation. For example: | |||
There was a problem hiding this comment.
- The ingress class annotation defaults to
nginxand notkong, so let's update that. - Please respect the 80 column length as is the case in the rest of this file.
- Let's move this note to the bottom of the file than here.
KongIngresscustom resource doesn't support theingress.classannotation as per this PR, so let's mention the resources that support the annotation explicitly.-
In case you have more than one
KongIngress deployed - Typo > ressources
| } | ||
| } | ||
|
|
||
| func TestIsValidPlugin(t *testing.T) { |
There was a problem hiding this comment.
Are these tests necessary to be repeated for each type since the method now takes in ObjectMeta?
If not, let remove those as additional tests are redundant.
| @@ -34,6 +37,32 @@ func buildIngress() *extensions.Ingress { | |||
| } | |||
| } | |||
|
|
|||
| func buildPlugin() *pluginv1.KongPlugin { | |||
There was a problem hiding this comment.
Same comment here, do these tests need to be repeated? The don't seem to be testing anything specific to a plugin, consumer or ingress but are operating on the ObjectMeta struct.
| if !class.IsValid(&plugin.ObjectMeta) { | ||
| glog.Infof("ignoring delete for plugin %v based on annotation %v", plugin.Name, class.IngressKey) | ||
| return | ||
| } |
There was a problem hiding this comment.
There seems to be a lot of duplication of code here.
Can we create a method for checking the validity of the resource and then reuse that across all 3 cache handlers?
There was a problem hiding this comment.
I've created 3 new methods in class package:
CanAddResourceCanDeleteResourceCanUpdateResource
I've also renamed the event handler annotatedCrdEventHandler which is now common to all CRD that supports annotations.
There was a problem hiding this comment.
The new code seems very odd in the way it is organized.
Organizing the code as follows:
AddFunc: func(obj interface{}) {
var p *pluginv1.KongPlugin
if p, ok := obj.(*pluginv1.KongPlugin); !ok {
// err
}
objectMeta = &p.ObjectMeta
if ok := class.IsValid(objectMeta); !ok
return
}
updateCh.In() <- Event{
Type: ConfigurationEvent,
Obj: obj,
}
},This helps in a few ways:
- Avoid duplicate logic per type
- IsValid is the only method that is called, making it easier to test and introduces changes in future.
There was a problem hiding this comment.
@hbagdi do you prefer 1 event handler per resource because the code you've suggested means this... I tried to keep only one handler anyway I can fall back to 3 handlers and have your last suggestion.
There was a problem hiding this comment.
I think 3 handlers are okay, since we need to type cast. But we should keep the hanlders small and have minimal logic as possible.
|
@nmacherey Manually merged in with some bug fixes and corrections. |
What this PR does / why we need it:
Add annotation support for CDR, in ListPlugins, ListConsumers, ListCredentials and in CRD event handling.
Which issue this PR fixes
Special notes for your reviewer:
My knowledge of GO is very poor, I've added tests and deployed it on my local kubernetes cluster and things seams to work fine. Anyway the code may need some customization.