[release-1.26] [occm] Use standard service account name in OCCM helm chart #2449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes the `openstack-` prefix from the service account name
used by the cloud-controller-manager and moves the default values into
the values file.
The change is motivated by the following:
- Create suitable service accounts, cluster roles and cluster role
bindings for use with `--use-service-accounts-credentials=true`
- Normalise service account names in the helm chart and plain
manifests
- Adhere to naming conventions across external cloud controller
managers for different clouds (e.g. AWS, GCP, ...)
Specifically the first point deserves further details. Prior to this
change, users who install the cloud controller manager with helm,
would run into the following error when creating load balancers:
```
E0818 08:27:33.802407 11 controller.go:291] error processing service default/hello-bug (will retry): failed to ensure load balancer: failed to patch service object default/hello-bug: services "hello-bug" is forbidden: User "system:serviceaccount:kube-system:cloud-controller-manager" cannot patch resource "services" in API group "" in the namespace "default"
```
Which is due to the fact that the controller is running with the
`cloud-controller-manager` service account because
`--use-service-account-credentials` is set to `true` by default and
the client is initialised with:
```
clientset := clientBuilder.ClientOrDie("cloud-controller-manager")
```
Whilst users can work around this by passing
`--use-service-account-credentials=false`, the desired behaviour would
be to install suitable RBAC in the first place.
See:
- https://kubernetes.io/docs/concepts/architecture/cloud-controller/
- https://kubernetes.io/docs/tasks/administer-cluster/running-cloud-controller/
- kubernetes#2049
- kubernetes#1722
- kubernetes#1755
Signed-off-by: Wolodja Wentland <[email protected]>
The space character was sadly introduced in an earlier PR and had not been picked up by the helm linter, presumably because the GHA did not run. Signed-off-by: Wolodja Wentland <[email protected]>
k8s-ci-robot
added
release-note-action-required
k8s-ci-robot
added
the
size/S
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jichenjc 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This is a cherry-pick of:
Which issue this PR fixes(if applicable):
fixes #2440
Special notes for reviewers:
At the time #2332 was merged the CI was sadly somewhat broken and missed a spurious space. This has been fixed in #2347 which is also included here to ensure that CI is passing.
As discussed, I bumped the helm chart version to
2.26.5.I've also tested this helm chart version in a new 1.26 cluster and was unable to reproduce the bug fixed in #2332.
Please let me know if you'd like me to combine (i.e. squash/fixup) both commits in here into a single, correct, one.
Release note: