Recreate etcd pods and certificates on update #675
Conversation
dosubot
bot
added
the
size:L
WalkthroughThis pull request updates the etcd chart version from 2.6.0 to 2.6.1 and introduces several Helm hook templates. New Kubernetes resources, including a Job, Role, RoleBinding, and ServiceAccount, are added to facilitate post-upgrade operations. Additionally, the versions mapping has been modified to assign a specific commit hash for version 2.6.0 and to add an entry for version 2.6.1. Changes
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/extra/etcd/templates/hook/job.yaml (2)
29-50: Post-upgrade Job Metadata and Command SetupThe post-upgrade job’s metadata, annotations, and general structure are properly set up. However, YAMLlint reports an indentation issue at line 47 where the command list item is indented with 10 spaces instead of the expected 12.
Recommendation: Adjust the indentation for the list element under the
command:key on line 47 to comply with YAML best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 47-47: wrong indentation: expected 12 but found 10
(indentation)
47-47: YAML Formatting: Trailing Whitespace and IndentationStatic analysis tools have flagged several formatting issues:
- Indentation: Line 47 is indented with 10 spaces instead of the expected 12.
- Trailing Whitespace: Lines 55, 59, 65, 74, and 78 contain trailing spaces.
Recommendation: Remove trailing whitespace and correct the indentation on line 47 using an automated formatter or text editor to ensure compliance with YAML standards.
Also applies to: 55-55, 59-59, 65-65, 74-74, 78-78
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 47-47: wrong indentation: expected 12 but found 10
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extra/etcd/Chart.yaml(1 hunks)packages/extra/etcd/templates/hook/job.yaml(1 hunks)packages/extra/etcd/templates/hook/role.yaml(1 hunks)packages/extra/etcd/templates/hook/rolebinding.yaml(1 hunks)packages/extra/etcd/templates/hook/serviceaccount.yaml(1 hunks)packages/extra/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extra/etcd/Chart.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/etcd/templates/hook/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/etcd/templates/hook/role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/etcd/templates/hook/rolebinding.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/etcd/templates/hook/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 47-47: wrong indentation: expected 12 but found 10
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
packages/extra/etcd/templates/hook/serviceaccount.yaml (1)
1-9: ServiceAccount for Helm hooks looks goodThis ServiceAccount will be created only when upgrading to chart version 2.6.1 or higher. The annotations properly configure it as both pre-upgrade and post-upgrade hook, with appropriate cleanup policies.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/etcd/templates/hook/rolebinding.yaml (1)
1-17: RoleBinding configuration is appropriateThe RoleBinding correctly associates the ServiceAccount with the Role of the same name. The hook annotations match those on the other resources, ensuring consistent lifecycle management during the upgrade process.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/versions_map (1)
10-11: Version pinning and update looks correctVersion 2.6.0 is now correctly pinned to a specific commit hash, and version 2.6.1 is set as HEAD. This aligns with the PR objective of addressing issues with the 2.6.0 upgrade by creating a new 2.6.1 version.
packages/extra/etcd/templates/hook/role.yaml (1)
1-31: Role permissions are appropriate for certificate and pod managementThe Role grants the necessary permissions for managing secrets (certificates), pods, and configmaps during the upgrade process. This matches the PR objective of automating certificate renewal and etcd pod recreation.
The permissions follow the principle of least privilege by only including the specific resources and verbs needed for the upgrade process.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/etcd/templates/hook/job.yaml (1)
1-27: Review of Pre-upgrade Job DefinitionThe pre-upgrade job correctly uses a Helm template condition with
semverCompare ">= 2.6.1"to ensure it only runs for the intended chart versions. The command at line 24 retrieves the current etcd chart version and updates theetcd-previous-versionConfigMap, which is in line with the automation goals for certificate renewal.Note: YAMLlint’s error on line 1 (expected node content but found '-') might be a false positive due to the Helm templating syntax. If this becomes a recurring nuisance, consider configuring YAMLlint to accommodate Helm templates.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| args: | ||
| - -c | ||
| - |- | ||
| version=$(kubectl -n tenant-lllamnyp get configmap etcd-previous-version -ojsonpath='{.data.version}') |
| @@ -0,0 +1,84 @@ | |||
| {{- if .Chart.Version | semverCompare ">= 2.6.1" }} | |||
There was a problem hiding this comment.
This check makes no sense, the .Chart.Version will always be more or equal 2.6.1
There was a problem hiding this comment.
You're right, this was helpful during testing, but is redundant in the release.
| serviceAccountName: etcd-hook | ||
| containers: | ||
| - name: helm | ||
| image: alpine/k8s:1.31.6 |
There was a problem hiding this comment.
Please use bitnami/kubectl:latest as we already consume it from in other hooks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/extra/etcd/templates/hook/job.yaml (4)
1-1: Helm Template Directive & YAMLlint False Positive
The helm template directive on line 1 ({{- if .Chart.Version | semverCompare ">= 2.6.1" }}) is standard for Helm charts. Note that YAMLlint may flag this as a syntax error due to the templating delimiters. Ensure that your YAML linter is configured to ignore helm template syntax.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
46-47: Indentation in the Command List
YAMLlint reports an indentation issue on line 47. Since thecommand:key’s list items should be indented two spaces further than the key, please update the indentation. For example:- command: - - bash + command: + - bashThis ensures proper YAML list formatting without ambiguity.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 47-47: wrong indentation: expected 12 but found 10
(indentation)
51-76: Post-upgrade Job Script: Semver Function & Version Validation
The inline shell script retrieves the previous version from the ConfigMap and defines asemver_less_thanfunction to compare versions. The implementation appears sound. However, consider the scenario where the retrieved version is"unknown"(the default when no deployed version is found). If"unknown"is passed to arithmetic operations in the function, it may result in unexpected errors. You might want to add validation for a non-numeric (or unparseable) version string before performing comparisons.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
55-55: Remove Trailing Whitespace
YAMLlint has flagged trailing spaces on several lines (specifically lines 55, 59, 65, 74, and 78). Please remove these extra spaces to adhere to YAML style guidelines.Also applies to: 59-59, 65-65, 74-74, 78-78
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 55-55: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extra/etcd/templates/hook/job.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/etcd/templates/hook/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 47-47: wrong indentation: expected 12 but found 10
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
packages/extra/etcd/templates/hook/job.yaml (4)
2-28: ETCD Pre-upgrade Job: Version Capture & ConfigMap Creation
The pre-upgrade job correctly retrieves the deployed chart version using a helm history command, processes it with jq, and updates theetcd-previous-versionConfigMap. Verify that the image (alpine/k8s:1.31.6) includes all required tools (e.g., bash, jq, sed) for this command to work reliably.
18-18: Container Image Selection – Pre-upgrade Job
The container image is set toalpine/k8s:1.31.6, but previous reviews have suggested usingbitnami/kubectl:latest(as used in other hooks) for consistency and possibly better support. Consider changing the image as follows:- image: alpine/k8s:1.31.6 + image: bitnami/kubectl:latest
29-36: ETCD Post-upgrade Job: Basic Structure
The post-upgrade job is set up correctly with the necessary annotations and service account. Ensure that the namespace is consistently templated via{{ .Release.Namespace }}as seen throughout the file.
45-45: Container Image Selection – Post-upgrade Job
The image in the post-upgrade job is also defined asalpine/k8s:1.31.6. For consistency with other hooks (and as recommended by previous comments), consider switching to:- image: alpine/k8s:1.31.6 + image: bitnami/kubectl:latest
Since updating from 2.5.0 to 2.6.0 renews all certificates including the trusted CAs for etcd, all etcd pods need a restart. As many people had problems with their etcds post-update, we decided to script the procedure, so the renewal of certs and pods is done automatically and in a predictable order. The hook fires when upgrading <2.6.1 to >=2.6.1.
lllamnyp
force-pushed
the
bug/etcd-migration-hook
branch
from
fc14167 to
8b88fb6
Compare
dosubot
bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extra/etcd/templates/hook/job.yaml (1)
10-39: Kubernetes Job Definition and Command ConsistencyThe Job resource is well-defined with appropriate annotations for a post-upgrade hook, and it reliably uses
{{ .Release.Namespace }}in all kubectl commands to ensure dynamic namespace resolution instead of a hardcoded value. One minor issue noted by YAMLlint is a trailing space on line 34. Please remove any trailing spaces to adhere to formatting standards.Below is a suggested diff for line 34 removal:
- kubectl --namespace={{ .Release.Namespace }} delete secrets etcd-ca-tls etcd-peer-ca-tls + kubectl --namespace={{ .Release.Namespace }} delete secrets etcd-ca-tls etcd-peer-ca-tls🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 30-30: wrong indentation: expected 12 but found 10
(indentation)
[error] 34-34: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/extra/etcd/Chart.yaml(1 hunks)packages/extra/etcd/templates/hook/job.yaml(1 hunks)packages/extra/etcd/templates/hook/role.yaml(1 hunks)packages/extra/etcd/templates/hook/rolebinding.yaml(1 hunks)packages/extra/etcd/templates/hook/serviceaccount.yaml(1 hunks)packages/extra/etcd/templates/version.yaml(1 hunks)packages/extra/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extra/etcd/templates/version.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/extra/etcd/templates/hook/serviceaccount.yaml
- packages/extra/etcd/templates/hook/role.yaml
- packages/extra/etcd/templates/hook/rolebinding.yaml
- packages/extra/etcd/Chart.yaml
- packages/extra/versions_map
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/etcd/templates/hook/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 5-5: wrong indentation: expected 0 but found 2
(indentation)
[warning] 6-6: wrong indentation: expected 0 but found 4
(indentation)
[warning] 7-7: wrong indentation: expected 0 but found 2
(indentation)
[warning] 30-30: wrong indentation: expected 12 but found 10
(indentation)
[error] 34-34: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
packages/extra/etcd/templates/hook/job.yaml (1)
1-9: Template Logic for Certificate Update FlagThe block correctly retrieves the
etcd-deployed-versionConfigMap and sets the$shouldUpdateCertsflag based on whether the deployed version satisfies the minimum requirement (i.e.< 2.6.1). This ensures the hook job is only triggered if certificate renewal is necessary.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 5-5: wrong indentation: expected 0 but found 2
(indentation)
[warning] 6-6: wrong indentation: expected 0 but found 4
(indentation)
[warning] 7-7: wrong indentation: expected 0 but found 2
(indentation)
2 participants
Since updating from 2.5.0 to 2.6.0 renews all certificates including the trusted CAs for etcd, all etcd pods need a restart. As many people had problems with their etcds post-update, we decided to script the procedure, so the renewal of certs and pods is done automatically and in a predictable order. The hook fires when upgrading from <2.6.0 to >=2.6.1.
Summary by CodeRabbit