fix(upgrade): Handle Mutated Images #3506
Conversation
WalkthroughThe changes introduce a new variable Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
upgrade/upgrage_test.go (2)
11-22: Consider enhancing the helper function for better reusability.While the current implementation works for these tests, consider making it more robust for potential reuse:
- Add validation for input parameters
- Consider adding options for setting other Pod fields
- Add documentation explaining the purpose and limitations
Example enhancement:
+// newDummyPod creates a minimal Pod object for testing purposes. +// It only sets the container name and image, other Pod fields are left as defaults. func newDummyPod(name string, image string) *corev1.Pod { + if name == "" || image == "" { + return nil + } return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, Spec: corev1.PodSpec{ Containers: []corev1.Container{ {
25-66: Add test cases for invalid inputs.Consider adding test cases for:
- Empty image strings
- Malformed image references
- Missing version tags
Example additional test cases:
{ runningManagerImage: "", deployManagerImage: "longhornio/longhorn-manager:v1.8.0", isOld: false, expectError: true, }, { runningManagerImage: "longhornio/longhorn-manager", // missing tag deployManagerImage: "longhornio/longhorn-manager:v1.8.0", isOld: false, expectError: true, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
upgrade/upgrade.go(2 hunks)upgrade/upgrage_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
upgrade/upgrage_test.go (2)
1-9: LGTM! Clean and minimal imports.The package declaration and imports are appropriate for the test requirements.
24-67: Test cases effectively cover the mutation scenarios.The test cases comprehensively cover various image mutation patterns, including:
- Registry prefixed images
- Version matches and upgrades
- Non-semver scenarios
This aligns well with the PR objective of handling mutated images.
Khalid-Nowaf
force-pushed
the
9638-mutating_webhook_image_compatibility
branch
4 times, most recently
from
d75d9ce to
731735f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Signed-off-by: Khalid Nowaf <[email protected]>
Signed-off-by: Khalid Nowaf <[email protected]>
Signed-off-by: Khalid Nowaf <[email protected]>
Khalid-Nowaf
force-pushed
the
9638-mutating_webhook_image_compatibility
branch
from
731735f to
c5a808c
Compare
|
To test it, I have built a container |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9638
What this PR does / why we need it:
The upgrade has a bug that prevents it from supporting K8s webhook mutation (which is widely adaptable for security reasons), which could alter the image's name.
The new mutated image can not be known before deployment, so there is no way to configure the helm char to adapt it.
Longhorn is not deployable now using the helm in such cases. So we need this fix to make it compatible.
Special notes for your reviewer:
Additional documentation or context
https://github.com/zarf-dev/zarf/blob/main/adr/0005-mutating-webhook.md