-
Notifications
You must be signed in to change notification settings - Fork 51
Replaced the webhook readiness init container logic to use curl #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…modern TLS support, avoiding the BusyBox wget handshake failure during certificate patch jobs.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes replace the webhook readiness check mechanism from wget-based health probing to curl-based health probing. This involves updating the Helm template to use curl instead of wget, changing the init container image from busybox to a dedicated curl image, and documenting the new configuration option. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @chrislusf, 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 enhances the stability of the webhook component by replacing the 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request replaces the wget command with curl in the webhook readiness init container to resolve TLS handshake failures. The changes are well-contained, updating the Helm template, default values, and documentation. My review includes a suggestion to make the new curl command more robust and efficient.
deploy/helm/templates/_helpers.tpl
Outdated
| - name: wait-for-webhook | ||
| image: {{ .Values.webhook.initContainer.image }} | ||
| command: ['sh', '-c', 'set -e; until wget -q --spider --timeout=5 --no-check-certificate https://{{ include "seaweedfs-operator.fullname" . }}-webhook.{{ .Release.Namespace }}.svc:443{{ .webhookPath }}; do echo waiting for webhook; sleep 1; done;'] | ||
| command: ['sh', '-c', 'set -e; until curl -sk --fail --connect-timeout 5 https://{{ include "seaweedfs-operator.fullname" . }}-webhook.{{ .Release.Namespace }}.svc:443{{ .webhookPath }} >/dev/null; do echo waiting for webhook; sleep 1; done;'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl command uses --connect-timeout, which only limits the time for establishing a connection. If the connection is made but the server is slow to respond, the command could still hang. It's better to use --max-time to limit the total duration of the operation, which is more equivalent to wget's --timeout behavior.
Additionally, since we are only checking for service readiness and not interested in the content, using the --head flag to only fetch HTTP headers would be more efficient, similar to wget's --spider option.
I suggest updating the command to use --max-time and --head for better robustness and efficiency.
command: ['sh', '-c', 'set -e; until curl -sk --fail --head --max-time 5 https://{{ include "seaweedfs-operator.fullname" . }}-webhook.{{ .Release.Namespace }}.svc:443{{ .webhookPath }} >/dev/null; do echo waiting for webhook; sleep 1; done;']
Fix #159
Replaced the webhook readiness init container logic to use curl with modern TLS support, avoiding the BusyBox wget handshake failure during certificate patch jobs.
Summary by CodeRabbit
Release Notes
Documentation
Chores