Move source-ip validation from cilium to kube-ovn side - #687

packages/system/kubeovn-webhook/images/kubeovn-webhook/main.go (2)

28-47: Consider adding graceful shutdown and health endpoints.

The current server implementation lacks graceful shutdown handling and health/readiness endpoints, which are beneficial for Kubernetes deployments.

Here's a suggested implementation with context handling for graceful shutdown:

-	mux := http.NewServeMux()
-	mux.HandleFunc("/mutate-pods", HandleMutatePods)
-
-	tlsCert, err := tls.LoadX509KeyPair(tlsCertFile, tlsKeyFile)
-	if err != nil {
-		log.Fatalf("Failed to load key pair: %v", err)
-	}
-
-	server := &http.Server{
-		Addr: ":8443",
-		TLSConfig: &tls.Config{
-			Certificates: []tls.Certificate{tlsCert},
-		},
-		Handler: mux,
-	}
-
-	log.Printf("Starting webhook server on %s", server.Addr)
-	if err := server.ListenAndServeTLS("", ""); err != nil {
-		log.Fatalf("Failed to start server: %v", err)
-	}
+	mux := http.NewServeMux()
+	mux.HandleFunc("/mutate-pods", HandleMutatePods)
+	mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+	mux.HandleFunc("/ready", func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+
+	tlsCert, err := tls.LoadX509KeyPair(tlsCertFile, tlsKeyFile)
+	if err != nil {
+		log.Fatalf("Failed to load key pair: %v", err)
+	}
+
+	server := &http.Server{
+		Addr: ":8443",
+		TLSConfig: &tls.Config{
+			Certificates: []tls.Certificate{tlsCert},
+		},
+		Handler: mux,
+	}
+
+	// Handle graceful shutdown
+	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stop()
+
+	go func() {
+		<-ctx.Done()
+		log.Println("Shutting down server...")
+		
+		shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+		defer cancel()
+		
+		if err := server.Shutdown(shutdownCtx); err != nil {
+			log.Fatalf("Server shutdown failed: %v", err)
+		}
+	}()
+
+	log.Printf("Starting webhook server on %s", server.Addr)
+	if err := server.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed {
+		log.Fatalf("Failed to start server: %v", err)
+	}

Don't forget to add required imports:

import (
	"context"
	"crypto/tls"
	"flag"
	"log"
	"net/http"
	"os"
	"os/signal"
	"syscall"
	"time"
)

---

3-8: Consider adding timeouts to HTTP server.

The HTTP server configuration lacks timeout settings, which could lead to resource leaks or denial-of-service vulnerabilities.

 import (
 	"crypto/tls"
 	"flag"
 	"log"
 	"net/http"
+	"time"
 )

And update the server configuration:

 	server := &http.Server{
 		Addr: ":8443",
 		TLSConfig: &tls.Config{
 			Certificates: []tls.Certificate{tlsCert},
 		},
 		Handler: mux,
+		ReadTimeout:    10 * time.Second,
+		WriteTimeout:   10 * time.Second,
+		MaxHeaderBytes: 1 << 20, // 1 MB
 	}

packages/system/kubeovn-webhook/images/kubeovn-webhook/Dockerfile (1)

1-17: Multi-stage Dockerfile Implementation
This Dockerfile correctly implements a multi-stage build. The builder stage uses the golang:1.23 image to compile a statically linked binary using CGO_ENABLED=0 and GOOS=linux, and the runtime stage uses an alpine:3.21.3 image to keep the final image minimal.

Consider adding a non-root user in the runtime stage for enhanced container security in production environments.

packages/system/kubeovn-webhook/templates/mutatingwebhookconfiguration.yaml (1)

5-6: Resolving YAML Linter Syntax Issue
The static analysis tool flagged a potential YAML syntax issue on line 6 due to the Helm templating. This is a common false positive when using unquoted templated values. To improve YAML parsing reliability, consider quoting the value as shown below:

-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "namespace-annotation-webhook.fullname" . }}-cert
+    cert-manager.io/inject-ca-from: '{{ .Release.Namespace }}/{{ include "namespace-annotation-webhook.fullname" . }}-cert'

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 6-6: syntax error: expected , but found ''

(syntax)

packages/system/kubeovn-webhook/Makefile (1)

9-20: Docker Image Build Process
The build target uses Docker Buildx with provenance disabled and leverages caching features as well as metadata extraction via yq to update the values.yaml file. This strategy helps ensure that the Docker image is correctly tagged with its digest.

It may be beneficial to confirm that $(REGISTRY) and $(TAG) are properly defined in the environment and to add error handling in case the metadata file generation or digest extraction fails.

packages/system/kubeovn-webhook/images/kubeovn-webhook/admission.go (1)

7-7: Use modern I/O packages instead of io/ioutil.
"io/ioutil" is deprecated in Go ≥ 1.19. Switch to the io or os packages.

Proposed fix:

-import (
-       "io/ioutil"
-       ...
-)
+import (
+       "io"
+       ...
+)

...

- body, err := ioutil.ReadAll(r.Body)
+ body, err := io.ReadAll(r.Body)

Also applies to: 26-26

🧰 Tools

🪛 golangci-lint (1.62.2)

7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.

(staticcheck)

packages/system/kubeovn-webhook/images/kubeovn-webhook/go.mod (1)

12-47: Indirect Dependencies Management
The indirect dependency block appears to be automatically generated. It is good practice to periodically run go mod tidy to ensure these dependencies stay updated and consistent with your codebase.

packages/system/kubeovn-webhook/images/kubeovn-webhook/main.go (1)

23-23: ⚠️ Potential issue

Fix typo in flag name "port-securty".

There's a typo in the flag name for port security.

-	flag.BoolVar(&PortSecurityGlobal, "port-securty", true, "If false, skip adding port_security unless specified by the Namespace.")
+	flag.BoolVar(&PortSecurityGlobal, "port-security", true, "If false, skip adding port_security unless specified by the Namespace.")

📝 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.

	flag.BoolVar(&PortSecurityGlobal, "port-security", true, "If false, skip adding port_security unless specified by the Namespace.")

packages/system/kubeovn-webhook/templates/deployment.yaml (1)

49-49: ⚠️ Potential issue

Typo in Argument Flag
The CLI argument flag is written as --port-securty={{ .Values.portSecurity }}. This appears to be a typo; it should likely be --port-security={{ .Values.portSecurity }} to ensure that the flag is correctly parsed and applied.

packages/core/platform/bundles/paas-full.yaml (1)

53-59: New Release Entry for kubeovn-webhook Integrated.
The new release entry for kubeovn-webhook is defined with the proper attributes and dependencies. For improved readability—and to comply with YAML linting suggestions—consider adding spaces after commas in the inline list. For example:

-  dependsOn: [cilium,kubeovn,cert-manager]
+  dependsOn: [cilium, kubeovn, cert-manager]

🧰 Tools

🪛 YAMLlint (1.35.1)

[warning] 58-58: too few spaces after comma

(commas)

---

[warning] 58-58: too few spaces after comma

(commas)

packages/system/kubeovn-webhook/images/kubeovn-webhook/admission.go (4)

7-7: Replace deprecated io/ioutil package.

The io/ioutil package has been deprecated since Go 1.19. Use equivalent functions from the io or os packages instead.

-	"io/ioutil"
+	"io"

And then update the corresponding function call:

-	body, err := ioutil.ReadAll(r.Body)
+	body, err := io.ReadAll(r.Body)

🧰 Tools

🪛 golangci-lint (1.62.2)

7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.

(staticcheck)

---

103-107: Improve annotation change detection logic.

The current approach only compares the length of annotation maps, which might incorrectly identify changes if a key is replaced with another one of the same length. Consider implementing a deeper comparison of the annotation contents.

-	if len(mergedAnnotations) == len(pod.Annotations) {
+	hasChanges := false
+	if len(mergedAnnotations) != len(pod.Annotations) {
+		hasChanges = true
+	} else {
+		for k, v := range mergedAnnotations {
+			if origValue, exists := pod.Annotations[k]; !exists || origValue != v {
+				hasChanges = true
+				break
+			}
+		}
+	}
+
+	if !hasChanges {

---

146-146: Add timeout to the context for Kubernetes API calls.

Using context.Background() without a timeout could lead to hanging requests if the Kubernetes API server is unresponsive. Add a reasonable timeout to prevent this issue.

-	ns, err := clientset.CoreV1().Namespaces().Get(context.Background(), namespace, metav1.GetOptions{})
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	ns, err := clientset.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{})

Remember to add the missing import:

+	"time"

---

160-160: Handle potential write error.

The write operation error is being discarded with _, _. Consider logging this error as it might indicate connectivity issues with the client.

-	_, _ = w.Write(resp)
+	if _, err := w.Write(resp); err != nil {
+		log.Printf("Error writing response: %v", err)
+	}

packages/system/kubeovn-webhook/templates/deployment.yaml (3)

32-38: Consider increasing resource limits for production deployments.

The current resource limits (100m CPU, 128Mi memory) might be sufficient for testing but could be inadequate for production deployments in large clusters with high pod creation rates. Consider adjusting these based on observed usage patterns.

---

23-46: Add health probes for better container lifecycle management.

The deployment lacks liveness and readiness probes, which are important for Kubernetes to properly manage the container lifecycle. Consider adding health probes to ensure proper detection of container health status.

          volumeMounts:
            - name: webhook-certs
              mountPath: /etc/webhook/certs
              readOnly: true
+          livenessProbe:
+            httpGet:
+              path: /healthz
+              port: 8443
+              scheme: HTTPS
+            initialDelaySeconds: 15
+            periodSeconds: 20
+          readinessProbe:
+            httpGet:
+              path: /healthz
+              port: 8443
+              scheme: HTTPS
+            initialDelaySeconds: 5
+            periodSeconds: 10

Note: Make sure your webhook server implements a /healthz endpoint or adjust the path accordingly.

---

8-9: Define a rolling update strategy for controlled updates.

The deployment doesn't specify a rolling update strategy, so it will use the default. Consider defining a specific strategy to ensure minimal disruption during updates.

spec:
  replicas: 2
+  strategy:
+    type: RollingUpdate
+    rollingUpdate:
+      maxUnavailable: 0
+      maxSurge: 1
  selector:

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/system/kubeovn-webhook/templates/mutatingwebhookconfiguration.yaml (1)

6-6: ⚠️ Potential issue

Fix YAML syntax by quoting template expression.

The static analysis tool detected a YAML syntax error. To prevent parsing issues, enclose the templated value in quotes.

-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "namespace-annotation-webhook.fullname" . }}-cert
+    cert-manager.io/inject-ca-from: "{{ .Release.Namespace }}/{{ include \"namespace-annotation-webhook.fullname\" . }}-cert"

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 6-6: syntax error: expected , but found ''

(syntax)

packages/system/kubeovn-webhook/templates/certmanager.yaml (1)

4-4: ⚠️ Potential issue

Fix YAML syntax by quoting template expressions.

The static analysis tool detected a YAML syntax error. To prevent parsing issues, enclose the templated value in quotes.

-  name: {{ include "namespace-annotation-webhook.fullname" . }}-selfsigned-issuer
+  name: "{{ include \"namespace-annotation-webhook.fullname\" . }}-selfsigned-issuer"

Apply similar changes to all other template expressions in this file (lines 12, 15, 17, 19, 25, 29, 34, 41, 42, 44, 45).

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 4-4: syntax error: expected , but found ''

(syntax)

packages/system/kubeovn-webhook/images/kubeovn-webhook/admission.go (4)

7-7: Update deprecated import.

The io/ioutil package has been deprecated since Go 1.19. As noted by the static analysis tool, it should be replaced with either the io or os package.

-import (
-	"context"
-	"encoding/json"
-	"fmt"
-	"io/ioutil"
-	"log"
-	"net/http"
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"io"
+	"log"
+	"net/http"

Additionally, replace the function call on line 26:

-	body, err := ioutil.ReadAll(r.Body)
+	body, err := io.ReadAll(r.Body)

🧰 Tools

🪛 golangci-lint (1.62.2)

7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.

(staticcheck)

---

136-151: Optimize Kubernetes client initialization.

A new Kubernetes client is created for every request, which can be inefficient. Consider initializing the client once during startup and reusing it for subsequent requests.

Create a global client variable and initialize it once:

var (
    k8sClient *kubernetes.Clientset
)

func init() {
    config, err := rest.InClusterConfig()
    if err != nil {
        log.Fatalf("Failed to get in-cluster config: %v", err)
    }
    
    clientset, err := kubernetes.NewForConfig(config)
    if err != nil {
        log.Fatalf("Failed to create Kubernetes client: %v", err)
    }
    
    k8sClient = clientset
}

func getNamespaceAnnotations(namespace string) (map[string]string, error) {
    ns, err := k8sClient.CoreV1().Namespaces().Get(context.Background(), namespace, metav1.GetOptions{})
    if err != nil {
        return nil, err
    }
    return ns.Annotations, nil
}

---

47-60: Early return could hide errors.

The code returns early with admissionResponse.Allowed = true when an error occurs, which could lead to silent failures. Consider adding more detailed logging or rejecting the request when critical errors occur.

For example, unmarshal errors could indicate malformed requests that should be rejected rather than allowed:

 	if err := json.Unmarshal(raw, &pod); err != nil {
+		log.Printf("Failed to unmarshal pod object: %v", err)
 		admissionResponse.Allowed = true
 		writeResponse(w, admissionReviewResp)
 		return
 	}

---

160-161: Improve error handling in writeResponse.

The write operation ignores potential errors. Consider adding error logging for better debuggability.

-	_, _ = w.Write(resp)
+	if _, err := w.Write(resp); err != nil {
+		log.Printf("Error writing response: %v", err)
+	}

packages/core/platform/bundles/paas-full.yaml (1)

53-58: LGTM! Addition of the kubeovn-webhook component.

The kubeovn-webhook component is properly integrated into the release pipeline with appropriate dependencies.

Fix the spacing after commas in the dependsOn list for improved readability:

-  dependsOn: [cilium,kubeovn,cert-manager]
+  dependsOn: [cilium, kubeovn, cert-manager]

🧰 Tools

🪛 YAMLlint (1.35.1)

[warning] 58-58: too few spaces after comma

(commas)

---

[warning] 58-58: too few spaces after comma

(commas)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/system/kubeovn-webhook/templates/certmanager.yaml (1)

31-46: 🛠️ Refactor suggestion

Quote template expressions in TLS certificate configuration.

Ensure all template expressions in the certificate configuration are properly quoted.

-  name: {{ include "namespace-annotation-webhook.fullname" . }}-cert
+  name: "{{ include "namespace-annotation-webhook.fullname" . }}-cert"
-  secretName: {{ include "namespace-annotation-webhook.fullname" . }}-tls
+  secretName: "{{ include "namespace-annotation-webhook.fullname" . }}-tls"
-    name: {{ include "namespace-annotation-webhook.fullname" . }}-ca-issuer
+    name: "{{ include "namespace-annotation-webhook.fullname" . }}-ca-issuer"
-  commonName: {{ include "namespace-annotation-webhook.fullname" . }}-tls
+  commonName: "{{ include "namespace-annotation-webhook.fullname" . }}-tls"
-    - {{ include "namespace-annotation-webhook.fullname" . }}
-    - {{ include "namespace-annotation-webhook.fullname" . }}.{{ .Release.Namespace }}.svc
+    - "{{ include "namespace-annotation-webhook.fullname" . }}"
+    - "{{ include "namespace-annotation-webhook.fullname" . }}.{{ .Release.Namespace }}.svc"

packages/system/kubeovn-webhook/templates/mutatingwebhookconfiguration.yaml (1)

5-6: ⚠️ Potential issue

Fix potential YAML parsing error by quoting template expression.

The annotation value contains template expressions that need to be quoted to prevent YAML parsing errors.

-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "namespace-annotation-webhook.fullname" . }}-cert
+    cert-manager.io/inject-ca-from: "{{ .Release.Namespace }}/{{ include "namespace-annotation-webhook.fullname" . }}-cert"

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 6-6: syntax error: expected , but found ''

(syntax)

packages/system/kubeovn-webhook/templates/certmanager.yaml (3)

3-4: ⚠️ Potential issue

Fix YAML syntax error by quoting template expression.

The template expression in the name field needs to be quoted to prevent YAML parsing errors.

-  name: {{ include "namespace-annotation-webhook.fullname" . }}-selfsigned-issuer
+  name: "{{ include "namespace-annotation-webhook.fullname" . }}-selfsigned-issuer"

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 4-4: syntax error: expected , but found ''

(syntax)

---

23-30: 🛠️ Refactor suggestion

Quote template expressions in CA issuer configuration.

Template expressions should be quoted to ensure proper YAML parsing.

-  name: {{ include "namespace-annotation-webhook.fullname" . }}-ca-issuer
+  name: "{{ include "namespace-annotation-webhook.fullname" . }}-ca-issuer"
-    secretName: {{ include "namespace-annotation-webhook.fullname" . }}-root-ca
+    secretName: "{{ include "namespace-annotation-webhook.fullname" . }}-root-ca"

---

11-21: 🛠️ Refactor suggestion

Quote all template expressions in certificate configuration.

Similar to the previous issue, all template expressions in the certificate configuration should be quoted.

-  name: {{ include "namespace-annotation-webhook.fullname" . }}-root-ca
+  name: "{{ include "namespace-annotation-webhook.fullname" . }}-root-ca"
-  secretName: {{ include "namespace-annotation-webhook.fullname" . }}-root-ca
+  secretName: "{{ include "namespace-annotation-webhook.fullname" . }}-root-ca"
-  commonName: {{ include "namespace-annotation-webhook.fullname" . }}-root-ca
+  commonName: "{{ include "namespace-annotation-webhook.fullname" . }}-root-ca"
-    name: {{ include "namespace-annotation-webhook.fullname" . }}-selfsigned-issuer
+    name: "{{ include "namespace-annotation-webhook.fullname" . }}-selfsigned-issuer"

packages/core/platform/bundles/paas-full.yaml (1)

53-58: Integration looks good, but fix spacing in dependsOn array.

The kubeovn-webhook is properly integrated into the platform bundle with correct namespace and dependencies.

-  dependsOn: [cilium,kubeovn,cert-manager]
+  dependsOn: [cilium, kubeovn, cert-manager]

🧰 Tools

🪛 YAMLlint (1.35.1)

[warning] 58-58: too few spaces after comma

(commas)

---

[warning] 58-58: too few spaces after comma

(commas)

packages/system/kubeovn-webhook/images/kubeovn-webhook/admission.go (1)

136-151: Consider caching namespace annotations.

The webhook creates a new Kubernetes client and fetches namespace information for every single pod creation. This could impact performance at scale. Consider implementing a caching mechanism for namespace annotations to reduce API server load.

// Add a simple time-based cache
type namespaceCache struct {
    annotations map[string]map[string]string
    expiry      map[string]time.Time
    mu          sync.RWMutex
}

var nsCache = &namespaceCache{
    annotations: make(map[string]map[string]string),
    expiry:      make(map[string]time.Time),
}

func (c *namespaceCache) get(namespace string) (map[string]string, bool) {
    c.mu.RLock()
    defer c.mu.RUnlock()
    
    annotations, exists := c.annotations[namespace]
    if !exists {
        return nil, false
    }
    
    expiry, _ := c.expiry[namespace]
    if time.Now().After(expiry) {
        return nil, false
    }
    
    return annotations, true
}

func (c *namespaceCache) set(namespace string, annotations map[string]string) {
    c.mu.Lock()
    defer c.mu.Unlock()
    
    c.annotations[namespace] = annotations
    c.expiry[namespace] = time.Now().Add(5 * time.Minute) // Cache for 5 minutes
}

func getNamespaceAnnotations(namespace string) (map[string]string, error) {
    // Check cache first
    if annotations, found := nsCache.get(namespace); found {
        return annotations, nil
    }

    // If not in cache, proceed with API call
    config, err := rest.InClusterConfig()
    if err != nil {
        return nil, err
    }
    clientset, err := kubernetes.NewForConfig(config)
    if err != nil {
        return nil, err
    }

    ns, err := clientset.CoreV1().Namespaces().Get(context.Background(), namespace, metav1.GetOptions{})
    if err != nil {
        return nil, err
    }
    
    // Update cache
    nsCache.set(namespace, ns.Annotations)
    
    return ns.Annotations, nil
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/system/kubeovn-webhook/templates/certmanager.yaml (5)

1-7: Ensure templating consistency in metadata.

In this first document (lines 1–7), verify that all template expressions within metadata (including namespace values) are properly quoted when needed. Consistent quoting helps avoid subtle YAML linting issues in CI/CD pipelines.

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 4-4: syntax error: expected , but found ''

(syntax)

---

9-20: Quote template values in Certificate metadata fields.

This Certificate resource (lines 9–20) uses template expressions in fields such as the resource name (line 12) and issuer reference (line 19). To ensure reliable YAML parsing, review and consider quoting these expressions as needed.

---

24-26: Wrap template output in quotes for CA Issuer.

In the CA Issuer definition (lines 22–30), the metadata name on line 25 is generated with a template expression. Please consider quoting it to prevent YAML parsing errors.

---

33-45: Apply quoting to templated values in Certificate resource.

Within this Certificate (lines 31–45), multiple metadata fields—such as the name (line 34), secret references, commonName, and dnsNames list items—use template expressions. Enclosing these in quotes can help avoid YAML linting problems and improve consistency.

---

16-16: Standardize inline comment language.

The inline comment on line 16 uses non-English text ("5 лет"). For consistency with the rest of the configuration (and to aid international collaborators), consider using English (e.g., "5 years").

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro