Add support to enable EndpointStatus in Helm chart #15844

carloscastrojumo · 2021-04-23T09:20:46Z

This commit introduces a new Helm parameter to enable --endpoint-status option in Cilium.

endpointStatus.enabled is disabled by default and endpointStatus.status is required once enabled and suggesting status: policy, controllers, health, log, state.

This aims to activate the values in the columns present in kubectl get cep.

maintainer-s-little-helper · 2021-04-23T09:20:48Z

Commit f039d7115c3230cd63663f98300693de5963ddef does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

aanm

Thanks for the changes, only one small clarification requested.

aanm · 2021-04-23T11:48:27Z

install/kubernetes/cilium/README.md

@@ -129,6 +129,8 @@ contributors across the globe, there is almost always someone available to help.
 | encryption.secretName | string | `"cilium-ipsec-keys"` | Name of the Kubernetes secret containing the encryption keys. This option is only effective when encryption.type is set to ipsec. |
 | encryption.type | string | `"ipsec"` | Encryption method. Can be either ipsec or wireguard. |
 | endpointHealthChecking.enabled | bool | `true` |  |
+| endpointStatus.enabled | bool | `false` | Enable endpoint status |
+| endpointStatus.status | string | `policy` | Endpoint status. Can be policy, health, controllers, logs or state |


If it can be all of those parameters, why does it default to policy?

I set it default to policy cause I was thinking most users will set this option true to enable the columns in the cep command but I'm happy to change to empty and required field so users can select the status they want.

Updated PR and description

I think we probably should enable policy by default specifically for this reason. It impacts scale, but we have a scalability guide to instruct users how to get the most scalable setup. On the other hand, disabling this by default means that most users just see the CEP output as broken and it's not obvious why.

nebril · 2021-04-23T12:06:12Z

install/kubernetes/cilium/templates/cilium-configmap.yaml

@@ -571,6 +571,9 @@ data:
 {{- if hasKey .Values.k8s "requireIPv6PodCIDR" }}
  k8s-require-ipv6-pod-cidr: {{ .Values.k8s.requireIPv6PodCIDR | quote }}
 {{- end }}
+{{- if and .Values.endpointStatus .Values.endpointStatus.enabled }}


Wouldn't {{- if .Values.endpointStatus.enabled }} be enough here?

Yes, it would. Updating.

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

aanm · 2021-04-23T13:13:24Z

install/kubernetes/cilium/values.yaml

+# -- Enable endpoint status
+endpointStatus:
+  enabled: false
+


I think we can leave the "status" field here but it would be empty.

aanm · 2021-04-23T13:15:07Z

install/kubernetes/cilium/templates/cilium-configmap.yaml

@@ -571,6 +571,9 @@ data:
 {{- if hasKey .Values.k8s "requireIPv6PodCIDR" }}
  k8s-require-ipv6-pod-cidr: {{ .Values.k8s.requireIPv6PodCIDR | quote }}
 {{- end }}
+{{- if .Values.endpointStatus.enabled }}
+  endpoint-status: {{ required "endpointStatus.status required: policy, health, controllers, logs or state" .Values.endpointStatus.status | quote }}


would be nice to give an example with 2 or mor options set. for example: "endpointStatus.status required: policy, health, controllers, logs and / or state. For 2 or more options use a comma: "policy, health""

It's a good example. Updated.

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

maintainer-s-little-helper · 2021-04-25T18:40:32Z

Commit 8c255f6e34c6fa1280f129bf414ad82cec682a8f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2021-04-25T18:41:13Z

Commit 8c255f6e34c6fa1280f129bf414ad82cec682a8f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

kaworu

Thanks for the contribution @carloscastrojumo, I have a small comment but the PR looks good to me!

install/kubernetes/cilium/values.yaml

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

aanm · 2021-04-26T13:52:41Z

test-me-please

kaworu · 2021-04-27T11:07:15Z

test-gke

aanm · 2021-04-27T17:43:37Z

merging since this is only changing helm and the CI failures might be flakes

carloscastrojumo requested a review from a team as a code owner April 23, 2021 09:20

carloscastrojumo requested review from a team and kaworu April 23, 2021 09:20

maintainer-s-little-helper bot assigned kaworu Apr 23, 2021

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 23, 2021

maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 23, 2021

carloscastrojumo requested a review from nathanjsweet April 23, 2021 09:20

maintainer-s-little-helper bot assigned nathanjsweet Apr 23, 2021

Add support to enable EndpointStatus in Helm chart

3994360

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

carloscastrojumo force-pushed the add-endpoint-status-helm-chart branch from f039d71 to 3994360 Compare April 23, 2021 09:40

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 23, 2021

aanm requested changes Apr 23, 2021

View reviewed changes

aanm added the release-note/misc This PR makes changes that have no direct user impact. label Apr 23, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 23, 2021

nebril reviewed Apr 23, 2021

View reviewed changes

carloscastrojumo added 2 commits April 23, 2021 13:33

Remove double verification

e45935b

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

Remove policy as default status and make status required once enabled

f0c58be

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

carloscastrojumo requested a review from aanm April 23, 2021 12:54

maintainer-s-little-helper bot assigned aanm Apr 23, 2021

carloscastrojumo requested a review from nebril April 23, 2021 12:54

maintainer-s-little-helper bot assigned nebril Apr 23, 2021

aanm requested changes Apr 23, 2021

View reviewed changes

Add status default as empty and example for more options

471b07e

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

carloscastrojumo requested a review from aanm April 23, 2021 14:57

aanm approved these changes Apr 23, 2021

View reviewed changes

aanm removed their assignment Apr 23, 2021

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 25, 2021

carloscastrojumo force-pushed the add-endpoint-status-helm-chart branch from 8c255f6 to 8b8ac6e Compare April 25, 2021 18:41

Update helm docs

a373912

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

carloscastrojumo force-pushed the add-endpoint-status-helm-chart branch from 8b8ac6e to a373912 Compare April 25, 2021 18:43

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 25, 2021

kaworu approved these changes Apr 26, 2021

View reviewed changes

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

Improve helm README.md documentation

184f0f6

Signed-off-by: Carlos Castro <carlos.castro@jumo.world>

aanm approved these changes Apr 26, 2021

View reviewed changes

aanm merged commit 8183ebe into cilium:master Apr 27, 2021

1.10.0 automation moved this from In progress to Done Apr 27, 2021

This was referenced Apr 28, 2021

Prepare for release v1.10.0-rc1 #15896

Closed

Prepare for release v1.10.0-rc1 #15897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to enable EndpointStatus in Helm chart #15844

Add support to enable EndpointStatus in Helm chart #15844

carloscastrojumo commented Apr 23, 2021 •

edited

maintainer-s-little-helper bot commented Apr 23, 2021

aanm left a comment

aanm Apr 23, 2021

carloscastrojumo Apr 23, 2021

carloscastrojumo Apr 23, 2021

joestringer Apr 26, 2021

nebril Apr 23, 2021

carloscastrojumo Apr 23, 2021

aanm Apr 23, 2021

aanm Apr 23, 2021

carloscastrojumo Apr 23, 2021

maintainer-s-little-helper bot commented Apr 25, 2021

maintainer-s-little-helper bot commented Apr 25, 2021

kaworu left a comment

aanm commented Apr 26, 2021

kaworu commented Apr 27, 2021

aanm commented Apr 27, 2021

Add support to enable EndpointStatus in Helm chart #15844

Add support to enable EndpointStatus in Helm chart #15844

Conversation

carloscastrojumo commented Apr 23, 2021 • edited

maintainer-s-little-helper bot commented Apr 23, 2021

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maintainer-s-little-helper bot commented Apr 25, 2021

maintainer-s-little-helper bot commented Apr 25, 2021

kaworu left a comment

Choose a reason for hiding this comment

aanm commented Apr 26, 2021

kaworu commented Apr 27, 2021

aanm commented Apr 27, 2021

carloscastrojumo commented Apr 23, 2021 •

edited