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

[feat.] Merge Webhook and Server Helm Chart #817

Merged
merged 10 commits into from
May 27, 2021

Conversation

rahulchheda
Copy link
Contributor

@rahulchheda rahulchheda commented May 26, 2021

Tried to merge Webhook and Server charts in this PR. We really don't need to maintain them separately.
PTAL, and let me know if any improvements are needed.

Signed-off-by: Rahul M Chheda rahul.chheda@accurics.com

Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>

webhook:
name: webhook.terrascan.io
failurePolicy: Ignore
Copy link
Contributor Author

@rahulchheda rahulchheda May 26, 2021

Choose a reason for hiding this comment

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

Had to change this to Ignore, because if service is un-available for webhook, it should not block all the specified resources in cluster.
Hope this is the desired behaviour.
cc: @dev-gaur @kanchwala-yusuf @jlk

Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>
@@ -4,7 +4,6 @@ metadata:
name: {{ .Values.name }}
namespace: {{ .Release.Namespace }}
spec:
type: LoadBalancer
Copy link
Contributor Author

@rahulchheda rahulchheda May 26, 2021

Choose a reason for hiding this comment

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

No need to create LoadBalancer service, until we are using this service from outside of this cluster. It's the user choice to change it afterwards to enhance its usecases.
cc: @jlk @kanchwala-yusuf @dev-gaur

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #817 (0e56753) into master (a4d7af9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #817   +/-   ##
=======================================
  Coverage   74.85%   74.85%           
=======================================
  Files         111      111           
  Lines        3345     3345           
=======================================
  Hits         2504     2504           
  Misses        656      656           
  Partials      185      185           

Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>
Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>
@@ -0,0 +1,63 @@
{{- if eq .Values.webhook.failurePolicy "Fail" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to create this file just to support validatingwebhookconfiguration failurePolicy to be FAIL.

It turns out, webhook doesn't allow the terrascan server pod to come up in case failurePolicy is Fail.

So, as a workaround, we create the webhook w/ Ignore, and then upgrade it to Fail in. post install chart hook. ref: https://v2.helm.sh/docs/charts_hooks/

Copy link
Contributor

@devang-gaur devang-gaur May 27, 2021

Choose a reason for hiding this comment

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

Had to create this file just to support validatingwebhookconfiguration failurePolicy to be FAIL.
It turns out, webhook doesn't allow the terrascan server pod to come up in case failurePolicy is Fail.
So, as a workaround, we create the webhook w/ Ignore, and then upgrade it to Fail in. post install chart hook. ref: https://v2.helm.sh/docs/charts_hooks/

Can you add this as a comment in the vw yaml file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Signed-off-by: Devang <devang.gaur.7@gmail.com>
@devang-gaur devang-gaur mentioned this pull request May 27, 2021
Signed-off-by: Devang <devang.gaur.7@gmail.com>
@devang-gaur
Copy link
Contributor

devang-gaur commented May 27, 2021

Hi @rahulchheda, I sent a PR to your branch for all the remaining changes.

rahulchheda#1

devang-gaur and others added 3 commits May 27, 2021 21:38
Signed-off-by: Devang <devang.gaur.7@gmail.com>
minor changes and shifting helm charts to deploy/helm/
Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>
@rahulchheda rahulchheda changed the title Merge Webhook and Server Helm Chart [feat.] Merge Webhook and Server Helm Chart May 27, 2021
Signed-off-by: Rahul M Chheda <rahul.chheda@accurics.com>
@sonarcloud
Copy link

sonarcloud bot commented May 27, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

2 participants