Skip to content
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.

Sign up for GitHub

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

add webhook port checker to e2e tests #268

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add webhook port checker to e2e tests #268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions test/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,18 @@ var _ = Describe("etcd-operator", Ordered, func() {
_, err = utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
})

By("wait until etcd-operator webhook is ready", func() {
cmd := exec.Command("kubectl", "apply", "-f", "test/e2e/webhook-checker.yaml")
_, err = utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred())

cmd = exec.Command("kubectl", "wait", "--namespace",
"etcd-operator-system", "pod/webhook-checker",
"--for", "jsonpath={.status.conditions[?(@.type==\"Ready\")].status}=True", "--timeout=5m")
_, err = utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
})
Comment on lines +65 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Webhook-checker.yaml file is missing or inaccessible.

The webhook-checker.yaml referenced in test/e2e/e2e_test.go does not exist or cannot be accessed based on the current context. This will cause the webhook readiness check to fail during the tests.

Recommendations:

  • Add the webhook-checker.yaml file to the test/e2e/ directory with the appropriate configuration for the webhook checker.

  • Verify the file path in the test to ensure it correctly points to the location of webhook-checker.yaml.

  • Update the shell script to accurately display the contents of the file once it is in place to facilitate future reviews.

🔗 Analysis chain

Approve the addition of webhook readiness check with suggestions for improvement.

The new step to check the readiness of the etcd-operator webhook is a valuable addition to the e2e tests, aligning well with the PR objectives. However, there are a few points to consider for improvement:

  1. The webhook-checker.yaml file is not provided in this context. Please include its contents for review to ensure it's correctly configured.

  2. Consider adding a cleanup step for the webhook-checker pod after the tests are complete. This can be done in the AfterAll function to ensure proper resource management.

  3. The error handling is minimal. Consider adding more detailed error logging to help diagnose issues if the webhook fails to become ready. For example:

if err != nil {
    fmt.Printf("Error waiting for webhook-checker pod: %v\n", err)
    ExpectWithOffset(1, err).NotTo(HaveOccurred())
}
  1. The timeout of 5 minutes seems reasonable, but you might want to make it configurable via an environment variable to allow for flexibility in different testing environments.

To verify the existence and contents of the webhook-checker.yaml file, please run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
cat test/e2e/webhook-checker.yaml

Length of output: 3476

})

if os.Getenv("DO_CLEANUP_AFTER_E2E") == "true" {
Expand Down
20 changes: 20 additions & 0 deletions test/e2e/webhook-checker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
apiVersion: v1
kind: Pod
metadata:
name: webhook-checker
namespace: etcd-operator-system
spec:
containers:
- image: curlimages/curl:8.10.1
name: webhook-checker
command:
- sh
- -c
- "sleep 360"
readinessProbe:
exec:
command:
- sh
- -c
- "curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster"
periodSeconds: 5
Comment on lines +14 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding failureThreshold and successThreshold to the readiness probe.

The readiness probe checks every 5 seconds, which is a good interval. However, for more precise control over when the pod is considered ready or not ready, consider adding failureThreshold and successThreshold.

Add these fields to fine-tune the probe behavior:

      readinessProbe:
        exec:
          command:
            - sh
            - -c
            - "curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster"
        periodSeconds: 5
+       failureThreshold: 3
+       successThreshold: 1

This configuration will consider the pod not ready after 3 consecutive failures and ready after 1 success, providing a balance between responsiveness and stability.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
readinessProbe:
exec:
command:
- sh
- -c
- "curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster"
periodSeconds: 5
readinessProbe:
exec:
command:
- sh
- -c
- "curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster"
periodSeconds: 5
failureThreshold: 3
successThreshold: 1

⚠️ Potential issue

Address SSL certificate validation in the readiness probe.

The readiness probe is well-structured to check the webhook service availability. However, the use of the -k flag with curl ignores SSL certificate validation, which could be a security concern.

Consider one of the following options:

  1. If this is intentional due to self-signed certificates in the test environment, add a comment explaining this decision.
  2. If possible, use proper SSL certificates and remove the -k flag for better security practices.

Example of adding a comment:

        exec:
          command:
            - sh
            - -c
-            - "curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster"
+            - |
+              # Using -k flag due to self-signed certificates in the test environment
+              curl -k https://etcd-operator-webhook-service/mutate-etcd-aenix-io-v1alpha1-etcdcluster

Committable suggestion was skipped due to low confidence.