From 90e20b99413720263c3a5b9f874ddb18e9683977 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Wed, 17 Feb 2021 12:26:10 +0000 Subject: [PATCH 1/8] Add static analysis github action Add golangci-lint action with extra linters besides the default set [1]. Add hadolint and shellcheck. [1] https://golangci-lint.run/usage/linters Signed-off-by: Kennelly, Martin --- .github/workflows/lint.yml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 00000000..7ef3653e --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,35 @@ +name: Go-static-analysis +on: [push, pull_request] +jobs: + golangci: + name: Lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: golangci-lint + uses: golangci/golangci-lint-action@v2 + with: + # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. + version: v1.29 + # Adding additional linters beside the default set - See https://golangci-lint.run/usage/linters + args: --enable=golint,bodyclose,gosec,whitespace + shellcheck: + name: Shellcheck + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Run ShellCheck + uses: ludeeus/action-shellcheck@master + hadolint: + runs-on: ubuntu-latest + name: Hadolint + steps: + - uses: actions/checkout@v2 + - uses: brpaz/hadolint-action@v1.2.1 + name: Run Hadolint + with: + dockerfile: Dockerfile + - uses: brpaz/hadolint-action@v1.2.1 + with: + dockerfile: Dockerfile.rhel7 + From 150e231f8f010265f32494bf6ffb989f4efa4e85 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Wed, 5 May 2021 13:04:40 +0100 Subject: [PATCH 2/8] Create Make target for linting Inclusion of golangci-lint config file allows local lint execution and Makefile inclusion lowers the barrier for devs and GH workflow consumption. Signed-off-by: Kennelly, Martin --- .golangci.yml | 113 ++++++++++++++++++++++++++++++++++++++++++++++++ Makefile | 3 ++ scripts/lint.sh | 32 ++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 .golangci.yml create mode 100755 scripts/lint.sh diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..a19bcf25 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,113 @@ +# Tested with golangci-lint ver. 1.37 +linters-settings: + depguard: + list-type: blacklist + packages: + # logging is allowed only by logutils.Log, logrus + # is allowed to use only in logutils package + - github.com/sirupsen/logrus + packages-with-error-message: + - github.com/sirupsen/logrus: "logging is allowed only by logutils.Log" + dupl: + threshold: 100 + funlen: + lines: 100 + statements: 50 + goconst: + min-len: 2 + min-occurrences: 2 + gocritic: + enabled-tags: + - diagnostic + - experimental + - opinionated + - performance + - style + disabled-checks: + - dupImport # https://github.com/go-critic/go-critic/issues/845 + - ifElseChain + - octalLiteral + - whyNoLint + - wrapperFunc + - unnamedResult + gocyclo: + min-complexity: 15 + goimports: + local-prefixes: github.com/k8snetworkplumbingwg/sriov-network-device-plugin + golint: + min-confidence: 0 + gomnd: + settings: + mnd: + # don't include the "operation" and "assign" + checks: argument,case,condition,return + lll: + line-length: 140 + misspell: + locale: US + prealloc: + # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them. + # True by default. + simple: true + range-loops: true # Report preallocation suggestions on range loops, true by default + for-loops: false # Report preallocation suggestions on for loops, false by default + +linters: + # please, do not use `enable-all`: it's deprecated and will be removed soon. + # inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint + disable-all: true + enable: + - bodyclose + - deadcode + - depguard + - dogsled + - dupl + - errcheck + - exportloopref + - exhaustive + - funlen + #- gochecknoinits + - goconst + - gocritic + - gocyclo + - gofmt + - goimports + - golint + - gomnd + - goprintffuncname + - gosec + - gosimple + #- govet + - ineffassign + - lll + - misspell + - nakedret + - prealloc + - rowserrcheck + #- scopelint + - staticcheck + - structcheck + - stylecheck + - typecheck + - unconvert + - unparam + - unused + - varcheck + - whitespace + +issues: + # Excluding configuration per-path, per-linter, per-text and per-source + exclude-rules: + - path: _test\.go + linters: + - gomnd + - gosec + - dupl + +run: + skip-dirs: + - .github/ + - deployments/ + - docs/ + - images/ + - scripts/ diff --git a/Makefile b/Makefile index 8832e48e..92ba512f 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,9 @@ image : test : scripts/test.sh +lint : + scripts/lint.sh + vendor : go mod tidy && go mod vendor diff --git a/scripts/lint.sh b/scripts/lint.sh new file mode 100755 index 00000000..53805851 --- /dev/null +++ b/scripts/lint.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +set -eo pipefail + +GOLANGCI_LINT_VER="v1.37.1" +GOLANGCI_BIN_NAME="golangci-lint" +GOLANGCI_INSTALL_URL="https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh" +RETRY=3 +TIMEOUT=300 + +repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../" && pwd )" +repo_bin="${repo_root:?}/bin" +export PATH="${PATH}:${repo_bin}" + +# install golangci lint if not available in PATH +if ! command -v "${GOLANGCI_BIN_NAME}" &> /dev/null; then + mkdir -p "${repo_bin}" + + curl --silent --show-error --fail --location --retry "${RETRY}" \ + --connect-timeout "${TIMEOUT}" "${GOLANGCI_INSTALL_URL}" \ + | sh -s -- -b "${repo_bin}" "${GOLANGCI_LINT_VER}" + + export PATH="${PATH}:${repo_bin}" + + # validate that we can access golangci lint + if ! command -v "${GOLANGCI_BIN_NAME}" &> /dev/null; then + echo "failed to install golangci lint in '${repo_bin}'" + exit 2 + fi +fi + +cd "${repo_root}" +"${GOLANGCI_BIN_NAME}" run From f7e78da51198788b9271656436000e92c0d1c820 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Wed, 5 May 2021 13:17:27 +0100 Subject: [PATCH 3/8] Update Go mod sum file Signed-off-by: Kennelly, Martin --- go.sum | 3 --- 1 file changed, 3 deletions(-) diff --git a/go.sum b/go.sum index 39b35fed..e9f6e55a 100644 --- a/go.sum +++ b/go.sum @@ -262,7 +262,6 @@ golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20190930134127-c5a3c61f89f3/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20191004110552-13f9640d40b9/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= -golang.org/x/net v0.0.0-20200520004742-59133d7f0dd7 h1:AeiKBIuRw3UomYXSbLy0Mc2dDLfdtbT/IVn4keq83P0= golang.org/x/net v0.0.0-20200520004742-59133d7f0dd7/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= golang.org/x/net v0.0.0-20201021035429-f5854403a974 h1:IX6qOQeG5uLjB/hjjwjedwfjND0hgjPMMyO1RoIXQNI= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= @@ -295,7 +294,6 @@ golang.org/x/sys v0.0.0-20191022100944-742c48ecaeb7/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191120155948-bd437916bb0e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20200519105757-fe76b779f299 h1:DYfZAGf2WMFjMxbgTjaC+2HC7NkNAQs+6Q8b9WEB/F4= golang.org/x/sys v0.0.0-20200519105757-fe76b779f299/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f h1:+Nyd8tzPX9R7BWHguqsrbFdRx3WQ/1ib8I44HXV5yTA= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -328,7 +326,6 @@ golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roY golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= -golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From 0f3978651467c5af0b659999821214564a6d6d00 Mon Sep 17 00:00:00 2001 From: Emma Kenny Date: Tue, 23 Feb 2021 14:39:51 +0000 Subject: [PATCH 4/8] Fix GA static analysis errors --- Dockerfile | 2 +- cmd/webhook/main.go | 24 +++++++---- pkg/installer/installer.go | 19 +++------ pkg/webhook/tlsutils.go | 4 +- pkg/webhook/webhook.go | 59 +++++++++++++------------- pkg/webhook/webhook_test.go | 66 +++++++++++++++--------------- scripts/build-image.sh | 15 ++++--- scripts/webhook-patch-ca-bundle.sh | 5 ++- test/e2e/e2e_tests_suite_test.go | 3 +- test/e2e/httpserver_test.go | 2 +- test/util/images.go | 2 +- test/util/pod.go | 10 ++--- 12 files changed, 104 insertions(+), 107 deletions(-) diff --git a/Dockerfile b/Dockerfile index ac59c3e4..e6a2f6cf 100644 --- a/Dockerfile +++ b/Dockerfile @@ -15,7 +15,7 @@ FROM golang:1.13-alpine as builder COPY . /usr/src/network-resources-injector WORKDIR /usr/src/network-resources-injector -RUN apk add --update --virtual build-dependencies build-base bash && \ +RUN apk add --no-cache --virtual build-dependencies build-base bash && \ make FROM alpine:3.11 diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 4279059a..dd69a346 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -15,23 +15,23 @@ package main import ( - "crypto/tls" "context" + "crypto/tls" "flag" "fmt" "net/http" "os" "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/api/errors" "github.com/fsnotify/fsnotify" "github.com/golang/glog" "github.com/k8snetworkplumbingwg/network-resources-injector/pkg/webhook" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( - defaultClientCa = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + defaultClientCa = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" userDefinedInjectionConfigMap = "nri-user-defined-injections" ) @@ -44,7 +44,7 @@ func main() { cert := flag.String("tls-cert-file", "cert.pem", "File containing the default x509 Certificate for HTTPS.") key := flag.String("tls-private-key-file", "key.pem", "File containing the default x509 private key matching --tls-cert-file.") insecure := flag.Bool("insecure", false, "Disable adding client CA to server TLS endpoint --insecure") - injectHugepageDownApi := flag.Bool("injectHugepageDownApi", false, "Enable hugepage requests and limits into Downward API.") + injectHugepageDownAPI := flag.Bool("injectHugepageDownApi", false, "Enable hugepage requests and limits into Downward API.") flag.Var(&clientCAPaths, "client-ca", "File containing client CA. This flag is repeatable if more than one client CA needs to be added to server") resourceNameKeys := flag.String("network-resource-name-keys", "k8s.v1.cni.cncf.io/resourceName", "comma separated resource name keys --network-resource-name-keys.") resourcesHonorFlag := flag.Bool("honor-resources", false, "Honor the existing requested resources requests & limits --honor-resources") @@ -68,7 +68,7 @@ func main() { glog.Infof("starting mutating admission controller for network resources injection") - keyPair, err := webhook.NewTlsKeypairReloader(*cert, *key) + keyPair, err := webhook.NewTLSKeypairReloader(*cert, *key) if err != nil { glog.Fatalf("error load certificate: %s", err.Error()) } @@ -81,7 +81,7 @@ func main() { /* init API client */ clientset := webhook.SetupInClusterClient() - webhook.SetInjectHugepageDownApi(*injectHugepageDownApi) + webhook.SetInjectHugepageDownAPI(*injectHugepageDownAPI) webhook.SetHonorExistingResources(*resourcesHonorFlag) @@ -149,8 +149,14 @@ func main() { keyUpdated := false for { - watcher.Add(*cert) - watcher.Add(*key) + err := watcher.Add(*cert) + if err != nil { + glog.Fatalf("error adding cert: %v", err) + } + err = watcher.Add(*key) + if err != nil { + glog.Fatalf("error adding key: %v", err) + } select { case event, ok := <-watcher.Events: diff --git a/pkg/installer/installer.go b/pkg/installer/installer.go index 0c42080d..1fa420c4 100644 --- a/pkg/installer/installer.go +++ b/pkg/installer/installer.go @@ -60,7 +60,7 @@ func getSignedCertificate(request []byte) ([]byte, error) { csr, err := clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) if csr != nil || err == nil { glog.Infof("CSR %s already exists, removing it first", csrName) - clientset.CertificatesV1beta1().CertificateSigningRequests().Delete(context.TODO(), csrName, metav1.DeleteOptions{}) + _ = clientset.CertificatesV1beta1().CertificateSigningRequests().Delete(context.TODO(), csrName, metav1.DeleteOptions{}) } glog.Infof("creating new CSR %s", csrName) @@ -101,7 +101,10 @@ func getSignedCertificate(request []byte) ([]byte, error) { /* wait for the cert to be issued */ glog.Infof("waiting for the signed certificate to be issued...") start := time.Now() - for range time.Tick(time.Second) { + ticker := time.NewTicker(time.Second) + defer ticker.Stop() + + for range ticker.C { csr, err = clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) if err != nil { return nil, errors.Wrap(err, "error getting signed ceritificate from the API server") @@ -219,18 +222,6 @@ func removeMutatingWebhookIfExists(configName string) { } } -func removeSecretIfExists(secretName string) { - secret, err := clientset.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{}) - if secret != nil && err == nil { - glog.Infof("secret %s already exists, removing it first", secretName) - err := clientset.CoreV1().Secrets(namespace).Delete(context.TODO(), secretName, metav1.DeleteOptions{}) - if err != nil { - glog.Errorf("error trying to remove secret: %s", err) - } - glog.Infof("secret %s removed", secretName) - } -} - // Install creates resources required by mutating admission webhook func Install(k8sNamespace, namePrefix string) { /* setup Kubernetes API client */ diff --git a/pkg/webhook/tlsutils.go b/pkg/webhook/tlsutils.go index ac24a4c9..8584617b 100644 --- a/pkg/webhook/tlsutils.go +++ b/pkg/webhook/tlsutils.go @@ -68,8 +68,8 @@ func (keyPair *tlsKeypairReloader) GetCertificateFunc() func(*tls.ClientHelloInf } } -// NewTlsKeypairReloader reload tlsKeypairReloader struct -func NewTlsKeypairReloader(certPath, keyPath string) (*tlsKeypairReloader, error) { +// reload tlsKeypairReloader struct +func NewTLSKeypairReloader(certPath, keyPath string) (*tlsKeypairReloader, error) { result := &tlsKeypairReloader{ certPath: certPath, keyPath: keyPath, diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 2b21bfa7..b98cdec1 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -9,7 +9,7 @@ // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and +// See the License for specific language governing permissions and // limitations under the License. package webhook @@ -68,7 +68,7 @@ const ( var ( clientset kubernetes.Interface - injectHugepageDownApi bool + injectHugepageDownAPI bool resourceNameKeys []string honorExistingResources bool userDefinedInjects = &userDefinedInjections{Patchs: make(map[string]jsonPatchOperation)} @@ -162,7 +162,7 @@ func deserializePod(ar *v1beta1.AdmissionReview) (corev1.Pod, error) { } ownerRef := pod.ObjectMeta.OwnerReferences - if ownerRef != nil && len(ownerRef) > 0 { + if len(ownerRef) > 0 { namespace, err := getNamespaceFromOwnerReference(pod.ObjectMeta.OwnerReferences[0]) if err != nil { return pod, err @@ -242,10 +242,9 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s } return - } -func toSafeJsonPatchKey(in string) string { +func toSafeJSONPatchKey(in string) string { out := strings.Replace(in, "~", "~0", -1) out = strings.Replace(out, "/", "~1", -1) return out @@ -360,9 +359,9 @@ func getNetworkAttachmentDefinition(namespace, name string) (*cniv1.NetworkAttac } networkAttachmentDefinition := cniv1.NetworkAttachmentDefinition{} - json.Unmarshal(rawNetworkAttachmentDefinition, &networkAttachmentDefinition) + err = json.Unmarshal(rawNetworkAttachmentDefinition, &networkAttachmentDefinition) - return &networkAttachmentDefinition, nil + return &networkAttachmentDefinition, err } func parseNetworkAttachDefinition(net *multus.NetworkSelectionElement, reqs map[string]int64, nsMap map[string]string) (map[string]int64, map[string]string, error) { @@ -418,13 +417,17 @@ func handleValidationError(w http.ResponseWriter, ar *v1beta1.AdmissionReview, o func writeResponse(w http.ResponseWriter, ar *v1beta1.AdmissionReview) { glog.Infof("sending response to the Kubernetes API server") resp, _ := json.Marshal(ar) - w.Write(resp) + _, err := w.Write(resp) + + if err != nil { + glog.Warningf("write response failed: %v", err) + } } func patchEmptyResources(patch []jsonPatchOperation, containerIndex uint, key string) []jsonPatchOperation { patch = append(patch, jsonPatchOperation{ Operation: "add", - Path: "/spec/containers/" + fmt.Sprintf("%d", containerIndex) + "/resources/" + toSafeJsonPatchKey(key), + Path: "/spec/containers/" + fmt.Sprintf("%d", containerIndex) + "/resources/" + toSafeJSONPatchKey(key), Value: corev1.ResourceList{}, }) return patch @@ -514,7 +517,6 @@ func createVolPatch(patch []jsonPatchOperation, hugepageResourceList []hugepageR func addEnvVar(patch []jsonPatchOperation, containerIndex int, firstElement bool, envName string, envVal string) []jsonPatchOperation { - env := corev1.EnvVar{ Name: envName, Value: envVal, @@ -539,7 +541,6 @@ func addEnvVar(patch []jsonPatchOperation, containerIndex int, firstElement bool func createEnvPatch(patch []jsonPatchOperation, container *corev1.Container, containerIndex int, envName string, envVal string) []jsonPatchOperation { - // Determine if requested ENV already exists found := false firstElement := false @@ -566,16 +567,15 @@ func createEnvPatch(patch []jsonPatchOperation, container *corev1.Container, func createNodeSelectorPatch(patch []jsonPatchOperation, existing map[string]string, desired map[string]string) []jsonPatchOperation { targetMap := make(map[string]string) - if existing != nil { - for k, v := range existing { - targetMap[k] = v - } + + for k, v := range existing { + targetMap[k] = v } - if desired != nil { - for k, v := range desired { - targetMap[k] = v - } + + for k, v := range desired { + targetMap[k] = v } + if len(targetMap) == 0 { return patch } @@ -651,12 +651,12 @@ func updateResourcePatch(patch []jsonPatchOperation, Containers []corev1.Contain func appendResource(patch []jsonPatchOperation, resourceName string, reqQuantity, limitQuantity resource.Quantity) []jsonPatchOperation { patch = append(patch, jsonPatchOperation{ Operation: "add", - Path: "/spec/containers/0/resources/requests/" + toSafeJsonPatchKey(resourceName), + Path: "/spec/containers/0/resources/requests/" + toSafeJSONPatchKey(resourceName), Value: reqQuantity, }) patch = append(patch, jsonPatchOperation{ Operation: "add", - Path: "/spec/containers/0/resources/limits/" + toSafeJsonPatchKey(resourceName), + Path: "/spec/containers/0/resources/limits/" + toSafeJSONPatchKey(resourceName), Value: limitQuantity, }) @@ -847,12 +847,12 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { // Determine if hugepages are being requested for a given container, // and if so, expose the value to the container via Downward API. var hugepageResourceList []hugepageResourceData - glog.Infof("injectHugepageDownApi=%v", injectHugepageDownApi) - if injectHugepageDownApi { + glog.Infof("injectHugepageDownAPI=%v", injectHugepageDownAPI) + if injectHugepageDownAPI { for containerIndex, container := range pod.Spec.Containers { found := false if len(container.Resources.Requests) != 0 { - if quantity, exists := container.Resources.Requests["hugepages-1Gi"]; exists && quantity.IsZero() == false { + if quantity, exists := container.Resources.Requests["hugepages-1Gi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "requests.hugepages-1Gi", ContainerName: container.Name, @@ -861,7 +861,7 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true } - if quantity, exists := container.Resources.Requests["hugepages-2Mi"]; exists && quantity.IsZero() == false { + if quantity, exists := container.Resources.Requests["hugepages-2Mi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "requests.hugepages-2Mi", ContainerName: container.Name, @@ -872,7 +872,7 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { } } if len(container.Resources.Limits) != 0 { - if quantity, exists := container.Resources.Limits["hugepages-1Gi"]; exists && quantity.IsZero() == false { + if quantity, exists := container.Resources.Limits["hugepages-1Gi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "limits.hugepages-1Gi", ContainerName: container.Name, @@ -881,7 +881,7 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true } - if quantity, exists := container.Resources.Limits["hugepages-2Mi"]; exists && quantity.IsZero() == false { + if quantity, exists := container.Resources.Limits["hugepages-2Mi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "limits.hugepages-2Mi", ContainerName: container.Name, @@ -924,7 +924,6 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { return } } - writeResponse(w, ar) } @@ -956,8 +955,8 @@ func SetupInClusterClient() kubernetes.Interface { // SetInjectHugepageDownApi sets a flag to indicate whether or not to inject the // hugepage request and limit for the Downward API. -func SetInjectHugepageDownApi(hugepageFlag bool) { - injectHugepageDownApi = hugepageFlag +func SetInjectHugepageDownAPI(hugepageFlag bool) { + injectHugepageDownAPI = hugepageFlag } // SetHonorExistingResources initialize the honorExistingResources flag diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index d4f6c43f..ab28f737 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -20,7 +20,6 @@ import ( . "github.com/onsi/gomega" "bytes" - "fmt" "net/http" "net/http/httptest" @@ -32,7 +31,6 @@ import ( ) var _ = Describe("Webhook", func() { - Describe("Preparing Admission Review Response", func() { Context("Admission Review Request is nil", func() { It("should return error", func() { @@ -108,7 +106,7 @@ var _ = Describe("Webhook", func() { Describe("Handling requests", func() { Context("Request body is empty", func() { It("mutate - should return an error", func() { - req := httptest.NewRequest("POST", fmt.Sprintf("https://fakewebhook/mutate"), nil) + req := httptest.NewRequest("POST", "https://fakewebhook/mutate", nil) w := httptest.NewRecorder() MutateHandler(w, req) resp := w.Result() @@ -118,7 +116,7 @@ var _ = Describe("Webhook", func() { Context("Content type is not application/json", func() { It("mutate - should return an error", func() { - req := httptest.NewRequest("POST", fmt.Sprintf("https://fakewebhook/mutate"), bytes.NewBufferString("fake-body")) + req := httptest.NewRequest("POST", "https://fakewebhook/mutate", bytes.NewBufferString("fake-body")) req.Header.Set("Content-Type", "invalid-type") w := httptest.NewRecorder() MutateHandler(w, req) @@ -139,7 +137,7 @@ var _ = Describe("Webhook", func() { "match pod label", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Labels: map[string]string{"nri-inject-annotation": "true"}, }, Spec: corev1.PodSpec{}, @@ -147,15 +145,15 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, }, []jsonPatchOperation{ { Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, }, ), @@ -163,7 +161,7 @@ var _ = Describe("Webhook", func() { "doesn't match pod label value", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Labels: map[string]string{"nri-inject-annotation": "false"}, }, Spec: corev1.PodSpec{}, @@ -171,8 +169,8 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, }, nil, @@ -181,7 +179,7 @@ var _ = Describe("Webhook", func() { "doesn't match pod label key", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Labels: map[string]string{"nri-inject-labels": "true"}, }, Spec: corev1.PodSpec{}, @@ -189,8 +187,8 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, }, nil, @@ -209,7 +207,7 @@ var _ = Describe("Webhook", func() { "k8s.v1.cni.cncf.io/networks", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Annotations: map[string]string{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, Spec: corev1.PodSpec{}, @@ -217,8 +215,8 @@ var _ = Describe("Webhook", func() { []jsonPatchOperation{ { Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net-user-defined"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net-user-defined"}, }, }, "sriov-net", @@ -229,7 +227,7 @@ var _ = Describe("Webhook", func() { "k8s.v1.cni.cncf.io/networks", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Annotations: map[string]string{"v1.multus-cni.io/default-network": "sriov-net"}, }, Spec: corev1.PodSpec{}, @@ -237,8 +235,8 @@ var _ = Describe("Webhook", func() { []jsonPatchOperation{ { Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net-user-defined"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net-user-defined"}, }, }, "sriov-net-user-defined", @@ -249,7 +247,7 @@ var _ = Describe("Webhook", func() { "k8s.v1.cni.cncf.io/networks", corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", Annotations: map[string]string{"v1.multus-cni.io/default-network": "sriov-net"}, }, Spec: corev1.PodSpec{}, @@ -257,8 +255,8 @@ var _ = Describe("Webhook", func() { []jsonPatchOperation{ { Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-user-defined"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-user-defined"}, }, }, "", @@ -290,8 +288,8 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, }, }, ), @@ -305,8 +303,8 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, }, }, ), @@ -328,8 +326,8 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, }, }, map[string]jsonPatchOperation{}, @@ -343,15 +341,15 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-old"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-old"}, }, }, map[string]jsonPatchOperation{ "nri-inject-annotation": jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", - Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-new"}, + Path: "/metadata/annotations", + Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-new"}, }, }, ), diff --git a/scripts/build-image.sh b/scripts/build-image.sh index de5111f4..be0981cd 100755 --- a/scripts/build-image.sh +++ b/scripts/build-image.sh @@ -20,12 +20,15 @@ set -e # set proxy args BUILD_ARGS=() -[ ! -z "$http_proxy" ] && BUILD_ARGS+=("--build-arg http_proxy=$http_proxy") -[ ! -z "$HTTP_PROXY" ] && BUILD_ARGS+=("--build-arg HTTP_PROXY=$HTTP_PROXY") -[ ! -z "$https_proxy" ] && BUILD_ARGS+=("--build-arg https_proxy=$https_proxy") -[ ! -z "$HTTPS_PROXY" ] && BUILD_ARGS+=("--build-arg HTTPS_PROXY=$HTTPS_PROXY") -[ ! -z "$no_proxy" ] && BUILD_ARGS+=("--build-arg no_proxy=$no_proxy") -[ ! -z "$NO_PROXY" ] && BUILD_ARGS+=("--build-arg NO_PROXY=$NO_PROXY") +[ -n "$http_proxy" ] && BUILD_ARGS+=("--build-arg http_proxy=$http_proxy") +[ -n "$HTTP_PROXY" ] && BUILD_ARGS+=("--build-arg HTTP_PROXY=$HTTP_PROXY") +[ -n "$https_proxy" ] && BUILD_ARGS+=("--build-arg https_proxy=$https_proxy") +[ -n "$HTTPS_PROXY" ] && BUILD_ARGS+=("--build-arg HTTPS_PROXY=$HTTPS_PROXY") +[ -n "$no_proxy" ] && BUILD_ARGS+=("--build-arg no_proxy=$no_proxy") +[ -n "$NO_PROXY" ] && BUILD_ARGS+=("--build-arg NO_PROXY=$NO_PROXY") +[ -n "$HTTP_PROXY" ] && BUILD_ARGS+=("--build-arg HTTP_PROXY=$HTTP_PROXY") +[ -n "$HTTPS_PROXY" ] && BUILD_ARGS+=("--build-arg HTTPS_PROXY=$HTTPS_PROXY") +[ -n "$NO_PROXY" ] && BUILD_ARGS+=("--build-arg NO_PROXY=$NO_PROXY") # build admission controller Docker image docker build ${BUILD_ARGS[@]} -f Dockerfile -t network-resources-injector . diff --git a/scripts/webhook-patch-ca-bundle.sh b/scripts/webhook-patch-ca-bundle.sh index 0093cc46..907c305c 100755 --- a/scripts/webhook-patch-ca-bundle.sh +++ b/scripts/webhook-patch-ca-bundle.sh @@ -5,8 +5,9 @@ set -o errexit set -o nounset set -o pipefail -export CA_BUNDLE=$(kubectl get configmap -n kube-system extension-apiserver-authentication -o=jsonpath='{.data.client-ca-file}' | base64 --w=0) - +jsonpath='{.data.client-ca-file}' +CA_BUNDLE=$(kubectl get configmap -n kube-system extension-apiserver-authentication -o=$jsonpath | base64 --w=0) +export CA_BUNDLE if command -v envsubst >/dev/null 2>&1; then envsubst else diff --git a/test/e2e/e2e_tests_suite_test.go b/test/e2e/e2e_tests_suite_test.go index b96bb46f..de5e54af 100644 --- a/test/e2e/e2e_tests_suite_test.go +++ b/test/e2e/e2e_tests_suite_test.go @@ -13,7 +13,6 @@ import ( "k8s.io/client-go/util/homedir" ) - const ( testNetworkName = "foo-network" testNetworkResName = "example.com/foo" @@ -32,7 +31,7 @@ type ClientSet struct { coreclient.CoreV1Interface } -func init() { +func init() { if home := homedir.HomeDir(); home != "" { kubeConfigPath = flag.String("kubeconfig", filepath.Join(home, ".kube", "config"), "path to your kubeconfig file") } else { diff --git a/test/e2e/httpserver_test.go b/test/e2e/httpserver_test.go index aff867b7..9a381d4c 100644 --- a/test/e2e/httpserver_test.go +++ b/test/e2e/httpserver_test.go @@ -7,7 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -var _ = Describe("Network injection testing", func(){ +var _ = Describe("Network injection testing", func() { var pod *corev1.Pod var err error diff --git a/test/util/images.go b/test/util/images.go index dc5ab0d6..fc91e2b4 100644 --- a/test/util/images.go +++ b/test/util/images.go @@ -6,7 +6,7 @@ import ( ) var ( - registry string + registry string testImage string ) diff --git a/test/util/pod.go b/test/util/pod.go index 3ced9cb5..55c0495d 100644 --- a/test/util/pod.go +++ b/test/util/pod.go @@ -51,15 +51,15 @@ func GetPodDefinition(ns string) *corev1.Pod { var graceTime int64 = 0 return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "nri-e2e-test", - Namespace: ns, + Name: "nri-e2e-test", + Namespace: ns, }, Spec: corev1.PodSpec{ TerminationGracePeriodSeconds: &graceTime, Containers: []corev1.Container{ { - Name: "test", - Image: GetPodTestImage(), + Name: "test", + Image: GetPodTestImage(), Command: []string{"/bin/sh", "-c", "sleep INF"}, }, }, @@ -82,7 +82,7 @@ func GetMultiNetworks(nad []string, ns string) *corev1.Pod { } //WaitForPodStateRunning waits for pod to enter running state -func WaitForPodStateRunning(core coreclient.CoreV1Interface , podName, ns string, timeout, interval time.Duration) error { +func WaitForPodStateRunning(core coreclient.CoreV1Interface, podName, ns string, timeout, interval time.Duration) error { time.Sleep(30 * time.Second) return wait.PollImmediate(interval, timeout, func() (done bool, err error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) From 2f23ebc6bfdb38646fbb40b70a614bad7d15ef68 Mon Sep 17 00:00:00 2001 From: Martin Kennelly Date: Thu, 3 Jun 2021 14:54:17 +0100 Subject: [PATCH 5/8] Fix various linting issues found by golanglint-ci Signed-off-by: Martin Kennelly --- .golangci.yml | 7 +++- cmd/webhook/main.go | 20 +++++++---- pkg/installer/installer.go | 58 +++++++++++++++++--------------- pkg/webhook/tlsutils.go | 8 ++--- pkg/webhook/webhook.go | 66 ++++++++++++++++++++----------------- pkg/webhook/webhook_test.go | 36 +++++++++++--------- test/e2e/httpserver_test.go | 4 +-- test/util/images.go | 2 +- test/util/pod.go | 23 ++++++++----- 9 files changed, 126 insertions(+), 98 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a19bcf25..b20b958d 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -103,7 +103,12 @@ issues: - gomnd - gosec - dupl - + - text: "Magic number: 2" + linters: + - gomnd + - text: "Magic number: 1024" + linters: + - gomnd run: skip-dirs: - .github/ diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index dd69a346..92ceb95b 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -33,6 +33,7 @@ import ( const ( defaultClientCa = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" userDefinedInjectionConfigMap = "nri-user-defined-injections" + userDefinedInjectionInterval = 30 * time.Second ) func main() { @@ -42,12 +43,17 @@ func main() { port := flag.Int("port", 8443, "The port on which to serve.") address := flag.String("bind-address", "0.0.0.0", "The IP address on which to listen for the --port port.") cert := flag.String("tls-cert-file", "cert.pem", "File containing the default x509 Certificate for HTTPS.") - key := flag.String("tls-private-key-file", "key.pem", "File containing the default x509 private key matching --tls-cert-file.") + key := flag.String("tls-private-key-file", "key.pem", + "File containing the default x509 private key matching --tls-cert-file.") insecure := flag.Bool("insecure", false, "Disable adding client CA to server TLS endpoint --insecure") - injectHugepageDownAPI := flag.Bool("injectHugepageDownApi", false, "Enable hugepage requests and limits into Downward API.") - flag.Var(&clientCAPaths, "client-ca", "File containing client CA. This flag is repeatable if more than one client CA needs to be added to server") - resourceNameKeys := flag.String("network-resource-name-keys", "k8s.v1.cni.cncf.io/resourceName", "comma separated resource name keys --network-resource-name-keys.") - resourcesHonorFlag := flag.Bool("honor-resources", false, "Honor the existing requested resources requests & limits --honor-resources") + injectHugepageDownAPI := flag.Bool("injectHugepageDownApi", false, + "Enable hugepage requests and limits into Downward API.") + flag.Var(&clientCAPaths, "client-ca", + "File containing client CA. This flag is repeatable if more than one client CA needs to be added to server") + resourceNameKeys := flag.String("network-resource-name-keys", "k8s.v1.cni.cncf.io/resourceName", + "comma separated resource name keys --network-resource-name-keys.") + resourcesHonorFlag := flag.Bool("honor-resources", false, + "Honor the existing requested resources requests & limits --honor-resources") flag.Parse() if *port < 1024 || *port > 65535 { @@ -100,7 +106,7 @@ func main() { return } if r.Method != http.MethodPost { - http.Error(w, "Invalid HTTP verb requested", 405) + http.Error(w, "invalid HTTP verb requested", http.StatusMethodNotAllowed) return } webhook.MutateHandler(w, r) @@ -187,7 +193,7 @@ func main() { continue } glog.Infof("watcher error: %v", err) - case <-time.After(30 * time.Second): + case <-time.After(userDefinedInjectionInterval): cm, err := clientset.CoreV1().ConfigMaps(namespace).Get( context.Background(), userDefinedInjectionConfigMap, metav1.GetOptions{}) if err != nil { diff --git a/pkg/installer/installer.go b/pkg/installer/installer.go index 1fa420c4..b584552f 100644 --- a/pkg/installer/installer.go +++ b/pkg/installer/installer.go @@ -57,42 +57,42 @@ func generateCSR() ([]byte, []byte, error) { func getSignedCertificate(request []byte) ([]byte, error) { csrName := strings.Join([]string{prefix, "csr"}, "-") - csr, err := clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) - if csr != nil || err == nil { + certSigReq, err := clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) + if certSigReq != nil || err == nil { glog.Infof("CSR %s already exists, removing it first", csrName) _ = clientset.CertificatesV1beta1().CertificateSigningRequests().Delete(context.TODO(), csrName, metav1.DeleteOptions{}) } glog.Infof("creating new CSR %s", csrName) /* build Kubernetes CSR object */ - csr = &v1beta1.CertificateSigningRequest{} - csr.ObjectMeta.Name = csrName - csr.ObjectMeta.Namespace = namespace - csr.Spec.Request = request - csr.Spec.Groups = []string{"system:authenticated"} - csr.Spec.Usages = []v1beta1.KeyUsage{v1beta1.UsageDigitalSignature, v1beta1.UsageServerAuth, v1beta1.UsageKeyEncipherment} + certSigReq = &v1beta1.CertificateSigningRequest{} + certSigReq.ObjectMeta.Name = csrName + certSigReq.ObjectMeta.Namespace = namespace + certSigReq.Spec.Request = request + certSigReq.Spec.Groups = []string{"system:authenticated"} + certSigReq.Spec.Usages = []v1beta1.KeyUsage{v1beta1.UsageDigitalSignature, v1beta1.UsageServerAuth, v1beta1.UsageKeyEncipherment} /* push CSR to Kubernetes API server */ - csr, err = clientset.CertificatesV1beta1().CertificateSigningRequests().Create(context.TODO(), csr, metav1.CreateOptions{}) + certSigReq, err = clientset.CertificatesV1beta1().CertificateSigningRequests().Create(context.TODO(), certSigReq, metav1.CreateOptions{}) if err != nil { return nil, errors.Wrap(err, "error creating CSR in Kubernetes API: %s") } glog.Infof("CSR pushed to the Kubernetes API") - if csr.Status.Certificate != nil { + if certSigReq.Status.Certificate != nil { glog.Infof("using already issued certificate for CSR %s", csrName) - return csr.Status.Certificate, nil + return certSigReq.Status.Certificate, nil } /* approve certificate in K8s API */ - csr.ObjectMeta.Name = csrName - csr.ObjectMeta.Namespace = namespace - csr.Status.Conditions = append(csr.Status.Conditions, v1beta1.CertificateSigningRequestCondition{ + certSigReq.ObjectMeta.Name = csrName + certSigReq.ObjectMeta.Namespace = namespace + certSigReq.Status.Conditions = append(certSigReq.Status.Conditions, v1beta1.CertificateSigningRequestCondition{ Type: v1beta1.CertificateApproved, Reason: "Approved by net-attach-def admission controller installer", Message: "This CSR was approved by net-attach-def admission controller installer.", LastUpdateTime: metav1.Now(), }) - _, err = clientset.CertificatesV1beta1().CertificateSigningRequests().UpdateApproval(context.TODO(), csr, metav1.UpdateOptions{}) + _, err = clientset.CertificatesV1beta1().CertificateSigningRequests().UpdateApproval(context.TODO(), certSigReq, metav1.UpdateOptions{}) glog.Infof("certificate approval sent") if err != nil { return nil, errors.Wrap(err, "error approving CSR in Kubernetes API") @@ -105,19 +105,20 @@ func getSignedCertificate(request []byte) ([]byte, error) { defer ticker.Stop() for range ticker.C { - csr, err = clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) + certSigReq, err = clientset.CertificatesV1beta1().CertificateSigningRequests().Get(context.TODO(), csrName, metav1.GetOptions{}) if err != nil { - return nil, errors.Wrap(err, "error getting signed ceritificate from the API server") + return nil, errors.Wrap(err, "error getting signed certificate from the API server") } - if csr.Status.Certificate != nil { - return csr.Status.Certificate, nil + if certSigReq.Status.Certificate != nil { + return certSigReq.Status.Certificate, nil } if time.Since(start) > 60*time.Second { break } } - return nil, errors.New("error getting certificate from the API server: request timed out - verify that Kubernetes certificate signer is setup, more at https://kubernetes.io/docs/tasks/tls/managing-tls-in-a-cluster/#a-note-to-cluster-administrators") + return nil, errors.New("error getting certificate from the API server: request timed out - verify that " + + "Kubernetes certificate signer is setup") } func writeToFile(certificate, key []byte, certFilename, keyFilename string) error { @@ -144,7 +145,7 @@ func createMutatingWebhookConfiguration(certificate []byte) error { }, }, Webhooks: []arv1beta1.MutatingWebhook{ - arv1beta1.MutatingWebhook{ + { Name: configName + ".k8s.cni.cncf.io", ClientConfig: arv1beta1.WebhookClientConfig{ CABundle: certificate, @@ -156,7 +157,7 @@ func createMutatingWebhookConfiguration(certificate []byte) error { }, FailurePolicy: &failurePolicy, Rules: []arv1beta1.RuleWithOperations{ - arv1beta1.RuleWithOperations{ + { Operations: []arv1beta1.OperationType{arv1beta1.Create}, Rule: arv1beta1.Rule{ APIGroups: []string{""}, @@ -168,7 +169,8 @@ func createMutatingWebhookConfiguration(certificate []byte) error { }, }, } - _, err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(context.TODO(), configuration, metav1.CreateOptions{}) + _, err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(context.TODO(), configuration, + metav1.CreateOptions{}) return err } @@ -211,10 +213,12 @@ func removeServiceIfExists(serviceName string) { } func removeMutatingWebhookIfExists(configName string) { - config, err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(context.TODO(), configName, metav1.GetOptions{}) + config, err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(context.TODO(), configName, + metav1.GetOptions{}) if config != nil && err == nil { glog.Infof("mutating webhook %s already exists, removing it first", configName) - err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(context.TODO(), configName, metav1.DeleteOptions{}) + err := clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(context.TODO(), configName, + metav1.DeleteOptions{}) if err != nil { glog.Errorf("error trying to remove mutating webhook configuration: %s", err) } @@ -238,14 +242,14 @@ func Install(k8sNamespace, namePrefix string) { prefix = namePrefix /* generate CSR and private key */ - csr, key, err := generateCSR() + certSigReq, key, err := generateCSR() if err != nil { glog.Fatalf("error generating CSR and private key: %s", err) } glog.Infof("raw CSR and private key successfully created") /* obtain signed certificate */ - certificate, err := getSignedCertificate(csr) + certificate, err := getSignedCertificate(certSigReq) if err != nil { glog.Fatalf("error getting signed certificate: %s", err) } diff --git a/pkg/webhook/tlsutils.go b/pkg/webhook/tlsutils.go index 8584617b..c579318b 100644 --- a/pkg/webhook/tlsutils.go +++ b/pkg/webhook/tlsutils.go @@ -83,7 +83,7 @@ func NewTLSKeypairReloader(certPath, keyPath string) (*tlsKeypairReloader, error return result, nil } -//NewClientCertPool will load a single client CA +// NewClientCertPool will load a single client CA func NewClientCertPool(clientCaPaths *ClientCAFlags, insecure bool) (*clientCertPool, error) { pool := &clientCertPool{ certPaths: clientCaPaths, @@ -97,7 +97,7 @@ func NewClientCertPool(clientCaPaths *ClientCAFlags, insecure bool) (*clientCert return pool, nil } -//Load a certificate into the client CA pool +// Load a certificate into the client CA pool func (pool *clientCertPool) Load() error { if pool.insecure { glog.Infof("can not load client CA pool. Remove --insecure flag to enable.") @@ -123,7 +123,7 @@ func (pool *clientCertPool) Load() error { return nil } -//GetCertPool returns a client CA pool +// GetCertPool returns a client CA pool func (pool *clientCertPool) GetCertPool() *x509.CertPool { if pool.insecure { return nil @@ -131,7 +131,7 @@ func (pool *clientCertPool) GetCertPool() *x509.CertPool { return pool.certPool } -//GetClientAuth determines the policy the http server will follow for TLS Client Authentication +// GetClientAuth determines the policy the http server will follow for TLS Client Authentication func GetClientAuth(insecure bool) tls.ClientAuthType { if insecure { return tls.NoClientCert diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index b98cdec1..82e13e3f 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -64,6 +64,8 @@ const ( networksAnnotationKey = "k8s.v1.cni.cncf.io/networks" nodeSelectorKey = "k8s.v1.cni.cncf.io/nodeSelector" defaultNetworkAnnotationKey = "v1.multus-cni.io/default-network" + annotPath = "/metadata/annotations" + oneMb = 1 << 20 ) var ( @@ -94,14 +96,14 @@ func readAdmissionReview(req *http.Request, w http.ResponseWriter) (*v1beta1.Adm var body []byte if req.Body != nil { - req.Body = http.MaxBytesReader(w, req.Body, 1<<20) + req.Body = http.MaxBytesReader(w, req.Body, oneMb) if data, err := ioutil.ReadAll(req.Body); err == nil { body = data } } if len(body) == 0 { - err := errors.New("Error reading HTTP request: empty body") + err := errors.New("error reading HTTP request: empty body") glog.Errorf("%s", err) return nil, http.StatusBadRequest, err } @@ -109,7 +111,7 @@ func readAdmissionReview(req *http.Request, w http.ResponseWriter) (*v1beta1.Adm /* validate HTTP request headers */ contentType := req.Header.Get("Content-Type") if contentType != "application/json" { - err := errors.Errorf("Invalid Content-Type='%s', expected 'application/json'", contentType) + err := errors.Errorf("invalid Content-Type='%s', expected 'application/json'", contentType) glog.Errorf("%v", err) return nil, http.StatusUnsupportedMediaType, err } @@ -241,7 +243,7 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s err = errors.New("pod namespace is not found") } - return + return namespace, err } func toSafeJSONPatchKey(in string) string { @@ -253,7 +255,7 @@ func toSafeJSONPatchKey(in string) string { func parsePodNetworkSelections(podNetworks, defaultNamespace string) ([]*multus.NetworkSelectionElement, error) { var networkSelections []*multus.NetworkSelectionElement - if len(podNetworks) == 0 { + if podNetworks == "" { err := errors.New("empty string passed as network selection elements list") glog.Error(err) return nil, err @@ -330,7 +332,7 @@ func parsePodNetworkSelectionElement(selection, defaultNamespace string) (*multu return networkSelectionElement, err } - validNameRegex, _ := regexp.Compile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) + validNameRegex := regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) for _, unit := range []string{namespace, name, netInterface} { ok := validNameRegex.MatchString(unit) if !ok && len(unit) > 0 { @@ -364,7 +366,8 @@ func getNetworkAttachmentDefinition(namespace, name string) (*cniv1.NetworkAttac return &networkAttachmentDefinition, err } -func parseNetworkAttachDefinition(net *multus.NetworkSelectionElement, reqs map[string]int64, nsMap map[string]string) (map[string]int64, map[string]string, error) { +func parseNetworkAttachDefinition(net *multus.NetworkSelectionElement, reqs map[string]int64, nsMap map[string]string) ( + map[string]int64, map[string]string, error) { /* for each network in annotation ask API server for network-attachment-definition */ networkAttachmentDefinition, err := getNetworkAttachmentDefinition(net.Namespace, net.Name) if err != nil { @@ -492,7 +495,6 @@ func addVolDownwardAPI(patch []jsonPatchOperation, hugepageResourceList []hugepa } func addVolumeMount(patch []jsonPatchOperation, containersLen int) []jsonPatchOperation { - vm := corev1.VolumeMount{ Name: "podnetinfo", ReadOnly: true, @@ -565,7 +567,7 @@ func createEnvPatch(patch []jsonPatchOperation, container *corev1.Container, return patch } -func createNodeSelectorPatch(patch []jsonPatchOperation, existing map[string]string, desired map[string]string) []jsonPatchOperation { +func createNodeSelectorPatch(patch []jsonPatchOperation, existing, desired map[string]string) []jsonPatchOperation { targetMap := make(map[string]string) for k, v := range existing { @@ -587,21 +589,22 @@ func createNodeSelectorPatch(patch []jsonPatchOperation, existing map[string]str return patch } -func createResourcePatch(patch []jsonPatchOperation, Containers []corev1.Container, resourceRequests map[string]int64) []jsonPatchOperation { +func createResourcePatch(patch []jsonPatchOperation, containers []corev1.Container, + resourceRequests map[string]int64) []jsonPatchOperation { /* check whether resources paths exists in the first container and add as the first patches if missing */ - if len(Containers[0].Resources.Requests) == 0 { + if len(containers[0].Resources.Requests) == 0 { patch = patchEmptyResources(patch, 0, "requests") } - if len(Containers[0].Resources.Limits) == 0 { + if len(containers[0].Resources.Limits) == 0 { patch = patchEmptyResources(patch, 0, "limits") } for resourceName := range resourceRequests { - for _, container := range Containers { - if _, exists := container.Resources.Limits[corev1.ResourceName(resourceName)]; exists { + for i := range containers { + if _, exists := containers[i].Resources.Limits[corev1.ResourceName(resourceName)]; exists { delete(resourceRequests, resourceName) } - if _, exists := container.Resources.Requests[corev1.ResourceName(resourceName)]; exists { + if _, exists := containers[i].Resources.Requests[corev1.ResourceName(resourceName)]; exists { delete(resourceRequests, resourceName) } } @@ -616,19 +619,20 @@ func createResourcePatch(patch []jsonPatchOperation, Containers []corev1.Contain return patch } -func updateResourcePatch(patch []jsonPatchOperation, Containers []corev1.Container, resourceRequests map[string]int64) []jsonPatchOperation { +func updateResourcePatch(patch []jsonPatchOperation, containers []corev1.Container, + resourceRequests map[string]int64) []jsonPatchOperation { var existingrequestsMap map[corev1.ResourceName]resource.Quantity var existingLimitsMap map[corev1.ResourceName]resource.Quantity - if len(Containers[0].Resources.Requests) == 0 { + if len(containers[0].Resources.Requests) == 0 { patch = patchEmptyResources(patch, 0, "requests") } else { - existingrequestsMap = Containers[0].Resources.Requests + existingrequestsMap = containers[0].Resources.Requests } - if len(Containers[0].Resources.Limits) == 0 { + if len(containers[0].Resources.Limits) == 0 { patch = patchEmptyResources(patch, 0, "limits") } else { - existingLimitsMap = Containers[0].Resources.Limits + existingLimitsMap = containers[0].Resources.Limits } resourceList := *getResourceList(resourceRequests) @@ -653,12 +657,12 @@ func appendResource(patch []jsonPatchOperation, resourceName string, reqQuantity Operation: "add", Path: "/spec/containers/0/resources/requests/" + toSafeJSONPatchKey(resourceName), Value: reqQuantity, - }) - patch = append(patch, jsonPatchOperation{ - Operation: "add", - Path: "/spec/containers/0/resources/limits/" + toSafeJSONPatchKey(resourceName), - Value: limitQuantity, - }) + }, + jsonPatchOperation{ + Operation: "add", + Path: "/spec/containers/0/resources/limits/" + toSafeJSONPatchKey(resourceName), + Value: limitQuantity, + }) return patch } @@ -682,7 +686,7 @@ func appendPodAnnotation(patch []jsonPatchOperation, pod corev1.Pod, userDefined } patch = append(patch, jsonPatchOperation{ Operation: "add", - Path: "/metadata/annotations", + Path: annotPath, Value: annotMap, }) return patch @@ -699,7 +703,7 @@ func createCustomizedPatch(pod corev1.Pod) ([]jsonPatchOperation, error) { // The userDefinedInjects will be injected when: // 1. Pod labels contain the patch key defined in userDefinedInjects, and // 2. The value of patch key in pod labels(not in userDefinedInjects) is "true" - if podValue, exists := pod.ObjectMeta.Labels[k]; exists && strings.ToLower(podValue) == "true" { + if podValue, exists := pod.ObjectMeta.Labels[k]; exists && strings.EqualFold(podValue, "true") { userDefinedPatch = append(userDefinedPatch, v) } } @@ -708,7 +712,7 @@ func createCustomizedPatch(pod corev1.Pod) ([]jsonPatchOperation, error) { func appendCustomizedPatch(patch []jsonPatchOperation, pod corev1.Pod, userDefinedPatch []jsonPatchOperation) []jsonPatchOperation { for _, p := range userDefinedPatch { - if p.Path == "/metadata/annotations" { + if p.Path == annotPath { patch = appendPodAnnotation(patch, pod, p) } } @@ -728,7 +732,7 @@ func getNetworkSelections(annotationKey string, pod corev1.Pod, userDefinedPatch // userDefinedPatch may contain user defined net-attach-defs if len(userDefinedPatch) > 0 { for _, p := range userDefinedPatch { - if p.Operation == "add" && p.Path == "/metadata/annotations" { + if p.Operation == "add" && p.Path == annotPath { for k, v := range p.Value.(map[string]interface{}) { if k == annotationKey { glog.Infof("%s is found in user-defined annotations", annotationKey) @@ -983,7 +987,7 @@ func SetCustomizedInjections(injections *corev1.ConfigMap) { } // metadata.Annotations is the only supported field for user definition // jsonPatchOperation.Path should be "/metadata/annotations" - if patch.Path != "/metadata/annotations" { + if patch.Path != annotPath { glog.Errorf("Path: %v is not supported, only /metadata/annotations can be defined by user", patch.Path) continue } diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index ab28f737..54b14f0e 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -86,7 +86,7 @@ var _ = Describe("Webhook", func() { Describe("Writing a response", func() { Context("with an AdmissionReview", func() { - It("should be marshalled and written to a HTTP Response Writer", func() { + It("should be marshaled and written to a HTTP Response Writer", func() { w := httptest.NewRecorder() ar := &v1beta1.AdmissionReview{} ar.Response = &v1beta1.AdmissionResponse{ @@ -130,7 +130,7 @@ var _ = Describe("Webhook", func() { func(pod corev1.Pod, userDefinedInjectPatchs map[string]jsonPatchOperation, out []jsonPatchOperation) { userDefinedInjects.Patchs = userDefinedInjectPatchs - appliedPatchs, _ := createCustomizedPatch(pod) + appliedPatchs := createCustomizedPatch(&pod) Expect(appliedPatchs).Should(Equal(out)) }, Entry( @@ -143,7 +143,7 @@ var _ = Describe("Webhook", func() { Spec: corev1.PodSpec{}, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, @@ -167,7 +167,7 @@ var _ = Describe("Webhook", func() { Spec: corev1.PodSpec{}, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, @@ -185,7 +185,7 @@ var _ = Describe("Webhook", func() { Spec: corev1.PodSpec{}, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, @@ -198,7 +198,7 @@ var _ = Describe("Webhook", func() { DescribeTable("Get network selections", func(annotateKey string, pod corev1.Pod, patchs []jsonPatchOperation, out string, shouldExist bool) { - nets, exist := getNetworkSelections(annotateKey, pod, patchs) + nets, exist := getNetworkSelections(annotateKey, &pod, patchs) Expect(exist).To(Equal(shouldExist)) Expect(nets).Should(Equal(out)) }, @@ -279,14 +279,15 @@ var _ = Describe("Webhook", func() { map[string]jsonPatchOperation{}, ), Entry( - "patch - addtional networks annotation", + "patch - additional networks annotation", &corev1.ConfigMap{ Data: map[string]string{ - "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": {\"k8s.v1.cni.cncf.io/networks\": \"sriov-net\"}}"}, + "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": " + + "{\"k8s.v1.cni.cncf.io/networks\": \"sriov-net\"}}"}, }, map[string]jsonPatchOperation{}, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"k8s.v1.cni.cncf.io/networks": "sriov-net"}, @@ -297,11 +298,12 @@ var _ = Describe("Webhook", func() { "patch - default network annotation", &corev1.ConfigMap{ Data: map[string]string{ - "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": {\"v1.multus-cni.io/default-network\": \"sriov-net\"}}"}, + "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": " + + "{\"v1.multus-cni.io/default-network\": \"sriov-net\"}}"}, }, map[string]jsonPatchOperation{}, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, @@ -312,7 +314,8 @@ var _ = Describe("Webhook", func() { "patch - non-annotation", &corev1.ConfigMap{ Data: map[string]string{ - "nri-inject-labels": "{\"op\": \"add\", \"path\": \"/metadata/labels\", \"value\": {\"v1.multus-cni.io/default-network\": \"sriov-net\"}}", + "nri-inject-labels": "{\"op\": \"add\", \"path\": \"/metadata/labels\", \"value\": " + + "{\"v1.multus-cni.io/default-network\": \"sriov-net\"}}", }, }, map[string]jsonPatchOperation{}, @@ -324,7 +327,7 @@ var _ = Describe("Webhook", func() { Data: map[string]string{}, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net"}, @@ -336,17 +339,18 @@ var _ = Describe("Webhook", func() { "patch - overwrite existing userDefinedInjects", &corev1.ConfigMap{ Data: map[string]string{ - "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": {\"v1.multus-cni.io/default-network\": \"sriov-net-new\"}}"}, + "nri-inject-annotation": "{\"op\": \"add\", \"path\": \"/metadata/annotations\", \"value\": " + + "{\"v1.multus-cni.io/default-network\": \"sriov-net-new\"}}"}, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-old"}, }, }, map[string]jsonPatchOperation{ - "nri-inject-annotation": jsonPatchOperation{ + "nri-inject-annotation": { Operation: "add", Path: "/metadata/annotations", Value: map[string]interface{}{"v1.multus-cni.io/default-network": "sriov-net-new"}, diff --git a/test/e2e/httpserver_test.go b/test/e2e/httpserver_test.go index 9a381d4c..cf9e1362 100644 --- a/test/e2e/httpserver_test.go +++ b/test/e2e/httpserver_test.go @@ -22,7 +22,7 @@ var _ = Describe("Network injection testing", func() { }) AfterEach(func() { - util.DeletePod(cs.CoreV1Interface, pod, timeout) + _ = util.DeletePod(cs.CoreV1Interface, pod, timeout) }) It("should have one limit injected", func() { @@ -45,7 +45,7 @@ var _ = Describe("Network injection testing", func() { }) AfterEach(func() { - util.DeletePod(cs.CoreV1Interface, pod, timeout) + _ = util.DeletePod(cs.CoreV1Interface, pod, timeout) }) It("should have two limits injected", func() { diff --git a/test/util/images.go b/test/util/images.go index fc91e2b4..c08e0b04 100644 --- a/test/util/images.go +++ b/test/util/images.go @@ -21,7 +21,7 @@ func init() { } } -//GetPodTestImage returns image to be used during testing +// GetPodTestImage returns image to be used during testing func GetPodTestImage() string { return fmt.Sprintf("%s/%s", registry, testImage) } diff --git a/test/util/pod.go b/test/util/pod.go index 55c0495d..5e0e2523 100644 --- a/test/util/pod.go +++ b/test/util/pod.go @@ -12,7 +12,9 @@ import ( coreclient "k8s.io/client-go/kubernetes/typed/core/v1" ) -//CreateRunningPod create a pod and wait until it is running +const waitPodStartDelay = 30 * time.Second + +// CreateRunningPod create a pod and wait until it is running func CreateRunningPod(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout, interval time.Duration) error { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -27,7 +29,7 @@ func CreateRunningPod(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout, i return nil } -//DeletePod will delete a pod +// DeletePod will delete a pod func DeletePod(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout time.Duration) error { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -35,7 +37,7 @@ func DeletePod(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout time.Dura return err } -//UpdatePodInfo will get the current pod state and return it +// UpdatePodInfo will get the current pod state and return it func UpdatePodInfo(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout time.Duration) (*corev1.Pod, error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -46,7 +48,7 @@ func UpdatePodInfo(ci coreclient.CoreV1Interface, pod *corev1.Pod, timeout time. return pod, nil } -//GetPodDefinition will return a test pod +// GetPodDefinition will return a test pod func GetPodDefinition(ns string) *corev1.Pod { var graceTime int64 = 0 return &corev1.Pod{ @@ -67,23 +69,23 @@ func GetPodDefinition(ns string) *corev1.Pod { } } -//GetOneNetwork add one network to pod +// GetOneNetwork add one network to pod func GetOneNetwork(nad, ns string) *corev1.Pod { pod := GetPodDefinition(ns) pod.Annotations = map[string]string{"k8s.v1.cni.cncf.io/networks": nad} return pod } -//GetMultiNetworks adds a network to annotation +// GetMultiNetworks adds a network to annotation func GetMultiNetworks(nad []string, ns string) *corev1.Pod { pod := GetPodDefinition(ns) pod.Annotations = map[string]string{"k8s.v1.cni.cncf.io/networks": strings.Join(nad, ",")} return pod } -//WaitForPodStateRunning waits for pod to enter running state +// WaitForPodStateRunning waits for pod to enter running state func WaitForPodStateRunning(core coreclient.CoreV1Interface, podName, ns string, timeout, interval time.Duration) error { - time.Sleep(30 * time.Second) + time.Sleep(waitPodStartDelay) return wait.PollImmediate(interval, timeout, func() (done bool, err error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -96,7 +98,10 @@ func WaitForPodStateRunning(core coreclient.CoreV1Interface, podName, ns string, return true, nil case corev1.PodFailed, corev1.PodSucceeded: return false, errors.New("pod failed or succeeded but is not running") + case corev1.PodPending, corev1.PodUnknown: + return false, nil + default: + return false, nil } - return false, nil }) } From 4c0bd8fa222cd5614ad0b9ec2061c8fdb57e9547 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Fri, 4 Jun 2021 15:02:30 +0100 Subject: [PATCH 6/8] Expose webhook port as installer binary argument Previous to this change, a webhook port is configurable via the webhook binary argument, however, installer does not have a mechanism to change the webhook svc port for the webhook. This patch exposes this option to the user and sets a default as a shared type between webhook and installer. Signed-off-by: Kennelly, Martin --- cmd/installer/main.go | 17 ++++++++++++++--- cmd/webhook/main.go | 3 ++- pkg/installer/installer.go | 12 ++++++------ pkg/types/types.go | 2 ++ 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/cmd/installer/main.go b/cmd/installer/main.go index de614602..616d454a 100644 --- a/cmd/installer/main.go +++ b/cmd/installer/main.go @@ -16,15 +16,26 @@ package main import ( "flag" + "github.com/golang/glog" "github.com/k8snetworkplumbingwg/network-resources-injector/pkg/installer" + "github.com/k8snetworkplumbingwg/network-resources-injector/pkg/types" ) func main() { - namespace := flag.String("namespace", "kube-system", "Namespace in which all Kubernetes resources will be created.") - prefix := flag.String("name", "network-resources-injector", "Prefix added to the names of all created resources.") + namespace := flag.String("namespace", "kube-system", + "Namespace in which all Kubernetes resources will be created.") + prefix := flag.String("name", "network-resources-injector", + "Prefix added to the names of all created resources.") + webhookPort := flag.Int("webhook-port", types.DefaultWebhookPort, "Port number which webhook will serve") + webhookSvcPort := flag.Int("webhook-service-port", types.DefaultServicePort, "Port number for webhook service") + + if *webhookPort < 1024 || *webhookPort > 65535 { + glog.Fatalf("invalid port number. Choose between 1024 and 65535") + } + flag.Parse() glog.Info("starting webhook installation") - installer.Install(*namespace, *prefix) + installer.Install(*namespace, *prefix, *webhookPort, *webhookSvcPort) } diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 92ceb95b..31235cf8 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -25,6 +25,7 @@ import ( "github.com/fsnotify/fsnotify" "github.com/golang/glog" + "github.com/k8snetworkplumbingwg/network-resources-injector/pkg/types" "github.com/k8snetworkplumbingwg/network-resources-injector/pkg/webhook" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -40,7 +41,7 @@ func main() { var namespace string var clientCAPaths webhook.ClientCAFlags /* load configuration */ - port := flag.Int("port", 8443, "The port on which to serve.") + port := flag.Int("port", types.DefaultWebhookPort, "The port on which to serve.") address := flag.String("bind-address", "0.0.0.0", "The IP address on which to listen for the --port port.") cert := flag.String("tls-cert-file", "cert.pem", "File containing the default x509 Certificate for HTTPS.") key := flag.String("tls-private-key-file", "key.pem", diff --git a/pkg/installer/installer.go b/pkg/installer/installer.go index b584552f..8750df44 100644 --- a/pkg/installer/installer.go +++ b/pkg/installer/installer.go @@ -174,7 +174,7 @@ func createMutatingWebhookConfiguration(certificate []byte) error { return err } -func createService() error { +func createService(webhookPort, webhookSvcPort int) error { serviceName := strings.Join([]string{prefix, "service"}, "-") removeServiceIfExists(serviceName) service := &corev1.Service{ @@ -186,9 +186,9 @@ func createService() error { }, Spec: corev1.ServiceSpec{ Ports: []corev1.ServicePort{ - corev1.ServicePort{ - Port: 443, - TargetPort: intstr.FromInt(8443), + { + Port: int32(webhookSvcPort), + TargetPort: intstr.FromInt(webhookPort), }, }, Selector: map[string]string{ @@ -227,7 +227,7 @@ func removeMutatingWebhookIfExists(configName string) { } // Install creates resources required by mutating admission webhook -func Install(k8sNamespace, namePrefix string) { +func Install(k8sNamespace, namePrefix string, webhookPort, webhookSvcPort int) { /* setup Kubernetes API client */ config, err := rest.InClusterConfig() if err != nil { @@ -269,7 +269,7 @@ func Install(k8sNamespace, namePrefix string) { glog.Infof("mutating webhook configuration successfully created") /* create service */ - err = createService() + err = createService(webhookPort, webhookSvcPort) if err != nil { glog.Fatalf("error creating service: %s", err) } diff --git a/pkg/types/types.go b/pkg/types/types.go index a085c802..27f370e5 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -23,4 +23,6 @@ const ( Hugepages2MRequestPath = "hugepages_2M_request" Hugepages1GLimitPath = "hugepages_1G_limit" Hugepages2MLimitPath = "hugepages_2M_limit" + DefaultWebhookPort = 8443 + DefaultServicePort = 443 ) From 9da205138c9ced5746590c7d53be16ed0079d888 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Fri, 4 Jun 2021 16:21:06 +0100 Subject: [PATCH 7/8] Optimise accessing data types In order, to gain a small performance boost in terms of memory and cpu cycles, we access the data structure using indexing or pointers instead of copying data when we loop over large data structures. Signed-off-by: Kennelly, Martin --- pkg/webhook/webhook.go | 106 ++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 82e13e3f..e84311bb 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -149,10 +149,10 @@ func deserializeNetworkAttachmentDefinition(ar *v1beta1.AdmissionReview) (cniv1. return netAttachDef, err } -func deserializePod(ar *v1beta1.AdmissionReview) (corev1.Pod, error) { +func deserializePod(ar *v1beta1.AdmissionReview) (*corev1.Pod, error) { /* unmarshal Pod from AdmissionReview request */ - pod := corev1.Pod{} - err := json.Unmarshal(ar.Request.Object.Raw, &pod) + pod := &corev1.Pod{} + err := json.Unmarshal(ar.Request.Object.Raw, pod) if pod.ObjectMeta.Namespace != "" { return pod, err } @@ -165,7 +165,7 @@ func deserializePod(ar *v1beta1.AdmissionReview) (corev1.Pod, error) { ownerRef := pod.ObjectMeta.OwnerReferences if len(ownerRef) > 0 { - namespace, err := getNamespaceFromOwnerReference(pod.ObjectMeta.OwnerReferences[0]) + namespace, err := getNamespaceFromOwnerReference(&pod.ObjectMeta.OwnerReferences[0]) if err != nil { return pod, err } @@ -178,7 +178,7 @@ func deserializePod(ar *v1beta1.AdmissionReview) (corev1.Pod, error) { return pod, err } -func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace string, err error) { +func getNamespaceFromOwnerReference(ownerRef *metav1.OwnerReference) (namespace string, err error) { namespace = "" switch ownerRef.Kind { case "ReplicaSet": @@ -187,9 +187,9 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s if err != nil { return } - for _, replicaSet := range replicaSets.Items { - if replicaSet.ObjectMeta.Name == ownerRef.Name && replicaSet.ObjectMeta.UID == ownerRef.UID { - namespace = replicaSet.ObjectMeta.Namespace + for i := range replicaSets.Items { + if replicaSets.Items[i].ObjectMeta.Name == ownerRef.Name && replicaSets.Items[i].ObjectMeta.UID == ownerRef.UID { + namespace = replicaSets.Items[i].ObjectMeta.Namespace err = nil break } @@ -200,9 +200,9 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s if err != nil { return } - for _, daemonSet := range daemonSets.Items { - if daemonSet.ObjectMeta.Name == ownerRef.Name && daemonSet.ObjectMeta.UID == ownerRef.UID { - namespace = daemonSet.ObjectMeta.Namespace + for i := range daemonSets.Items { + if daemonSets.Items[i].ObjectMeta.Name == ownerRef.Name && daemonSets.Items[i].ObjectMeta.UID == ownerRef.UID { + namespace = daemonSets.Items[i].ObjectMeta.Namespace err = nil break } @@ -213,9 +213,9 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s if err != nil { return } - for _, statefulSet := range statefulSets.Items { - if statefulSet.ObjectMeta.Name == ownerRef.Name && statefulSet.ObjectMeta.UID == ownerRef.UID { - namespace = statefulSet.ObjectMeta.Namespace + for i := range statefulSets.Items { + if statefulSets.Items[i].ObjectMeta.Name == ownerRef.Name && statefulSets.Items[i].ObjectMeta.UID == ownerRef.UID { + namespace = statefulSets.Items[i].ObjectMeta.Namespace err = nil break } @@ -226,9 +226,10 @@ func getNamespaceFromOwnerReference(ownerRef metav1.OwnerReference) (namespace s if err != nil { return } - for _, replicationController := range replicationControllers.Items { - if replicationController.ObjectMeta.Name == ownerRef.Name && replicationController.ObjectMeta.UID == ownerRef.UID { - namespace = replicationController.ObjectMeta.Namespace + for i := range replicationControllers.Items { + if replicationControllers.Items[i].ObjectMeta.Name == ownerRef.Name && + replicationControllers.Items[i].ObjectMeta.UID == ownerRef.UID { + namespace = replicationControllers.Items[i].ObjectMeta.Namespace err = nil break } @@ -517,8 +518,7 @@ func createVolPatch(patch []jsonPatchOperation, hugepageResourceList []hugepageR return patch } -func addEnvVar(patch []jsonPatchOperation, containerIndex int, firstElement bool, - envName string, envVal string) []jsonPatchOperation { +func addEnvVar(patch []jsonPatchOperation, containerIndex int, firstElement bool, envName, envVal string) []jsonPatchOperation { env := corev1.EnvVar{ Name: envName, Value: envVal, @@ -541,13 +541,12 @@ func addEnvVar(patch []jsonPatchOperation, containerIndex int, firstElement bool return patch } -func createEnvPatch(patch []jsonPatchOperation, container *corev1.Container, - containerIndex int, envName string, envVal string) []jsonPatchOperation { +func createEnvPatch(patch []jsonPatchOperation, envVar []corev1.EnvVar, containerIndex int, envName, envVal string) []jsonPatchOperation { // Determine if requested ENV already exists found := false firstElement := false - if len(container.Env) != 0 { - for _, env := range container.Env { + if len(envVar) != 0 { + for _, env := range envVar { if env.Name == envName { found = true if env.Value != envVal { @@ -676,7 +675,7 @@ func getResourceList(resourceRequests map[string]int64) *corev1.ResourceList { return &resourceList } -func appendPodAnnotation(patch []jsonPatchOperation, pod corev1.Pod, userDefinedPatch jsonPatchOperation) []jsonPatchOperation { +func appendPodAnnotation(patch []jsonPatchOperation, pod *corev1.Pod, userDefinedPatch jsonPatchOperation) []jsonPatchOperation { annotMap := make(map[string]string) for k, v := range pod.ObjectMeta.Annotations { annotMap[k] = v @@ -692,7 +691,7 @@ func appendPodAnnotation(patch []jsonPatchOperation, pod corev1.Pod, userDefined return patch } -func createCustomizedPatch(pod corev1.Pod) ([]jsonPatchOperation, error) { +func createCustomizedPatch(pod *corev1.Pod) []jsonPatchOperation { var userDefinedPatch []jsonPatchOperation // lock for reading @@ -707,10 +706,10 @@ func createCustomizedPatch(pod corev1.Pod) ([]jsonPatchOperation, error) { userDefinedPatch = append(userDefinedPatch, v) } } - return userDefinedPatch, nil + return userDefinedPatch } -func appendCustomizedPatch(patch []jsonPatchOperation, pod corev1.Pod, userDefinedPatch []jsonPatchOperation) []jsonPatchOperation { +func appendCustomizedPatch(patch []jsonPatchOperation, pod *corev1.Pod, userDefinedPatch []jsonPatchOperation) []jsonPatchOperation { for _, p := range userDefinedPatch { if p.Path == annotPath { patch = appendPodAnnotation(patch, pod, p) @@ -719,7 +718,7 @@ func appendCustomizedPatch(patch []jsonPatchOperation, pod corev1.Pod, userDefin return patch } -func getNetworkSelections(annotationKey string, pod corev1.Pod, userDefinedPatch []jsonPatchOperation) (string, bool) { +func getNetworkSelections(annotationKey string, pod *corev1.Pod, userDefinedPatch []jsonPatchOperation) (string, bool) { // User defined annotateKey takes precedence than userDefined injections glog.Infof("search %s in original pod annotations", annotationKey) nets, exists := pod.ObjectMeta.Annotations[annotationKey] @@ -767,11 +766,7 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { } glog.Infof("AdmissionReview request received for pod %s/%s", pod.ObjectMeta.Namespace, pod.ObjectMeta.Name) - userDefinedPatch, err := createCustomizedPatch(pod) - if err != nil { - glog.Warningf("failed to create user-defined injection patch for pod %s/%s, err: %v", - pod.ObjectMeta.Namespace, pod.ObjectMeta.Name, err) - } + userDefinedPatch := createCustomizedPatch(pod) defaultNetSelection, defExist := getNetworkSelections(defaultNetworkAnnotationKey, pod, userDefinedPatch) additionalNetSelections, addExists := getNetworkSelections(networksAnnotationKey, pod, userDefinedPatch) @@ -850,46 +845,52 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { // Determine if hugepages are being requested for a given container, // and if so, expose the value to the container via Downward API. - var hugepageResourceList []hugepageResourceData + var ( + hugepageResourceList []hugepageResourceData + res corev1.ResourceRequirements + name string + ) glog.Infof("injectHugepageDownAPI=%v", injectHugepageDownAPI) if injectHugepageDownAPI { - for containerIndex, container := range pod.Spec.Containers { + for i := range pod.Spec.Containers { found := false - if len(container.Resources.Requests) != 0 { - if quantity, exists := container.Resources.Requests["hugepages-1Gi"]; exists && !quantity.IsZero() { + res = pod.Spec.Containers[i].Resources + name = pod.Spec.Containers[i].Name + if len(res.Requests) != 0 { + if quantity, exists := res.Requests["hugepages-1Gi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "requests.hugepages-1Gi", - ContainerName: container.Name, - Path: types.Hugepages1GRequestPath + "_" + container.Name, + ContainerName: name, + Path: types.Hugepages1GRequestPath + "_" + name, } hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true } - if quantity, exists := container.Resources.Requests["hugepages-2Mi"]; exists && !quantity.IsZero() { + if quantity, exists := res.Requests["hugepages-2Mi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "requests.hugepages-2Mi", - ContainerName: container.Name, - Path: types.Hugepages2MRequestPath + "_" + container.Name, + ContainerName: name, + Path: types.Hugepages2MRequestPath + "_" + name, } hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true } } - if len(container.Resources.Limits) != 0 { - if quantity, exists := container.Resources.Limits["hugepages-1Gi"]; exists && !quantity.IsZero() { + if len(res.Limits) != 0 { + if quantity, exists := res.Limits["hugepages-1Gi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "limits.hugepages-1Gi", - ContainerName: container.Name, - Path: types.Hugepages1GLimitPath + "_" + container.Name, + ContainerName: name, + Path: types.Hugepages1GLimitPath + "_" + name, } hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true } - if quantity, exists := container.Resources.Limits["hugepages-2Mi"]; exists && !quantity.IsZero() { + if quantity, exists := res.Limits["hugepages-2Mi"]; exists && !quantity.IsZero() { hugepageResource := hugepageResourceData{ ResourceName: "limits.hugepages-2Mi", - ContainerName: container.Name, - Path: types.Hugepages2MLimitPath + "_" + container.Name, + ContainerName: name, + Path: types.Hugepages2MLimitPath + "_" + name, } hugepageResourceList = append(hugepageResourceList, hugepageResource) found = true @@ -900,12 +901,11 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) { // 'container.Name' as an environment variable to the container // so container knows its name and can process hugepages properly. if found { - patch = createEnvPatch(patch, &container, containerIndex, - types.EnvNameContainerName, container.Name) + patch = createEnvPatch(patch, pod.Spec.Containers[i].Env, i, types.EnvNameContainerName, name) } } } - patch = createVolPatch(patch, hugepageResourceList, &pod) + patch = createVolPatch(patch, hugepageResourceList, pod) patch = appendCustomizedPatch(patch, pod, userDefinedPatch) } patch = createNodeSelectorPatch(patch, pod.Spec.NodeSelector, desiredNsMap) @@ -997,7 +997,7 @@ func SetCustomizedInjections(injections *corev1.ConfigMap) { } } // remove stale entries from userDefined configMap - for k, _ := range userDefinedPatchs { + for k := range userDefinedPatchs { if _, ok := injections.Data[k]; ok { continue } From 249d7a47cb7a5407e9a656fa067c5d8b4f28f024 Mon Sep 17 00:00:00 2001 From: "Kennelly, Martin" Date: Fri, 4 Jun 2021 17:27:03 +0100 Subject: [PATCH 8/8] Depreciate golint from golanglint-ci Golint is archived and unmaintained. Signed-off-by: Kennelly, Martin --- .golangci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index b20b958d..a0af54d6 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -72,7 +72,7 @@ linters: - gocyclo - gofmt - goimports - - golint + #- golint - gomnd - goprintffuncname - gosec