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: possibility to install operator as standalone app #32019

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Helm: possibility to install operator as standalone app #32019

merged 3 commits into from
Apr 18, 2024

Conversation

balous
Copy link

@balous balous commented Apr 17, 2024 •
edited

This PR fixes problems encountered when installing multiple instances of operator into a single kubernetes cluster. We use Cilium in Openstack clusters and external LoadBalancer connected to clustermesh. Agents are installed on LB and OS hosts, operator is run in a common kubernetes clusters. Each OS and LB cluster has its its own operator instance.

Problems needed to be fixed in helm chart:

  • logic around installing cluster-wide RBAC was added - rbac.create setting was already present in chart values.yaml but was not used anywhere. Now it is used to control ClusterRoles and ClusterRoleBindings installation throughout the chart.
  • hostNetwork for operator was made configurable so that operator pods won't collide on open ports
  • fine tuned logic around cilium-config config map installation - it is mounted as volume by agent, operator and clustermesh apiserver so it has to be installed if either of the three is enabled, not just agent.

@balous
helm: create configmap when operator or clustermesh apiserver is enabled
b21cf83 
cilium-config map is mounted as a volume in agent, operator and clustermesh
apiserver. It should be created when either of these components is enabled,
not just when agent is enabled. This fixes a problem when agent is disabled
and operator or clustermesh apiserver are enabled.

Signed-off-by: Petr Baloun <petr.baloun@firma.seznam.cz>
@balous balous requested review from a team as code owners April 17, 2024 07: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 17, 2024
@balous balous requested a review from sayboras April 17, 2024 07:56
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 17, 2024
@gandro
Copy link
Member

gandro commented Apr 17, 2024

Thanks for the PR. Since this adds a bit of complexity to the Helm charts, could you elaborate a bit why you need to install operator on a cluster with no cilium-agents? What purpose those that standalone operator serve?

@gandro gandro added 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 17, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 17, 2024
@balous
Copy link
Author

balous commented Apr 17, 2024

Thanks for the PR. Since this adds a bit of complexity to the Helm charts, could you elaborate a bit why you need to install operator on a cluster with no cilium-agents? What purpose those that standalone operator serve?

We run Cilium on OpenStack Hosts and load balacer. We install agents on OS and LB hosts (as systemd service). There are multiple OS clusters so there are multiple control planes (etcd, kube and cilium operator) that need to be run somewhere. We run them in a common K8S cluster as ordinary Kubernetes workload. Installing a Kubernetes control plane as helm chart is not a problem, Cilium chart (with everything but operator disabled) needs these fixes.

@gandro
Copy link
Member

gandro commented Apr 17, 2024

Thanks for the PR. Since this adds a bit of complexity to the Helm charts, could you elaborate a bit why you need to install operator on a cluster with no cilium-agents? What purpose those that standalone operator serve?

We run Cilium on OpenStack Hosts and load balacer. We install agents on OS and LB hosts (as systemd service). There are multiple OS clusters so there are multiple control planes (etcd, kube and cilium operator) that need to be run somewhere. We run them in a common K8S cluster as ordinary Kubernetes workload. Installing a Kubernetes control plane as helm chart is not a problem, Cilium chart (with everything but operator disabled) needs these fixes.

Are you running Cilium in kvstore mode then?

gandro
gandro reviewed Apr 17, 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.

The changes to the PR itself look fine to me. Though I think we should just remove the rbac.create option honestly. It hasn't been working for a while now and will just be a maintenance burden for the project

install/kubernetes/cilium/templates/cilium-agent/clusterrole.yaml Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
@balous
helm: possibility to disable hostNetwork for operator
da6f7a5 
This creates a possibility to run multiple operators in a single kubernetes cluster.

Signed-off-by: Petr Baloun <petr.baloun@firma.seznam.cz>
@balous
helm: install cluster-wide RBAC only when rbac.create == enabled
7c66783 
rbac.create is present in values.yaml but is not used anywhere in the chart.
This commit adds logic that installs ClusterRoles and ClusterRoleBindings
when rbac.create is enabled (which is the default).

It is useful to disabled cluster-wide RBAC when installing multiple instances
of cilium chart in a single Kubernetes cluster.

Signed-off-by: Petr Baloun <petr.baloun@firma.seznam.cz>
gandro
gandro approved these changes Apr 17, 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.

Helm LGTM

@balous
Copy link
Author

balous commented Apr 17, 2024

Are you running Cilium in kvstore mode then?

Yes, we do. The purpose of this is to have Kubernetes and OpenStack clusters and even load balancer connected to a single mesh. We use etcd installed as helm chart in the same namespace as K8S control plane and Cilium operator but we are now migrating it to OS VMs as this solution is not very reliable.

sayboras
sayboras approved these changes Apr 17, 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.

LGTM for servicemesh's related changes.

YutaroHayakawa
YutaroHayakawa approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

ClusterMesh LGTM.

@gandro
Copy link
Member

gandro commented Apr 18, 2024

/test

@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 18, 2024
@gandro gandro added this pull request to the merge queue Apr 18, 2024
Merged via the queue into cilium:main with commit 2fe9337 Apr 18, 2024
62 checks passed
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

@joamaki joamaki joamaki approved these changes

@sayboras sayboras sayboras approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa 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. 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
@balous @gandro @joamaki @sayboras @YutaroHayakawa