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

add support for validating admission webhook in terrascan #620

Merged

Conversation

kanchwala-yusuf
Copy link
Contributor

No description provided.

@kanchwala-yusuf kanchwala-yusuf force-pushed the feature/k8s-validating-webhooks branch 3 times, most recently from d813f8f to a439fea Compare March 17, 2021 11:33
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #620 (1911323) into master (6103c45) will decrease coverage by 2.67%.
The diff coverage is 57.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #620      +/-   ##
==========================================
- Coverage   78.09%   75.41%   -2.68%     
==========================================
  Files         104      109       +5     
  Lines        2597     3002     +405     
==========================================
+ Hits         2028     2264     +236     
- Misses        422      570     +148     
- Partials      147      168      +21     
Impacted Files Coverage Δ
pkg/http-server/server.go 100.00% <ø> (ø)
pkg/http-server/start.go 0.00% <0.00%> (ø)
pkg/http-server/webhook-scan-logs.go 0.00% <0.00%> (ø)
pkg/utils/tempfile.go 40.00% <40.00%> (ø)
pkg/http-server/webhook-scan.go 70.00% <70.00%> (ø)
pkg/cli/server.go 50.00% <75.00%> (+30.00%) ⬆️
pkg/k8s/dblogs/webhook-scan-logger.go 78.18% <78.18%> (ø)
pkg/k8s/admission-webhook/validating-webhook.go 81.48% <81.48%> (ø)
pkg/config/config-reader.go 86.95% <100.00%> (+0.59%) ⬆️
pkg/http-server/handler.go 100.00% <100.00%> (ø)
... and 7 more

@kanchwala-yusuf kanchwala-yusuf force-pushed the feature/k8s-validating-webhooks branch 4 times, most recently from 9f19b92 to f8106b9 Compare March 19, 2021 17:36
@kanchwala-yusuf kanchwala-yusuf force-pushed the feature/k8s-validating-webhooks branch 2 times, most recently from 71126ca to 61162e2 Compare March 31, 2021 03:46
pkg/http-server/webhook-scan-logs.go Outdated Show resolved Hide resolved
pkg/cli/server.go Outdated Show resolved Hide resolved
pkg/http-server/webhook-scan-logs.go Show resolved Hide resolved
pkg/http-server/webhook-scan.go Show resolved Hide resolved
pkg/http-server/webhook-scan_test.go Outdated Show resolved Hide resolved
pkg/k8s/admission-webhook/validating-webhook.go Outdated Show resolved Hide resolved
pkg/k8s/admission-webhook/validating-webhook_test.go Outdated Show resolved Hide resolved
return &APIHandler{}
func NewAPIHandler(configFile string) *APIHandler {
return &APIHandler{
configFile: configFile,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should change this logic. the configFile is loaded in the LoadGlobalConfig( ) call. by the cli framework itself here https://github.com/accurics/terrascan/blob/master/pkg/cli/register.go#L44

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config file changes are quite significant and can be handled as separate PR. adding those changes in this PR would just increase the bulk of this PR.

If you are okay, we can open a separate issue for this and address it as a part of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. let's tackle this refactor seperately.

I'll also create one for helm charts.

pkg/http-server/start.go Outdated Show resolved Hide resolved
@@ -29,13 +29,19 @@ type Route struct {

// Routes returns a slice of routes of API endpoints to be registered with
// http server
func (g *APIServer) Routes() []*Route {
h := NewAPIHandler()
func (g *APIServer) Routes(configFile string) []*Route {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and ofcourse we'll have to change the function a bit.. to consume the config values... config package can be used directly.

func (g *APIServer) Routes() []*Route {
h := NewAPIHandler()
func (g *APIServer) Routes(configFile string) []*Route {
h := NewAPIHandler(configFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this function

@kanchwala-yusuf kanchwala-yusuf force-pushed the feature/k8s-validating-webhooks branch from df17b05 to 8bca20e Compare April 5, 2021 04:06
jlk
jlk previously approved these changes Apr 6, 2021
Copy link
Contributor

@jlk jlk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Wouldn't mind changing that CSS file, though.

pkg/http-server/templates/show.html Outdated Show resolved Hide resolved
result runtime.Output
)

if flag.Lookup("test.v") != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not looking for a change, but this feels strange as almost all the code uses Cobra...

patilpankaj212
patilpankaj212 previously approved these changes Apr 7, 2021
Copy link
Contributor

@patilpankaj212 patilpankaj212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
We should add e2e tests for admission webhook later.

Copy link
Contributor

@devang-gaur devang-gaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requested a small change in docs

@sonarcloud
Copy link

sonarcloud bot commented Apr 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@patilpankaj212 patilpankaj212 merged commit dcfbd54 into tenable:master Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants