Skip to content
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

Additional Pod Controller Scans #166

Merged
merged 32 commits into from Jul 31, 2019

Conversation

@endzyme
Copy link
Member

commented Jul 16, 2019

  • Refactored the way controllers work to be an interface
  • Added configurable controllers to include in scans
  • Added daemonsets, jobs and cronjobs in scans

Explicitly did not add ReplicationControllers and ReplicaSets since they are either deprecated or controlled by deployments (generally).

  • Added tests to supportedController scanner
  • Added documentation on the exit codes
  • Added code documentation for longer functions
endzyme added 2 commits Jul 6, 2019
Additions
- Refactored the way controllers work to be an interface
- Added configurable controllers to include in scans
- Added daemonsets, jobs and cronjobs in scans

Explicitly did not add ReplicationControllers and ReplicaSets since
they are either deprecated or controlled by deployments (generally).

- Added tests to supportedController scanner

@endzyme endzyme requested review from rbren and kimschles Jul 16, 2019

README.md Outdated Show resolved Hide resolved
if yes := v.Config.CheckIfKindIsConfiguredForValidation(req.AdmissionRequest.Kind.Kind); !yes {
logrus.Warnf("Skipping, kind (%s) isn't something we are configured to scan", req.AdmissionRequest.Kind.Kind)
// FIXME: Should we be returning an OK response as skipped?
return admission.ErrorResponse(http.StatusBadRequest, err)

This comment has been minimized.

Copy link
@rbren

rbren Jul 16, 2019

Contributor

Yes, I think for the webhook this should be an OK response for unsupported types. Otherwise (IIUC) if a new controller type is introduced, we'll reject it until Polaris gets updated to support it.

This comment has been minimized.

Copy link
@rbren

rbren Jul 24, 2019

Contributor

Still seeing the FIXME here, and looks like it's still StatusBadRequest.

This comment has been minimized.

Copy link
@endzyme

endzyme Jul 24, 2019

Author Member

I think maybe it was a bad browser cache on your side? I am not seeing it

@rbren

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

You'll need to update the RBAC definitions in the chart to regenerate everything in the ./deploy folder as well.

@endzyme

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

You'll need to update the RBAC definitions in the chart to regenerate everything in the ./deploy folder as well.

For sure, I saw that on your PR for the limit ranges. This is a WIP and I wanted to validate this isn't too over the top or out of line with the requested functionality.

@rbren rbren added this to the Q32019 milestone Jul 16, 2019

@rbren

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

FYI, in #107 someone mentions that ReplicationControllers are still widely in use - maybe it makes sense to add them?

@codecov

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

Merging #166 into master will decrease coverage by 2.17%.
The diff coverage is 79.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
- Coverage   85.01%   82.84%   -2.18%     
==========================================
  Files           9       10       +1     
  Lines         534      647     +113     
==========================================
+ Hits          454      536      +82     
- Misses         58       87      +29     
- Partials       22       24       +2
Impacted Files Coverage Δ
pkg/config/config.go 72% <ø> (ø) ⬆️
pkg/validator/fullaudit.go 100% <100%> (ø) ⬆️
pkg/config/supportedcontrollers.go 57.4% <57.4%> (ø)
pkg/kube/resources.go 67.1% <81.53%> (+8.17%) ⬆️
pkg/validator/controller.go 88.88% <84.61%> (-11.12%) ⬇️
pkg/validator/types.go 96.87% <95%> (-3.13%) ⬇️

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 4aa360a...c168c01. Read the comment docs.

endzyme added 6 commits Jul 23, 2019
Adjustments
- Added `ReplicationController` type controllers to the supported list
- Adjusted logic for failed YAML parsing to bubble up errors
- Added better logic for calculating summaries on cluster wide results
- Relocated responsibilities for counting types into validators vs spreading it around more packages
- Fixed bug where cronjob parsing was using wrong KIND
- Added fixtures for mocking new controller types
- Added example yamls to test scanning files
- Added functions to NamespacedResult(s) to reduce code complexity deep set iterations
- Refactored how results get added to namespacedresults so adding more later is easier
- Minor signature changes for interface implementing structs for controllers
@endzyme

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

@bobby-brennan: I've incorporated the following (per request)

  • Added replicationController type
  • Added more testing (better coverage)
  • Fixed webhook integration and support for skipping non-scanned controllers and other kinds

@endzyme endzyme marked this pull request as ready for review Jul 23, 2019

@endzyme endzyme requested a review from robscott as a code owner Jul 23, 2019

pkg/config/supportedcontrollers.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rbren

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Looking great Nick! A few comments but I think this is just about ready.

I really like using the interface for different controller types, makes a lot more sense than what I had.

As a side note, we should start testing the webhook for correctness and feature parity. Opened #178 to track.

pkg/kube/resources.go Outdated Show resolved Hide resolved
if yes := v.Config.CheckIfKindIsConfiguredForValidation(req.AdmissionRequest.Kind.Kind); !yes {
logrus.Warnf("Skipping, kind (%s) isn't something we are configured to scan", req.AdmissionRequest.Kind.Kind)
// FIXME: Should we be returning an OK response as skipped?
return admission.ErrorResponse(http.StatusBadRequest, err)

This comment has been minimized.

Copy link
@rbren

rbren Jul 24, 2019

Contributor

Still seeing the FIXME here, and looks like it's still StatusBadRequest.

endzyme added 3 commits Jul 24, 2019
Adjustments
- Added requested var name changes
- Fixed Fairwinds typo
- Adjusted the logic for finding key in map
endzyme added 5 commits Jul 24, 2019
@rbren
rbren approved these changes Jul 30, 2019
Copy link
Contributor

left a comment

Looking great, thanks for doing this Nick!

README.md Outdated Show resolved Hide resolved
endzyme added 13 commits Jul 31, 2019
pkg/dashboard/helpers.go Outdated Show resolved Hide resolved

@endzyme endzyme merged commit 75f7035 into master Jul 31, 2019

4 of 6 checks passed

codecov/patch 79.03% of diff hit (target 85.01%)
Details
codecov/project 82.84% (-2.18%) compared to 4aa360a
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test_k8s Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@endzyme endzyme deleted the nh/add-more-controller-support branch Jul 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.