Revert the PR 1744 as the yaml files have been added back#1924
Revert the PR 1744 as the yaml files have been added back#1924geeknoid merged 2 commits intoistio:masterfrom
Conversation
The PR 1744 fixes the non-existent yaml files in the description of Citadel health checking in https://preliminary.istio.io/docs/tasks/security/health-check.html reported by the issue istio/istio#6922. Since the PR istio/istio#7178 adds back the removed yaml files, the PR 1744 is reverted.
|
/cc @wattli |
|
/cc @ymesika |
|
/cc @myidpt |
|
|
||
| {{< text bash >}} | ||
| $ kubectl apply -f install/kubernetes/istio-demo-auth.yaml | ||
| $ kubectl delete svc istio-citadel -n istio-system |
There was a problem hiding this comment.
We don't need to delete the service. The service is enabled by default now.
There was a problem hiding this comment.
Done: no deleting the service.
| {{< text bash >}} | ||
| $ kubectl delete deploy istio-citadel -n istio-system | ||
| $ kubectl delete -f install/kubernetes/istio-citadel-with-health-check.yaml | ||
| $ kubectl delete svc istio-citadel -n istio-system |
| - --probe-check-interval=15s # interval for health status check | ||
| - --logtostderr | ||
| - --stderrthreshold | ||
| - INFO |
There was a problem hiding this comment.
So I guess Citadel can't work well without these lines? If we need to add these lines, we should modify our yaml file template.
There was a problem hiding this comment.
To be consistent with the yaml file, these lines have been removed.
sdake
left a comment
sdake
left a comment
There was a problem hiding this comment.
Thank you for the contribution! One portion of this PR needs a small bit of rework. We expect Operators will create their own manifests with Helm. We do not expect nor want Operators to use istio-demo.yaml or istio-demo-auth.yaml for production workloads.
|
|
||
| ## Cleanup | ||
|
|
||
| * To disable health checking on Citadel, deploy Citadel with health checking disabled by setting the Helm argument `security.healthCheckEnabled` as `false`. |
There was a problem hiding this comment.
this content should not be lost. Please revert this part of the change.
There was a problem hiding this comment.
@sdake We actually don't have this Helm flag at the moment so shouldn't be documented
There was a problem hiding this comment.
@ymesika my bad, thought we did :) It must have been removed.
/lgtm
/approve
|
/assign @sdake |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lei-tang, sdake 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 |
|
Just noticed the linter failed in many areas - can you resolve? TIA |
7 participants
The PR 1744 fixes the non-existent yaml files in the description of Citadel health checking in
https://preliminary.istio.io/docs/tasks/security/health-check.html reported by the issue istio/istio#6922. Since the PR istio/istio#7178 adds back the removed yaml files, the PR 1744 is reverted.