Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
6 changes: 3 additions & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ jobs:
build:
strategy:
matrix:
go-version: [1.25.x]
go-version: [1.26.x]
Comment thread
coderabbitai[bot] marked this conversation as resolved.
os: [ubuntu-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Install Go
uses: actions/setup-go@v3
uses: actions/setup-go@be3c94b385c4f180051c996d336f57a34c397495 # v3
with:
go-version: ${{ matrix.go-version }}

- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@a37ce9120846195fa4ece8f58b268e6043cb2f26 # v3

- name: Build
run: ./hack/build.sh
12 changes: 6 additions & 6 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@ jobs:
test:
strategy:
matrix:
go-version: [1.25.x]
go-version: [1.26.x]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

rg -n 'matrix\.(go|go-version)' .github/workflows/test.yml

Repository: openshift/multus-admission-controller

Length of output: 265


🏁 Script executed:

sed -n '1,80p' .github/workflows/test.yml

Repository: openshift/multus-admission-controller

Length of output: 1250


Use matrix.go-version in the coverage flag name. The workflow matrix defines go-version, but this step reads matrix.go, so the flag becomes empty and can collide across versions.

Proposed fix
-        flag-name: Go-${{ matrix.go }}
+        flag-name: Go-${{ matrix.go-version }}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/test.yml at line 7, Update the coverage flag configuration
in the workflow step to reference the matrix.go-version value defined by the
go-version matrix, replacing the incorrect matrix.go reference and preserving
distinct coverage flags for each Go version.

os: [ubuntu-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Install Go
uses: actions/setup-go@v3
uses: actions/setup-go@be3c94b385c4f180051c996d336f57a34c397495 # v3
with:
go-version: ${{ matrix.go-version }}

- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@a37ce9120846195fa4ece8f58b268e6043cb2f26 # v3

- name: Run Revive Action by pulling pre-built image
uses: docker://morphy/revive-action:v2
uses: docker://morphy/revive-action@sha256:1ec5904abc4d8b2cd5fbb2987fc79a227f21a4231d757cc9a09a9ee3ab3c328b # v2
with:
exclude: "./vendor/..."

Expand All @@ -32,7 +32,7 @@ jobs:
run: sudo ./hack/test.sh

- name: Send coverage
uses: shogo82148/actions-goveralls@v1
uses: shogo82148/actions-goveralls@8781f5dd05b691c4dd042d5e859c11c73e0104fa # v1
with:
path-to-profile: coverage.out
flag-name: Go-${{ matrix.go }}
Expand All @@ -43,6 +43,6 @@ jobs:
needs: test
runs-on: ubuntu-latest
steps:
- uses: shogo82148/actions-goveralls@v1
- uses: shogo82148/actions-goveralls@8781f5dd05b691c4dd042d5e859c11c73e0104fa # v1
with:
parallel-finished: true
2 changes: 1 addition & 1 deletion build/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM golang:1.25
FROM golang:1.26

LABEL org.opencontainers.image.source=https://github.com/k8snetworkplumbingwg/net-attach-def-admission-controller
ADD . /usr/src/net-attach-def-admission-controller
Expand Down
40 changes: 21 additions & 19 deletions cmd/webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,12 @@ package main

import (
"context"
"crypto/sha512"
"crypto/tls"
"encoding/hex"
"errors"
"flag"
"fmt"
"io/ioutil"
"net/http"
"os"
"path/filepath"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -98,6 +94,13 @@ func main() {

glog.Infof("starting net-attach-def-admission-controller webhook server")

// Capture the certificate baseline immediately before loading the key pair.
// Any replacement after this point will be detected by the watcher below.
previousCertInfo, err := os.Stat(*cert)
if err != nil {
glog.Fatalf("failed to stat certificate file %s: %v", *cert, err)
}

keyPair, err := webhook.NewTLSKeypairReloader(*cert, *key)
if err != nil {
glog.Fatalf("error load certificate: %s", err.Error())
Expand Down Expand Up @@ -130,34 +133,33 @@ func main() {
// Start watching for pod creations
go controller.StartWatching(ignoreNamespaces)

// watch the cert file and restart http sever if the file updated.
oldHashVal := ""
// Watch certificate metadata and reload the key pair when the certificate is
// updated. Avoid reading the file contents here: NewTLSKeypairReloader is the
// only component that needs access to the certificate and private key data.
for {
hasher := sha512.New()
certPath, err := filepath.Abs(*cert)
if err != nil {
glog.Fatalf("illegal path %s in certPath: %s: %v", *cert, certPath, err)
os.Exit(1)
}
s, err := ioutil.ReadFile(certPath)
hasher.Write(s)
currentCertInfo, err := os.Stat(*cert)
if err != nil {
glog.Fatalf("failed to read file %s: %v", *cert, err)
os.Exit(1)
glog.Fatalf("failed to stat certificate file %s: %v", *cert, err)
}
newHashVal := hex.EncodeToString(hasher.Sum(nil))
if oldHashVal != "" && newHashVal != oldHashVal {

if certificateFileChanged(previousCertInfo, currentCertInfo) {
if err := proc.Signal(syscall.SIGHUP); err != nil {
glog.Fatalf("failed to send certificate update notification: %v", err)
}
}
oldHashVal = newHashVal
previousCertInfo = currentCertInfo

time.Sleep(1 * time.Second)
}

}

func certificateFileChanged(previous, current os.FileInfo) bool {
return !os.SameFile(previous, current) ||
previous.Size() != current.Size() ||
!previous.ModTime().Equal(current.ModTime())
}
Comment on lines +157 to +161

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Metadata equality does not prove certificate equality.

An in-place rewrite can retain the inode and size, and can have the same modtime on coarse-resolution filesystems or when timestamps are preserved. This returns false and skips reloading changed TLS material. Retain a content fingerprint for polling, or use filesystem change notifications; add a same-size, preserved-modtime regression case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/webhook/main.go` around lines 151 - 155, Update certificateFileChanged to
detect in-place certificate content changes that preserve inode, size, and
modification time by comparing a retained content fingerprint (or equivalent
file-content state) during polling. Ensure the polling flow stores and updates
that fingerprint when the certificate is read, and add a regression case
covering same-size content with preserved modification time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the odds of hitting this in a prod env are so low that we can ignore this, if we ever hit this edge case we can re-eval

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bpickard22 Understood. The edge case is valid but sufficiently low-risk to defer; we can revisit it if it occurs in production.


func startHTTPServers(config *ServerConfig) (func(), error) {
// Parse TLS configuration
tlsCipherSuiteIDs, err := cliflag.TLSCipherSuites(config.TLSCipherSuites)
Expand Down
65 changes: 65 additions & 0 deletions cmd/webhook/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"errors"
"fmt"
"math/big"
"net"
Expand All @@ -38,6 +39,7 @@ import (

var (
_ = Describe("StringSliceFlag", testStringSliceFlag)
_ = Describe("Certificate file change detection", testCertificateFileChanged)
_ = Describe("HTTP Servers", testHTTPServers)
)

Expand Down Expand Up @@ -74,6 +76,69 @@ func testStringSliceFlag() {
)
}

func testCertificateFileChanged() {
var (
certPath string
cleanup func()
)

BeforeEach(func() {
certFile, err := os.CreateTemp("", "certificate-change-test-*.pem")
Expect(err).NotTo(HaveOccurred())
certPath = certFile.Name()
Expect(certFile.Close()).To(Succeed())
cleanup = func() {
Expect(os.Remove(certPath)).To(Succeed())
}
})

AfterEach(func() {
cleanup()
})

It("does not report an unchanged file", func() {
previous, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())
current, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())

Expect(certificateFileChanged(previous, current)).To(BeFalse())
})

It("detects an in-place update", func() {
previous, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())
Expect(os.WriteFile(certPath, []byte("updated certificate"), 0o600)).To(Succeed())
current, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())

Expect(certificateFileChanged(previous, current)).To(BeTrue())
})

It("detects an atomic file replacement", func() {
previous, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())

replacement, err := os.CreateTemp("", "certificate-replacement-*.pem")
Expect(err).NotTo(HaveOccurred())
replacementPath := replacement.Name()
defer func() {
err := os.Remove(replacementPath)
Expect(err == nil || errors.Is(err, os.ErrNotExist)).To(BeTrue(),
"failed to remove replacement certificate %q: %v", replacementPath, err)
}()
Expect(replacement.Close()).To(Succeed())
Expect(os.Chtimes(replacementPath, previous.ModTime(), previous.ModTime())).To(Succeed())
Expect(os.Rename(replacementPath, certPath)).To(Succeed())

current, err := os.Stat(certPath)
Expect(err).NotTo(HaveOccurred())
Expect(current.Size()).To(Equal(previous.Size()))
Expect(current.ModTime()).To(Equal(previous.ModTime()))
Expect(certificateFileChanged(previous, current)).To(BeTrue())
})
}

func testHTTPServers() {
var (
certFile string
Expand Down
113 changes: 57 additions & 56 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module github.com/k8snetworkplumbingwg/net-attach-def-admission-controller

go 1.25.9
go 1.26.0

toolchain go1.26.5

require (
github.com/containernetworking/cni v1.3.0
Expand All @@ -9,26 +11,25 @@ require (
github.com/onsi/ginkgo v1.16.5
github.com/onsi/gomega v1.37.0
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.22.0
github.com/prometheus/client_golang v1.23.2
gopkg.in/k8snetworkplumbingwg/multus-cni.v4 v4.2.3
k8s.io/api v0.34.2
k8s.io/apimachinery v0.34.2
k8s.io/client-go v0.34.2
k8s.io/component-base v0.34.2
k8s.io/api v0.36.2
k8s.io/apimachinery v0.36.2
k8s.io/client-go v0.36.2
k8s.io/component-base v0.36.2
)

require (
github.com/beorn7/perks v1.0.1 // indirect
github.com/cespare/xxhash/v2 v2.3.0 // indirect
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/emicklei/go-restful/v3 v3.12.2 // indirect
github.com/emicklei/go-restful/v3 v3.13.0 // indirect
github.com/fsnotify/fsnotify v1.9.0 // indirect
github.com/fxamacker/cbor/v2 v2.9.0 // indirect
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-logr/logr v1.4.3 // indirect
github.com/go-openapi/jsonpointer v0.21.0 // indirect
github.com/go-openapi/jsonreference v0.20.2 // indirect
github.com/go-openapi/swag v0.23.0 // indirect
Comment thread
coderabbitai[bot] marked this conversation as resolved.
github.com/gogo/protobuf v1.3.2 // indirect
github.com/google/gnostic-models v0.7.0 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/google/uuid v1.6.0 // indirect
Expand All @@ -40,64 +41,64 @@ require (
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/nxadm/tail v1.4.8 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/prometheus/client_model v0.6.1 // indirect
github.com/prometheus/common v0.62.0 // indirect
github.com/prometheus/procfs v0.15.1 // indirect
github.com/spf13/cobra v1.9.1 // indirect
github.com/spf13/pflag v1.0.6 // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/prometheus/client_model v0.6.2 // indirect
github.com/prometheus/common v0.67.5 // indirect
github.com/prometheus/procfs v0.19.2 // indirect
github.com/spf13/cobra v1.10.2 // indirect
github.com/spf13/pflag v1.0.9 // indirect
github.com/vishvananda/netns v0.0.5 // indirect
github.com/x448/float16 v0.8.4 // indirect
go.yaml.in/yaml/v2 v2.4.2 // indirect
go.yaml.in/yaml/v2 v2.4.3 // indirect
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/net v0.41.0 // indirect
golang.org/x/oauth2 v0.28.0 // indirect
golang.org/x/sys v0.33.0 // indirect
golang.org/x/term v0.32.0 // indirect
golang.org/x/text v0.26.0 // indirect
golang.org/x/time v0.9.0 // indirect
google.golang.org/protobuf v1.36.6 // indirect
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
golang.org/x/net v0.55.0 // indirect
golang.org/x/oauth2 v0.34.0 // indirect
golang.org/x/sys v0.45.0 // indirect
golang.org/x/term v0.43.0 // indirect
golang.org/x/text v0.37.0 // indirect
golang.org/x/time v0.14.0 // indirect
google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect
gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/klog/v2 v2.130.1 // indirect
k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b // indirect
k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 // indirect
sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect
k8s.io/klog/v2 v2.140.0 // indirect
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a // indirect
k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 // indirect
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect
sigs.k8s.io/randfill v1.0.0 // indirect
sigs.k8s.io/structured-merge-diff/v6 v6.3.0 // indirect
sigs.k8s.io/structured-merge-diff/v6 v6.3.2 // indirect
sigs.k8s.io/yaml v1.6.0 // indirect
)

replace (
github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2
k8s.io/api => k8s.io/api v0.34.2
k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.34.2
k8s.io/apimachinery => k8s.io/apimachinery v0.34.2
k8s.io/apiserver => k8s.io/apiserver v0.34.2
k8s.io/cli-runtime => k8s.io/cli-runtime v0.34.2
k8s.io/client-go => k8s.io/client-go v0.34.2
k8s.io/cloud-provider => k8s.io/cloud-provider v0.34.2
k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.34.2
k8s.io/code-generator => k8s.io/code-generator v0.34.2
k8s.io/component-base => k8s.io/component-base v0.34.2
k8s.io/component-helpers => k8s.io/component-helpers v0.34.2
k8s.io/controller-manager => k8s.io/controller-manager v0.34.2
k8s.io/cri-api => k8s.io/cri-api v0.34.2
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.34.2
k8s.io/dynamic-resource-allocation => k8s.io/dynamic-resource-allocation v0.34.2
k8s.io/kms => k8s.io/kms v0.34.2
k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.34.2
k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.34.2
k8s.io/kube-proxy => k8s.io/kube-proxy v0.34.2
k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.34.2
k8s.io/kubectl => k8s.io/kubectl v0.34.2
k8s.io/kubelet => k8s.io/kubelet v0.34.2
k8s.io/kubernetes => k8s.io/kubernetes v1.34.1
k8s.io/metrics => k8s.io/metrics v0.34.2
k8s.io/mount-utils => k8s.io/mount-utils v0.34.2
k8s.io/pod-security-admission => k8s.io/pod-security-admission v0.34.2
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.34.2
k8s.io/api => k8s.io/api v0.36.2
k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.36.2
k8s.io/apimachinery => k8s.io/apimachinery v0.36.2
k8s.io/apiserver => k8s.io/apiserver v0.36.2
k8s.io/cli-runtime => k8s.io/cli-runtime v0.36.2
k8s.io/client-go => k8s.io/client-go v0.36.2
k8s.io/cloud-provider => k8s.io/cloud-provider v0.36.2
k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.36.2
k8s.io/code-generator => k8s.io/code-generator v0.36.2
k8s.io/component-base => k8s.io/component-base v0.36.2
k8s.io/component-helpers => k8s.io/component-helpers v0.36.2
k8s.io/controller-manager => k8s.io/controller-manager v0.36.2
k8s.io/cri-api => k8s.io/cri-api v0.36.2
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.36.2
k8s.io/dynamic-resource-allocation => k8s.io/dynamic-resource-allocation v0.36.2
k8s.io/kms => k8s.io/kms v0.36.2
k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.36.2
k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.36.2
k8s.io/kube-proxy => k8s.io/kube-proxy v0.36.2
k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.36.2
k8s.io/kubectl => k8s.io/kubectl v0.36.2
k8s.io/kubelet => k8s.io/kubelet v0.36.2
k8s.io/kubernetes => k8s.io/kubernetes v1.36.2
k8s.io/metrics => k8s.io/metrics v0.36.2
k8s.io/mount-utils => k8s.io/mount-utils v0.36.2
k8s.io/pod-security-admission => k8s.io/pod-security-admission v0.36.2
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.36.2
)
Loading