fix(helm): use | as sed delimiter for cert substitution#7322
fix(helm): use | as sed delimiter for cert substitution#7322rajsinghtech wants to merge 1 commit intokarmada-io:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Helm chart's pre-installation and pre-upgrade processes. It resolves an issue where Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Helm hook Jobs that substitute certificate placeholders (e.g., {{ ca_crt }}) with base64-encoded certificate data, which can contain / and break sed substitutions when / is used as the delimiter.
Changes:
- Updated
sedsubstitutions in the pre-install Job to use|as the delimiter for all cert placeholder replacements. - Updated
sedsubstitutions in the pre-upgrade Job to use|as the delimiter when patching existing ConfigMaps.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| charts/karmada/templates/pre-install-job.yaml | Switches all cert-related sed substitutions to use ` |
| charts/karmada/templates/pre-upgrade-job.yaml | Switches ca_crt substitution to use ` |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l and pre-upgrade jobs
The sed commands that replace {{ ca_crt }} and other cert placeholders use /
as the delimiter. Base64-encoded certificate values can contain / characters,
which breaks the sed substitution and leaves raw {{ ca_crt }} placeholders in
the static-resources ConfigMap. This causes the karmada-static-resource Job to
fail with a YAML parsing error when applying the APIService resources.
Switch all sed commands in pre-install-job.yaml and pre-upgrade-job.yaml to use
| as the delimiter instead, since | cannot appear in base64-encoded output.
Fixes karmada-io#7321
Signed-off-by: Raj Singh <rajsinghcpre@gmail.com>
Signed-off-by: Raj Singh <raj@tailscale.com>
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug in the Helm chart's pre-install and pre-upgrade jobs. The change correctly updates the sed delimiter from / to | when substituting base64-encoded certificate values. This resolves the issue where the original delimiter could conflict with characters in the base64 strings, leading to failed substitutions and incorrect YAML parsing. The chosen delimiter | is a suitable replacement as it is not present in base64 encoding, ensuring the reliability of certificate provisioning.
da3037c to
d6ec1ca
Compare
|
Thank you for the quick fix. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7322 +/- ##
=======================================
Coverage 42.02% 42.03%
=======================================
Files 874 874
Lines 53551 53551
=======================================
+ Hits 22505 22508 +3
+ Misses 29354 29351 -3
Partials 1692 1692
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @rajsinghtech.
I'm fine with replacing However, I wonder if |
|
Closing — can't confirm the root cause and the maintainers couldn't reproduce. The |
What type of PR is this?
/kind bug
What this PR does / why we need it
The
sedcommands in the pre-install and pre-upgrade Jobs that substitute{{ ca_crt }}(and other cert placeholders) with actual base64-encoded certificate values use/as the delimiter. Since base64-encoded values can contain/characters, this breaks thesedsubstitution pattern, leaving raw{{ ca_crt }}placeholders in thekarmada-static-resourcesConfigMap.When the
karmada-static-resourceJob subsequently runskubectl apply -f /static-resources/, it fails with:This is because
{{ ca_crt }}is interpreted as a YAML flow mapping rather than a string value.How this PR fixes it
Switches all
sedcommands inpre-install-job.yamlandpre-upgrade-job.yamlfrom using/to|as the delimiter. The|character cannot appear in base64-encoded output (A-Za-z0-9+/=), making it a safe delimiter choice.Which issue(s) this PR fixes
Fixes #7321
Does this PR introduce a user-facing change?