Add support for multiple classes #230
Conversation
Signed-off-by: Kevin Fox <[email protected]>
| // If MissingClassName is set and ClassName is set, any CR without a ClassName | ||
| // specified will also be handled by this controller. | ||
| // +optional | ||
| MissingClassName bool `json:"missingClassName,omitempty"` |
There was a problem hiding this comment.
Is there a potential for conflict with this configurable? If 1 controller has ClassName empty and other has this set to true, they will both try to process the same CR.
There was a problem hiding this comment.
Yes. Same thing happens with Ingress Controllers. Or two different controllers with ClassName empty.
Signed-off-by: Kevin Fox <[email protected]>
| // +optional | ||
| ClassName string `json:"className,omitempty"` | ||
|
|
||
| // If MissingClassName is set and ClassName is set, any CR without a ClassName |
There was a problem hiding this comment.
Is the presence of and naming of this configurable conventional? What's the use case?
There was a problem hiding this comment.
Not sure how much precedent there is. I can compare against what they did in ingress-nginx.
As for the use case, say you deploy one spire instance. no class name on it. users upload crs with no class name attached.
Then a second spire instance is desired. So, how is it safe to have both? A migration procedure is needed.
First, the existing spire server is updated to have
ClassName: foo
MissingClassName: true
So it will now have a name, and it will still handle all existing CR's without a ClassName.
The new spire server can then be installed with
ClassName: bar
MissingClassName: false
and new CRs of ClassName bar can be added to the cluster.
There was a problem hiding this comment.
ingress-nginx is quite verbose with their flag:
https://github.com/kubernetes/ingress-nginx/blob/a8798294083d4b0d874046648ec20d495849d000/charts/ingress-nginx/templates/_params.tpl#L55C18-L55C18
--watch-ingress-without-class=true
We could do WatchCRsWithoutClassName: true?
There was a problem hiding this comment.
Is the problem here backwards compatibility? If starting from scratch we would probably have ClassName set to a default value and then the separate "watch without class" bool set to false by default.
There was a problem hiding this comment.
I see. How about --watch-classless?
In terms of defaults, we need to preserve the default behavior of "handle everything", which should be the behavior if ClassName is empty. Using WatchClassless to allow a controller to simultaneously handle CR's without a ClassName as well as CRs matching the class name seems like a fine migration behavior.
There was a problem hiding this comment.
Yeah, the default is to watch all. (className empty)
There was a problem hiding this comment.
Will rename the missing flag.
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
fixes: #229