Add health check flag to the Citadel#6961
Add health check flag to the Citadel#6961ymesika wants to merge 5 commits intoistio:release-1.0from ymesika:addCitadelHealthCheck
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ymesika Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| selfSigned: true # indicate if self-signed CA is used. | ||
| cleanUpOldCA: true | ||
| # Enabled Health Check will add the livenessProbe to the Citadel container | ||
| healthCheckEnabled: false |
There was a problem hiding this comment.
why do we need a flag for this? why not keep it default?
There was a problem hiding this comment.
@rshriram Just to match how it was before. There is a security task to enable this optional health check but I see your point of just having it part of default without flag.
@wattli @lei-tang Do you see any issue with having the livenessProbe part of default with no need to go through a task to learn how to enable it? It will make the task a verification/customization task.
There was a problem hiding this comment.
I do not see an issue of having health check on by default. But the section "(Optional) Configuring the health checking" (https://preliminary.istio.io/docs/tasks/security/health-check.html#optional-configuring-the-health-checking) still uses istio-citadel-with-health-check.yaml. I am not sure that adding the healthCheckEnabled flag will address the missing istio-citadel-with-health-check.yaml file problem in #6922 for the section "(Optional) Configuring the health checking".
There was a problem hiding this comment.
@lei-tang The idea is to remove the flag and always have the health check (livenessProve) in the container so users don't need to go through a task to configure/add it.
There was a problem hiding this comment.
Removing/adding the healthCheckEnabled flag does not solve the problem reported in #6922 because the the section "(Optional) Configuring the health checking" (https://preliminary.istio.io/docs/tasks/security/health-check.html#optional-configuring-the-health-checking) still uses istio-citadel-with-health-check.yaml to configure the parameters such as liveness-probe-interval, probe-check-interval, and etc. These parameters are orthogonal to the healthCheckEnabled flag.
There was a problem hiding this comment.
It's not orthogonal because Helm template generates an istio.yaml that have those paramters about livenessProbe and the users can modify those to fit their needs. Just like any customisation with all other components. Yes, the doc needs to be updated.
P.S. does it really deserve a task or is it just an installation option?
There was a problem hiding this comment.
# Conflicts: # install/kubernetes/helm/istio/values.yaml
Codecov Report
@@ Coverage Diff @@
## release-1.0 #6961 +/- ##
============================================
- Coverage 72% 71% -<1%
============================================
Files 355 360 +5
Lines 30444 31327 +883
============================================
+ Hits 21772 22198 +426
- Misses 7763 8254 +491
+ Partials 909 875 -34
Continue to review full report at Codecov.
|
Enables adding the
livenessProbeto the Citadel container.Fixes #6922