Skip to content

Fix panic when webhook specifies a rendering template via ConfigMap#305

Open
yuguorui wants to merge 2 commits intoAliyunContainerService:masterfrom
yuguorui:master
Open

Fix panic when webhook specifies a rendering template via ConfigMap#305
yuguorui wants to merge 2 commits intoAliyunContainerService:masterfrom
yuguorui:master

Conversation

@yuguorui
Copy link
Copy Markdown

What type of PR is this?
PR类型是什么?
/kind bug

What this PR does / why we need it:
这个PR解决了什么问题:
Fix panic when webhook sinks specifies a rendering template via ConfigMap

Which issue(s) this PR fixes:
PR相关联issue:

Fixes #

Does this PR introduce a breaking change?:
PR带来的破坏性变更:
NONE

Test/Final result:
测试/最终运行结果:

Copilot AI review requested due to automatic review settings October 27, 2025 17:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a panic that occurs when webhook sinks specify a rendering template via ConfigMap by adding nil checks for the URI parameter in the Kubernetes client configuration functions.

Key Changes:

  • Added nil check for URI parameter before accessing its properties to prevent panic
  • Modified client singleton retrieval logic to handle nil URI cases
  • Fixed minor formatting issue in Dockerfile

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
deploy/Dockerfile Added missing -y flag to apt-get install command for consistency
common/kubernetes/configs.go Added nil checks for URI parameter to prevent panic when webhook uses ConfigMap templates
Comments suppressed due to low confidence (3)

common/kubernetes/configs.go:96

  • The code accesses configOverrides.ClusterInfo.Server without checking if configOverrides is nil. When uri is nil, configOverrides remains nil (as initialized on line 71), which will cause a panic. Add a nil check for configOverrides before accessing its fields.
		if configOverrides.ClusterInfo.Server != "" {

common/kubernetes/configs.go:101

  • These lines access configOverrides fields without a nil check. When uri is nil, configOverrides will be nil, causing a panic. Wrap these accesses in a nil check for configOverrides.
		kubeConfig.Insecure = configOverrides.ClusterInfo.InsecureSkipTLSVerify
		if configOverrides.ClusterInfo.InsecureSkipTLSVerify {

common/kubernetes/configs.go:1

  • The code accesses configOverrides.ClusterInfo fields without checking if configOverrides is nil. When uri is nil, this will cause a panic. Add appropriate nil checks or provide default values when configOverrides is nil.
// Copyright 2014 Google Inc. All Rights Reserved.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread common/kubernetes/configs.go
ruogui.ygr added 2 commits October 28, 2025 01:15
NewWebHookSink() will try to retrieve the k8s client with
kubernetes.GetKubernetesClient(nil), which cause a panic by having a nil
on k8s client.

Allow kubernetes.GetKubernetesClient(nil) to fallback to in-cluster
config instead of return nil to the caller.

Signed-off-by: ruogui.ygr <ruogui.ygr@alibaba-inc.com>
Signed-off-by: ruogui.ygr <ruogui.ygr@alibaba-inc.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@yshngg
Copy link
Copy Markdown
Contributor

yshngg commented Oct 28, 2025

Hi @yuguorui , thanks for your contribution!

Could you provide detailed information on how to reproduce this panic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants