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.

Sign up for GitHub

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

Jump to bottom

helm: Add automatic lookup option for k8s service info #31885

Merged
merged 1 commit into from
Apr 15, 2024
Merged

helm: Add automatic lookup option for k8s service info #31885

merged 1 commit into from
Apr 15, 2024

Conversation

kreeuwijk
Copy link
Contributor

@kreeuwijk kreeuwijk commented Apr 10, 2024 •
edited
Loading

This commit adds an option for kubeadm-based clusters to specify

k8sServiceHost: "auto"

and have a helper function automatically determine the Kubernetes host and port information from the cluster-info ConfigMap in the kube-public namespace, and use that to populate the k8sServiceHost and k8sServicePort values. This can significantly simplify the enabling of kubeproxy-less mode, especially for CAPI clusters where the value for k8sServiceHost cannot always be known ahead of time.

If any other value than "auto" is provided for k8sServiceHost, the behavior reverts to its original form.

Add option to automatically discover k8sServiceHost and k8sServicePort info (kubeadm clusters only)

@kreeuwijk kreeuwijk requested review from a team as code owners April 10, 2024 13:55
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 10, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 10, 2024
@kreeuwijk
Copy link
Contributor Author

label should be kind/enhancement

@gandro gandro added kind/enhancement This would improve or streamline existing functionality. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience labels Apr 10, 2024
@maintainer-s-little-helper 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 10, 2024
gandro
gandro reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, this is a neat idea!

Overall, I like the approach but I have a bit of feedback on the implementation

install/kubernetes/cilium/templates/_helpers.tpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/templates/_helpers.tpl Outdated Show resolved Hide resolved
gandro
gandro reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Awesome, thank you, this looks good to me now overall. Two minor things:

  1. Please squash your two commits into one
  2. I believe you have to run make -C Documentation update-helm-values to also update the Helm values in the web docs

@kreeuwijk
Copy link
Contributor Author

@gandro I squashed the commits.

I don't think there's any other documentation to update though I already updated install/kubernetes/cilium/README.md and there's no changes to the regular documentation as the change is only for the helm chart itself.

@gandro
Copy link
Member

gandro commented Apr 11, 2024 •
edited
Loading

Awesome!

I don't think there's any other documentation to update though I already updated install/kubernetes/cilium/README.md and there's no changes to the regular documentation as the change is only for the helm chart itself.

We also have https://docs.cilium.io/en/latest/helm-reference/ which like the README.md contains all the Helm values and their description. That also needs to be regenerated via make -C Documentation update-helm-values, which is why the docs workflow is failing

While at it, the commit message lint fails with the following error message:

Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 501)

Thanks!

@maintainer-s-little-helper
Copy link

Commit 326b84e does not match "(?m)^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 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 12, 2024
@maintainer-s-little-helper 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 12, 2024
@kreeuwijk kreeuwijk requested a review from a team as a code owner April 12, 2024 11:17
@kreeuwijk kreeuwijk requested a review from qmonnet April 12, 2024 11:17
@kreeuwijk
Copy link
Contributor Author

@gandro ok I think I got it sorted now. Apologies, git is not my strong suit 😅

@maintainer-s-little-helper
Copy link

Commit e2705ca does not match "(?m)^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 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 12, 2024
@maintainer-s-little-helper 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 12, 2024
@sayboras sayboras changed the title k8sService: automatic lookup option helm: Add automatic automatic lookup option for k8s service info Apr 12, 2024
sayboras
sayboras reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Seems like Documentation/helm-values.tmp is added unintentionally, I have only one nitpick, the rest looks good to me.

install/kubernetes/cilium/templates/_helpers.tpl Outdated Show resolved Hide resolved
@kreeuwijk
Copy link
Contributor Author

@sayboras removed Documentation/helm-values.tmp

sayboras
sayboras approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks a lot, lgtm.

@kreeuwijk kreeuwijk force-pushed the auto-k8s-service branch 2 times, most recently from 1a4e016 to 6b0e81b Compare April 12, 2024 12:17
@sayboras sayboras changed the title helm: Add automatic automatic lookup option for k8s service info helm: Add automatic lookup option for k8s service info Apr 12, 2024
@kreeuwijk
k8sService: automatic lookup option
3d28574 
This commit adds an option for kubeadm-based clusters to specify
```
k8sServiceHost: "auto"
```
and have a helper function automatically determine the Kubernetes host and port information from the `cluster-info` ConfigMap in the `kube-public` namespace, and use that to populate the `k8sServiceHost` and `k8sServicePort` values. This can significantly simplify the enabling of kubeproxy-less mode, especially for CAPI clusters where the value for `k8sServiceHost` cannot always be known ahead of time.

If any other value than "auto" is provided for `k8sServiceHost`, the behavior reverts to its original form.

Signed-off-by: Kevin Reeuwijk <kevin.reeuwijk@spectrocloud.com>
@qmonnet
Copy link
Member

qmonnet commented Apr 12, 2024

/test

qmonnet
qmonnet approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thank you

@sayboras sayboras requested a review from gandro April 12, 2024 23:59
@gandro gandro removed the request for review from tommyp1ckles April 15, 2024 07:53
@gandro gandro added this pull request to the merge queue Apr 15, 2024
Merged via the queue into cilium:main with commit 41daa21 Apr 15, 2024
62 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 15, 2024
@kreeuwijk kreeuwijk deleted the auto-k8s-service branch April 15, 2024 08:50
@ozhankaraman
Copy link

ozhankaraman commented May 2, 2024 •
edited
Loading

Hi, do we plan to add this feature to upcoming 1.16 release? This is an important feature for clusterapi/kubeadm based kubernetes deployments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@sayboras sayboras sayboras approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience kind/community-contribution This was a contribution made by a community member. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kreeuwijk @gandro @qmonnet @ozhankaraman @sayboras