Add leader elections flags/values to the deployment manifests #455

a-hilaly · 2023-07-17T21:05:48Z

Issue: aws-controllers-k8s/community#1753 (comment)

ACK controllers use k8s-sigs/controller-runtime behind the scenes, which
support leader election. This feature is not properly working due to a
missing configuration LeaderElectionNamespace which is used by the
manager to create k8s.io/coordination Lease objects.

This patch sets the default LeaderElectionNamespace to ack-system
and adds the capability of enabling leader election using helm values.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Co-authored-by: Adam Cornett [email protected]

a-hilaly · 2023-07-17T21:06:34Z

templates/cmd/controller/main.go.tpl

+		LeaderElection:          ackCfg.EnableLeaderElection,
+		LeaderElectionID:        awsServiceAPIGroup,
+		Namespace:               ackCfg.WatchNamespace,
+		LeaderElectionNamespace: ackSystemNamespace,


I was thinking about adding a --leader-election-namespace flag to allow users to set their own.. thoughts?

I'm not the sure what the use case for this flag is, or what it actually does under the covers. operator-sdk only scaffolds with the below options:

Scheme: scheme, MetricsBindAddress: metricsAddr, Port: 9443, HealthProbeBindAddress: probeAddr, LeaderElection: enableLeaderElection, LeaderElectionID: leaderElectionID,

LeaderElectionNamespace is the namespace where the Lease object is created

Without namespace, i'm running into unable to create controller manager {"aws.service": "lambda", "error": "unable to find leader election namespace: not running in-cluster, please specify LeaderElectionNamespace"} - does operator-sdk use leases as well? or maybe configmaps (that's what controller-runtime used before leases introduction)?

a-hilaly · 2023-07-17T21:11:30Z

Example logs of a controller acquiring leader lease:

I0717 22:07:29.267656  313484 leaderelection.go:248] attempting to acquire leader lease ack-system/lambda.services.k8s.aws...
I0717 22:07:29.282064  313484 leaderelection.go:258] successfully acquired lease ack-system/lambda.services.k8s.aws
2023-07-17T22:07:29.282+0100	DEBUG	events	ma_ca5abb0a-74fe-45a9-85e1-3b0c3a95e9ac became leader	{"type": "Normal", "object": {"kind":"Lease","namespace":"ack-system","name":"lambda.services.k8s.aws","uid":"a68f0032-2021-436a-b4a2-91fb44f297e9","apiVersion":"coordination.k8s.io/v1","resourceVersion":"383697"}, "reason": "LeaderElection"}

Example logs of a controller waiting for leader lease to be freed:

I0717 22:10:32.402053  314991 leaderelection.go:248] attempting to acquire leader lease ack-system/lambda.services.k8s.aws...

acornett21

I feel like this change should be duplicated to the kuztomize templates as well.

a-hilaly · 2023-07-25T23:33:01Z

/retest

a-hilaly · 2023-07-27T16:27:17Z

/retest

acornett21

A few changes to this are needed. Also, we still need the leader-elect RBAC files and changes that I sent as a patch, otherwise this will not work.

templates/config/controller/deployment.yaml.tpl

Issue #, if available: N/A Description of changes: Fixing deployment `args` to have boolean values on a single line. This is needed since the presence of the flag itself is a `truthy` statement, and the next line for the `env` is ignored. See [this](#455 (comment)) comment on `leader-elect` PR for more info. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly · 2023-08-25T14:49:38Z

/test ecr-controller-test

acornett21 · 2023-08-29T14:41:53Z

templates/config/rbac/leader_election_role.yaml.tpl

+  - ""
+  resources:
+  - configmaps
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete


I have confirmed that ACK does not need the configmap. This is sort of an either/or thing, where configmap or leases can be used, and the operator developer can choose which to use. For legacy purposes and for operators that might have been developed before leases were created, this is kept in the scaffolding.

Since ACK isn't exposing this flag which allows the choice, we are safe to remove the confimap section from the RBAC.

@a-hilaly I hope all the above makes sense, let me know if there are questions...After this is removed I think we are good to go.

a-hilaly · 2023-08-31T16:55:20Z

/retest

a-hilaly · 2023-09-05T18:40:16Z

/hold

need to pin runtime v0.27.1

acornett21 · 2023-09-05T19:48:58Z

@a-hilaly Why did you revert the RBAC changes?

a-hilaly · 2023-09-05T20:44:41Z

@a-hilaly Why did you revert the RBAC changes?

@acornett21 Looks like it's gonna be a bigger change than we expected.. since the feature is disabled by default, it's safe to go with this now and we will address the RBAC matters in a seperate PR

a-hilaly · 2023-09-05T21:11:41Z

/test dynamodb-controller-test

acornett21 · 2023-09-05T21:38:31Z

since the feature is disabled by default, it's safe to go with this now and we will address the RBAC matters in a seperate PR

@a-hilaly Can you elaborate on this? If someone goes to enable this, the controller will fail to function and throw errors without RBAC changes present.

a-hilaly · 2023-09-05T22:03:35Z

@a-hilaly Can you elaborate on this? If someone goes to enable this, the controller will fail to function and throw errors without RBAC changes present.

Yes that's correct.

We wanted to make the release quickly today without RBAC support for few reasons:

We need to release new images with security patches + fix a panic with controllers running in k8s 1.27
The current tests with Leases RBACs were failing
It's a bit hard to know what do with cluster scoped vs namepsaced scoped controllers. (this influences whether a controller will use a role vs clusterRole
FieldExport currently doesn't come with supported RBAC permissions. Customers set their own (secret/cm) RBACs. Leader election will take similar path at least in the short term.

I still would like to come up with a plan to add those RBACs in a seperate PR, i want to open one as soon as this one is merged.

a-hilaly · 2023-09-05T22:04:04Z

/test s3-controller-test
/test ecr-controller-test

a-hilaly · 2023-09-06T20:53:13Z

@acornett21 good points, and thank you for bringing this! I just added few configurations for kustomize and helm chart templates.

ACK controllers use k8s-sigs/controller-runtime behind the scenes, which support leader election. This feature is not properly working due to a missing configuration `LeaderElectionNamespace` which is used by the manager to create `k8s.io/coordination` Lease objects. This patch sets the default `LeaderElectionNamespace` to `ack-system` and adds the capability of enabling leader election using helm values. Co-authored-by: Adam Cornett <[email protected]>

a-hilaly · 2023-09-06T21:35:57Z

/retest

templates/cmd/controller/main.go.tpl

templates/config/rbac/leader-election-role-binding.yaml.tpl

templates/config/rbac/leader-election-role.yaml.tpl

templates/config/rbac/leader-election-role-binding.yaml.tpl

templates/helm/values.yaml.tpl

RedbackThomson

/lgtm

RedbackThomson

/lgtm

a-hilaly · 2023-09-06T23:59:07Z

/retest

a-hilaly · 2023-09-07T00:07:20Z

/lgtm cancel

a-hilaly · 2023-09-07T00:07:34Z

/hold

a-hilaly · 2023-09-07T00:24:19Z

/unhold

RedbackThomson · 2023-09-07T00:27:49Z

/lgtm

ack-prow · 2023-09-07T00:27:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, RedbackThomson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RedbackThomson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…1884) Issue #, if available: - Relates: aws-controllers-k8s/code-generator#455 Description of changes: Adding these env's to the configmap so when leader election code is merged, installs on OpenShift can still function properly. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ntrollers-k8s#457) Issue #, if available: N/A Description of changes: Fixing deployment `args` to have boolean values on a single line. This is needed since the presence of the flag itself is a `truthy` statement, and the next line for the `env` is ignored. See [this](aws-controllers-k8s#455 (comment)) comment on `leader-elect` PR for more info. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow bot requested review from jljaco and vijtrip2 July 17, 2023 21:05

ack-prow bot added the approved label Jul 17, 2023

a-hilaly commented Jul 17, 2023

View reviewed changes

a-hilaly requested a review from RedbackThomson July 17, 2023 21:06

a-hilaly mentioned this pull request Jul 17, 2023

Support controllers leader election mode aws-controllers-k8s/community#1753

Closed

acornett21 suggested changes Jul 17, 2023

View reviewed changes

a-hilaly force-pushed the leader-election branch from 6449e45 to 5d66bae Compare July 19, 2023 21:45

a-hilaly changed the title ~~Add missing leader election configuration to the controller manager~~ Add leader elections flags/values to the deployment manifests Jul 19, 2023

acornett21 reviewed Aug 2, 2023

View reviewed changes

templates/config/controller/deployment.yaml.tpl Outdated Show resolved Hide resolved

templates/config/controller/deployment.yaml.tpl Outdated Show resolved Hide resolved

acornett21 mentioned this pull request Aug 2, 2023

fixing boolean flag in deployment args to be on a single line #457

Merged

a-hilaly force-pushed the leader-election branch 3 times, most recently from b5a9929 to fb8f197 Compare August 24, 2023 21:21

acornett21 reviewed Aug 29, 2023

View reviewed changes

acornett21 mentioned this pull request Aug 30, 2023

updating OpenShift install docs to include leader election env vars aws-controllers-k8s/community#1884

Merged

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2023

a-hilaly force-pushed the leader-election branch from 66bb8f8 to 79b3864 Compare September 6, 2023 20:21

acornett21 approved these changes Sep 6, 2023

View reviewed changes

a-hilaly force-pushed the leader-election branch from b1fad3c to d61015a Compare September 6, 2023 21:10

RedbackThomson suggested changes Sep 6, 2023

View reviewed changes

a-hilaly force-pushed the leader-election branch from eca490f to 1601f66 Compare September 6, 2023 23:30

RedbackThomson approved these changes Sep 6, 2023

View reviewed changes

ack-prow bot assigned RedbackThomson Sep 6, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023

Tiny improvements

2b46d30

a-hilaly force-pushed the leader-election branch from 1601f66 to 2b46d30 Compare September 6, 2023 23:37

ack-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023

RedbackThomson reviewed Sep 6, 2023

View reviewed changes

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023

ack-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2023

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023

Fix helm container arguments

3adc84a

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2023

ack-prow bot merged commit 2f2b5e9 into aws-controllers-k8s:main Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add leader elections flags/values to the deployment manifests #455

Add leader elections flags/values to the deployment manifests #455

a-hilaly commented Jul 17, 2023 •

edited

Loading

a-hilaly Jul 17, 2023

acornett21 Jul 17, 2023

a-hilaly Jul 18, 2023

a-hilaly Jul 18, 2023

a-hilaly commented Jul 17, 2023

acornett21 left a comment

a-hilaly commented Jul 25, 2023

a-hilaly commented Jul 27, 2023

acornett21 left a comment

a-hilaly commented Aug 25, 2023

acornett21 Aug 29, 2023

a-hilaly commented Aug 31, 2023

a-hilaly commented Sep 5, 2023

acornett21 commented Sep 5, 2023

a-hilaly commented Sep 5, 2023 •

edited

Loading

a-hilaly commented Sep 5, 2023

acornett21 commented Sep 5, 2023

a-hilaly commented Sep 5, 2023

a-hilaly commented Sep 5, 2023

a-hilaly commented Sep 6, 2023

a-hilaly commented Sep 6, 2023

RedbackThomson left a comment

RedbackThomson left a comment

a-hilaly commented Sep 6, 2023

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 7, 2023

RedbackThomson commented Sep 7, 2023

ack-prow bot commented Sep 7, 2023

Add leader elections flags/values to the deployment manifests #455

Add leader elections flags/values to the deployment manifests #455

Conversation

a-hilaly commented Jul 17, 2023 • edited Loading

a-hilaly Jul 17, 2023

Choose a reason for hiding this comment

acornett21 Jul 17, 2023

Choose a reason for hiding this comment

a-hilaly Jul 18, 2023

Choose a reason for hiding this comment

a-hilaly Jul 18, 2023

Choose a reason for hiding this comment

a-hilaly commented Jul 17, 2023

acornett21 left a comment

Choose a reason for hiding this comment

a-hilaly commented Jul 25, 2023

a-hilaly commented Jul 27, 2023

acornett21 left a comment

Choose a reason for hiding this comment

a-hilaly commented Aug 25, 2023

acornett21 Aug 29, 2023

Choose a reason for hiding this comment

a-hilaly commented Aug 31, 2023

a-hilaly commented Sep 5, 2023

acornett21 commented Sep 5, 2023

a-hilaly commented Sep 5, 2023 • edited Loading

a-hilaly commented Sep 5, 2023

acornett21 commented Sep 5, 2023

a-hilaly commented Sep 5, 2023

a-hilaly commented Sep 5, 2023

a-hilaly commented Sep 6, 2023

a-hilaly commented Sep 6, 2023

RedbackThomson left a comment

Choose a reason for hiding this comment

RedbackThomson left a comment

Choose a reason for hiding this comment

a-hilaly commented Sep 6, 2023

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 7, 2023

RedbackThomson commented Sep 7, 2023

ack-prow bot commented Sep 7, 2023

a-hilaly commented Jul 17, 2023 •

edited

Loading

a-hilaly commented Sep 5, 2023 •

edited

Loading