Adding support for third-party Controllers (e.g. OpenShift) #240

baderbuddy · 2020-01-31T19:13:38Z

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.

codecov · 2020-01-31T19:21:58Z

Codecov Report

Merging #240 into master will increase coverage by 3.98%.
The diff coverage is 82.52%.

@@            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

Impacted Files	Coverage Δ
pkg/config/schema.go	`13.25% <0%> (ø)`	⬆️
pkg/validator/summary.go	`48.03% <0%> (-0.97%)`	⬇️
pkg/validator/container.go	`72.72% <100%> (ø)`	⬆️
pkg/validator/fullaudit.go	`47.61% <100%> (-5.58%)`	⬇️
pkg/validator/schema.go	`79.56% <100%> (+1.67%)`	⬆️
pkg/validator/pod.go	`63.63% <100%> (ø)`	⬆️
pkg/validator/controller.go	`81.48% <80%> (-4.73%)`	⬇️
pkg/kube/resources.go	`70.21% <85.36%> (+19.82%)`	⬆️

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 37660e0...57f3a8b. Read the comment docs.

rbren

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.

pkg/validator/controller.go

pkg/validator/summary.go

baderbuddy · 2020-02-03T13:05:46Z

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.

rbren · 2020-02-03T13:57:09Z

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 CronJob objects - one for the original, and one for each Job it's created. And if the ownership is XController -> Pod, with no intermediate controller type, we'll miss it.

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.

jordandoig

👍 Thanks Bader!

baderbuddy · 2020-03-17T21:16:16Z

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.

rbren

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

pkg/config/supportedcontrollers.go

pkg/kube/resources_test.go

pkg/validator/controllers/interface.go

pkg/kube/resources.go

rbren · 2020-03-19T18:32:00Z

pkg/validator/fullaudit_test.go

@@ -33,8 +33,8 @@ func TestGetTemplateData(t *testing.T) {

 	sum := CountSummary{
 		Successes: uint(0),
-		Warnings:  uint(9),
-		Errors:    uint(9),
+		Warnings:  uint(1),


Looks like we lost a lot of tests - why is that?

…b/openshift

CLAassistant · 2020-03-23T13:27:54Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ baderbuddy
❌ Robert Brennan

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.}

baderbuddy · 2020-03-25T18:24:16Z

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.

rbren

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

pkg/validator/controllers/generic.go

rbren · 2020-03-25T20:07:11Z

pkg/validator/controllers/generic.go

+	}
+	// 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 {


Should we log an info message if there are multiple owners?

Got the logging in there now.

rbren · 2020-03-25T20:13:24Z

pkg/validator/controllers/generic.go

+	PodSpec     kubeAPICoreV1.PodSpec
+	ObjectMeta  kubeAPIMetaV1.ObjectMeta
+	Kind        config.SupportedController
+	KindString  string


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?

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.

rbren · 2020-03-25T20:24:58Z

pkg/validator/controllers/cronjob.go

-	controller.Name = originalDeploymentResource.Name
-	controller.Namespace = originalDeploymentResource.Namespace
-	controller.K8SResource = originalDeploymentResource
+func NewCronJobController(originalResource kubeAPIBatchV1beta1.CronJob) GenericController {


Seems like these functions are just used in the tests - possible to nuke them or move them to the /test package?

The validator webhook still uses these too

pkg/validator/output.go

Bader Boland added 4 commits January 31, 2020 12:44

Add note about config change

330d7f0

Prevent divide by 0

51d639e

Directly return 0 for divide by 0

aaa54cc

Added check for parent and rollup to parent.

31de106

baderbuddy requested review from jordandoig, makoscafee and rbren as code owners January 31, 2020 19:13

rbren reviewed Jan 31, 2020

View reviewed changes

pkg/validator/controller.go Outdated Show resolved Hide resolved

pkg/validator/controller.go Outdated Show resolved Hide resolved

rbren reviewed Jan 31, 2020

View reviewed changes

pkg/validator/summary.go Show resolved Hide resolved

Merge branch 'master' into bb/openshift

b5ac0bc

jordandoig approved these changes Feb 19, 2020

View reviewed changes

baderbuddy and others added 11 commits February 19, 2020 15:34

Merge branch 'master' into bb/openshift

619e17d

Merge branch 'master' into bb/openshift

fe55216

Dynamically retrieve parents

bb34be7

Remove unnecessary queries

0d3fe61

Fix tests

7fdebfc

Cut out logic specific to controller types

3c68527

Cut out duplicitive code.

8c769e4

Add additional logging

fb0d7c5

Add support for CronJobs in support versions list

c43ace2

Adding support for v2alpha1 for cron

61ecb69

Merge branch 'master' into bb/openshift

ee2ccb0

baderbuddy requested a review from rbren March 17, 2020 21:15

baderbuddy and others added 2 commits March 18, 2020 16:45

Merge branch 'master' into bb/openshift

98533bc

Merge branch 'master' into bb/openshift

d18bf85

rbren reviewed Mar 19, 2020

View reviewed changes

baderbuddy added 2 commits March 23, 2020 09:27

Feedback from PR

68fe230

Merge branch 'bb/openshift' of github.com:FairwindsOps/polaris into b…

49dbd1d

…b/openshift

baderbuddy and others added 3 commits March 23, 2020 09:27

Merge branch 'master' into bb/openshift

aa0658b

deduplicate results right away

5769acf

Fix tests

a5828a2

baderbuddy requested a review from rbren March 25, 2020 19:30

rbren reviewed Mar 25, 2020

View reviewed changes

baderbuddy and others added 2 commits March 25, 2020 16:50

Cleanup SupportedControllers

3c46f40

Merge branch 'master' into bb/openshift

dcf02b9

baderbuddy requested a review from rbren March 25, 2020 20:58

rbren approved these changes Mar 25, 2020

View reviewed changes

rbren changed the title ~~Adding support for OpenShift~~ Adding support for third-party Controllers (e.g. OpenShift) Mar 25, 2020

Merge branch 'master' into bb/openshift

57f3a8b

baderbuddy merged commit c03ecbd into master Mar 26, 2020

baderbuddy deleted the bb/openshift branch March 26, 2020 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for third-party Controllers (e.g. OpenShift) #240

Adding support for third-party Controllers (e.g. OpenShift) #240

baderbuddy commented Jan 31, 2020

codecov bot commented Jan 31, 2020 •

edited

Loading

rbren left a comment

baderbuddy commented Feb 3, 2020

rbren commented Feb 3, 2020

jordandoig left a comment

baderbuddy commented Mar 17, 2020

rbren left a comment

rbren Mar 19, 2020

CLAassistant commented Mar 23, 2020

baderbuddy commented Mar 25, 2020

rbren left a comment

rbren Mar 25, 2020

baderbuddy Mar 25, 2020

rbren Mar 25, 2020

baderbuddy Mar 25, 2020

rbren Mar 25, 2020

baderbuddy Mar 25, 2020

Adding support for third-party Controllers (e.g. OpenShift) #240

Adding support for third-party Controllers (e.g. OpenShift) #240

Conversation

baderbuddy commented Jan 31, 2020

codecov bot commented Jan 31, 2020 • edited Loading

Codecov Report

rbren left a comment

Choose a reason for hiding this comment

baderbuddy commented Feb 3, 2020

rbren commented Feb 3, 2020

jordandoig left a comment

Choose a reason for hiding this comment

baderbuddy commented Mar 17, 2020

rbren left a comment

Choose a reason for hiding this comment

rbren Mar 19, 2020

Choose a reason for hiding this comment

CLAassistant commented Mar 23, 2020

baderbuddy commented Mar 25, 2020

rbren left a comment

Choose a reason for hiding this comment

rbren Mar 25, 2020

Choose a reason for hiding this comment

baderbuddy Mar 25, 2020

Choose a reason for hiding this comment

rbren Mar 25, 2020

Choose a reason for hiding this comment

baderbuddy Mar 25, 2020

Choose a reason for hiding this comment

rbren Mar 25, 2020

Choose a reason for hiding this comment

baderbuddy Mar 25, 2020

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2020 •

edited

Loading