-
Notifications
You must be signed in to change notification settings - Fork 213
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
Adding support for third-party Controllers (e.g. OpenShift) #240
Conversation
Codecov Report
@@ Coverage Diff @@
## master #240 +/- ##
==========================================
+ Coverage 51.66% 55.64% +3.98%
==========================================
Files 12 12
Lines 753 629 -124
==========================================
- Hits 389 350 -39
+ Misses 309 247 -62
+ Partials 55 32 -23
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the way this is currently implemented breaks our ControllersToScan
config, since the user may not have specified owner.Kind
in that list.
I think a safer way to do things might be to see if ControllersToScan
contains a type that we don't have first-class support for. For those cases, we can retrieve all pods and find ones that have a relevant owner. Do you think that would catch the OpenShift types?
We should also create a new controller type for these cases in pkg/validator/controllers
, e.g. Miscellaneous
.
What was the original thinking with the ControllersToScan option? That might help me in thinking through how to fit this in. I was trying to think through any kind of scenario where I wouldn't just want to scan every kind of controller that I could. |
TBH, I don't fully remember 😅 - the guy who implemented it is no longer on the project. I wouldn't be opposed to removing it for v1. I still have some concerns over the current implementation though - e.g. I think we'll end up seeing multiple I do dislike our current pattern of needing a bunch of boilerplate code for each controller type, but I think it'd be good to stay consistent between vanilla k8s and open shift. Maybe we can sync offline and come up with a better path forward. I think checking pod parents like you're doing here might be an interesting route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks Bader!
Robert, do you want to take another look? I think I have the dynamic querying for parents working, I even have some dynamicness working for loading from a YAML file. I think this is at least the right track, but there's probably more to clean up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely coming along!
A few things:
- I think we need to get rid of
controllersToScan
in- pkg/config/config.go
- examples/config.yaml
- examples/config-full.yaml
- pkg/config/config_test.go
- can/should probably also get rid of pkg/config/supportedcontrollers.go
- there are some dropped tests that concern me - ideally they should stay the same
@@ -33,8 +33,8 @@ func TestGetTemplateData(t *testing.T) { | |||
|
|||
sum := CountSummary{ | |||
Successes: uint(0), | |||
Warnings: uint(9), | |||
Errors: uint(9), | |||
Warnings: uint(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we lost a lot of tests - why is that?
Robert Brennan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
I fixed up all of the tests, a few don't have as much data and I added TODOs there, that's because we're only retrieving things at the pod level now and all of the fake controllers haven't added pods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're just about there! A couple smaller comments.
I don't quite understand your comment about mocking pods in the tests - it looks to me like each of the fake controllers has a MockPod
} | ||
// If an owner exists then set the name to the controller. | ||
// This allows us to handle CRDs creating Controllers or DeploymentConfigs in OpenShift. | ||
for len(owners) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log an info message if there are multiple owners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the logging in there now.
pkg/validator/controllers/generic.go
Outdated
PodSpec kubeAPICoreV1.PodSpec | ||
ObjectMeta kubeAPIMetaV1.ObjectMeta | ||
Kind config.SupportedController | ||
KindString string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of tracking both Kind
and KindString
- it's not immediately apparent why they'd diverge (though I can see from the code that they do)
Is there anything wrong with always setting Kind
to the controller Kind, and getting rid of the SupportedController
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of the Kind on the GenericController, but left the SupportedController type because it still seemed useful for a few things, especially it seems like we use it on the validation webhook side.
controller.Name = originalDeploymentResource.Name | ||
controller.Namespace = originalDeploymentResource.Namespace | ||
controller.K8SResource = originalDeploymentResource | ||
func NewCronJobController(originalResource kubeAPIBatchV1beta1.CronJob) GenericController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these functions are just used in the tests - possible to nuke them or move them to the /test
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator webhook still uses these too
When pulling ReplicationController it will now look for an OwnerReference and roll up duplicates to that owner. This allows Polaris to work with OpenShift DeploymentConfigs but also any other CRD.
Also added a quick check for dividing by 0.