From bb897444f80ecb975f604e91e9a6a3b126822579 Mon Sep 17 00:00:00 2001 From: Richard Kovacs Date: Mon, 18 Dec 2023 23:23:25 +0100 Subject: [PATCH] Introduce static code analysis --- .github/workflows/build_test_ci.yml | 94 ++++++- .github/workflows/codeql.yml | 69 +++++ .golangci.yml | 262 ++++++++++++++++++ .husky/hooks/pre-push | 2 +- Makefile | 32 ++- api/v1alpha1/linodemachine_types.go | 11 +- cloud/scope/cluster.go | 11 +- cloud/scope/machine.go | 6 +- cmd/main.go | 2 +- controller/linodemachine_controller.go | 183 ++++++------ .../linodemachine_controller_helpers.go | 14 +- controller/linodemachine_controller_test.go | 5 +- controller/suite_test.go | 2 + 13 files changed, 589 insertions(+), 104 deletions(-) create mode 100644 .github/workflows/codeql.yml create mode 100644 .golangci.yml diff --git a/.github/workflows/build_test_ci.yml b/.github/workflows/build_test_ci.yml index 98451b576..fb3349eb1 100644 --- a/.github/workflows/build_test_ci.yml +++ b/.github/workflows/build_test_ci.yml @@ -6,11 +6,31 @@ on: pull_request: branches: [ "main" ] -jobs: +permissions: + contents: read + pull-requests: read + actions: read + +concurrency: + group: build-test-ci-${{ github.ref }}-1 + cancel-in-progress: true +jobs: go-build-test: runs-on: ubuntu-latest steps: + - name: Harden Runner + uses: step-security/harden-runner@v2 + with: + disable-sudo: true + egress-policy: block + allowed-endpoints: > + api.github.com:443 + github.com:443 + proxy.golang.org:443 + sum.golang.org:443 + objects.githubusercontent.com:443 + - uses: actions/checkout@v4 - name: Set up Go @@ -24,9 +44,81 @@ jobs: - name: Test run: make test + go-analyse: + needs: go-build-test + runs-on: ubuntu-latest + steps: + - name: Harden Runner + uses: step-security/harden-runner@v2 + with: + disable-sudo: true + egress-policy: block + allowed-endpoints: > + api.github.com:443 + github.com:443 + proxy.golang.org:443 + sum.golang.org:443 + objects.githubusercontent.com:443 + registry-1.docker.io:443 + auth.docker.io:443 + production.cloudflare.docker.com:443 + vuln.go.dev:443 + + - uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 'stable' + + - name: Docker cache + uses: ScribeMD/docker-cache@0.3.7 + with: + key: docker-${{ runner.os }}-${{ hashFiles('Makefile') }}} + + - name: Lint + run: make lint + + - name: Gosec + run: make gosec + + # Retry is a workaround, because sometimes nilaway randomly killed. + - uses: nick-fields/retry@v2 + with: + timeout_minutes: 5 + max_attempts: 3 + warning_on_retry: false + command: make nilcheck + + - name: Vulncheck + run: make vulncheck + docker-build: runs-on: ubuntu-latest steps: + - name: Harden Runner + uses: step-security/harden-runner@v2 + with: + disable-sudo: true + egress-policy: block + allowed-endpoints: > + api.github.com:443 + github.com:443 + proxy.golang.org:443 + sum.golang.org:443 + objects.githubusercontent.com:443 + registry-1.docker.io:443 + auth.docker.io:443 + production.cloudflare.docker.com:443 + gcr.io:443 + storage.googleapis.com:443 + - uses: actions/checkout@v4 + + - name: Docker cache + uses: ScribeMD/docker-cache@0.3.7 + with: + key: docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }} + - name: Build the Docker image run: make docker-build diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 000000000..de963e742 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,69 @@ +name: "CodeQL" + +on: + push: + branches: [ "main" ] + pull_request: + # The branches below must be a subset of the branches above + branches: [ "main" ] + +permissions: + contents: read + +concurrency: + group: codeql-${{ github.ref }}-1 + cancel-in-progress: true + +jobs: + analyze: + name: Analyze + runs-on: ubuntu-latest + permissions: + actions: read + contents: read + security-events: write + + strategy: + fail-fast: false + + steps: + - name: Harden Runner + uses: step-security/harden-runner@v2 + with: + disable-sudo: true + egress-policy: block + allowed-endpoints: > + api.github.com:443 + github.com:443 + proxy.golang.org:443 + sum.golang.org:443 + objects.githubusercontent.com:443 + + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 'stable' + + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v2 + with: + # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support + languages: go + # If you wish to specify custom queries, you can do so here or in a config file. + # By default, queries listed here will override any specified in a config file. + # Prefix the list here with "+" to use these queries and those in the config file. + + # Details on CodeQL's query packs refer to: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs + # queries: security-extended,security-and-quality + + - name: Build + run: make build + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v2 + with: + category: "/language:go" diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..89e2a8d1d --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,262 @@ +run: + timeout: 5m + + skip-files: + - "zz_generated\\..+\\.go$" + + issues-exit-code: 1 + +output: + # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number" + format: colored-line-number + +linters-settings: + errcheck: + # report about not checking of errors in type assetions: `a := b.(MyStruct)`; + # default is false: such cases aren't reported by default. + check-type-assertions: true + + # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; + # default is false: such cases aren't reported by default. + check-blank: true + + # [deprecated] comma-separated list of pairs of the form pkg:regex + # the regex is used to ignore names within pkg. (default "fmt:.*"). + # see https://github.com/kisielk/errcheck#the-deprecated-method for details + ignore: fmt:.*,io/ioutil:^Read.* + + govet: + # report about shadowed variables + check-shadowing: false + + golint: + # minimal confidence for issues, default is 0.8 + min-confidence: 0.8 + + gofmt: + # simplify code: gofmt with `-s` option, true by default + simplify: true + + goimports: + # put imports beginning with prefix after 3rd-party packages; + # it's a comma-separated list of prefixes + local-prefixes: github.com/crossplane/provider-template + + gocyclo: + # minimal code complexity to report, 30 by default (but we recommend 10-20) + min-complexity: 15 + + cyclop: + max-complexity: 15 + + maligned: + # print struct with more effective memory layout or not, false by default + suggest-new: true + + dupl: + # tokens count to trigger issue, 150 by default + threshold: 100 + + goconst: + # minimal length of string constant, 3 by default + min-len: 3 + # minimal occurrences count to trigger, 3 by default + min-occurrences: 5 + + lll: + # tab width in spaces. Default to 1. + tab-width: 1 + + unused: + # treat code as a program (not a library) and report unused exported identifiers; default is false. + # XXX: if you enable this setting, unused will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find funcs usages. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + + unparam: + # Inspect exported functions, default is false. Set to true if no external program/library imports your code. + # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find external interfaces. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + + prealloc: + # XXX: we don't recommend using this linter before doing performance profiling. + # For most programs usage of prealloc will be a premature optimization. + + # 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 + + gocritic: + # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks. + # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". + enabled-tags: + - diagnostic + - experimental + - opinionated + - performance + - style + + # disabled-checks: + # - unnamedResult + # - hugeParam + + settings: # settings passed to gocritic + captLocal: # must be valid enabled check name + paramsOnly: true + rangeValCopy: + sizeThreshold: 32 + + nolintlint: + require-explanation: true + require-specific: true + +linters: + enable: + - asasalint + - asciicheck + - bidichk + - bodyclose + - containedctx + - contextcheck + - cyclop + - decorder + # - depguard + - dogsled + - dupl + - dupword + - durationcheck + - errchkjson + - errname + - errorlint + - errcheck + - exportloopref + - exhaustive + - exportloopref + - forbidigo + - forcetypeassert + # - funlen + # - gci + - gocheckcompilerdirectives + - gocognit + - goconst + - gocritic + # - godot + # - godox + # - goerr113 + - gofmt + - goimports + - gomnd + - gocyclo + - goprintffuncname + - gosec + - gosimple + - govet + - ineffassign + - loggercheck + - maintidx + - makezero + - misspell + - nestif + - nilerr + - nilnil + - nlreturn + - noctx + - nolintlint + - paralleltest + - prealloc + - predeclared + - reassign + # - revive + - staticcheck + # - stylecheck + - tenv + - thelper + - typecheck + - unconvert + - unparam + - unused + - usestdlibvars + - varnamelen + - whitespace + # - wrapcheck + + presets: + - bugs + - unused + fast: false + + +issues: + # Excluding configuration per-path and per-linter + exclude-rules: + # Exclude some linters from running on tests files. + - path: _test(ing)?\.go + linters: + - gocyclo + - errcheck + - dupl + - gosec + - exportloopref + - unparam + + # Ease some gocritic warnings on test files. + - path: _test\.go + text: "(unnamedResult|exitAfterDefer)" + linters: + - gocritic + + # These are performance optimisations rather than style issues per se. + # They warn when function arguments or range values copy a lot of memory + # rather than using a pointer. + - text: "(hugeParam|rangeValCopy):" + linters: + - gocritic + + # This "TestMain should call os.Exit to set exit code" warning is not clever + # enough to notice that we call a helper method that calls os.Exit. + - text: "SA3000:" + linters: + - staticcheck + + - text: "k8s.io/api/core/v1" + linters: + - goimports + + # This is a "potential hardcoded credentials" warning. It's triggered by + # any variable with 'secret' in the same, and thus hits a lot of false + # positives in Kubernetes land where a Secret is an object type. + - text: "G101:" + linters: + - gosec + - gas + + # This is an 'errors unhandled' warning that duplicates errcheck. + - text: "G104:" + linters: + - gosec + - gas + + # Independently from option `exclude` we use default exclude patterns, + # it can be disabled by this option. To list all + # excluded by default patterns execute `golangci-lint run --help`. + # Default value for this option is true. + exclude-use-default: false + + # Show only new issues: if there are unstaged changes or untracked files, + # only those changes are analyzed, else only changes in HEAD~ are analyzed. + # It's a super-useful option for integration of golangci-lint into existing + # large codebase. It's not practical to fix all existing issues at the moment + # of integration: much better don't allow issues in new code. + # Default is false. + new: false + + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-per-linter: 0 + + # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. + max-same-issues: 0 diff --git a/.husky/hooks/pre-push b/.husky/hooks/pre-push index e8cf01abd..49e0f6034 100755 --- a/.husky/hooks/pre-push +++ b/.husky/hooks/pre-push @@ -16,4 +16,4 @@ fi make generate manifests git diff --exit-code --quiet || (git status && exit 1) -make test +make lint gosec nilcheck test diff --git a/Makefile b/Makefile index 9f8f81eb2..7aab7407d 100644 --- a/Makefile +++ b/Makefile @@ -62,9 +62,25 @@ fmt: ## Run go fmt against code. vet: ## Run go vet against code. go vet ./... +.PHONY: gosec +gosec: ## Run gosec against code. + docker run --rm -w /workdir -v $(PWD):/workdir securego/gosec:2.18.2 -exclude-dir=bin -exclude-generated ./... + +.PHONY: lint +lint: ## Run lint against code. + docker run --rm -w /workdir -v $(PWD):/workdir golangci/golangci-lint:v1.55 golangci-lint run -c .golangci.yml + +.PHONY: nilcheck +nilcheck: nilaway ## Run nil check against code. + nilaway ./... + +.PHONY: vulncheck +vulncheck: govulncheck ## Run vulnerability check against code. + govulncheck ./... + .PHONY: test test: manifests generate fmt vet envtest ## Run tests. - KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test ./... -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -race -timeout 60s ./... -coverprofile cover.out ##@ Build @@ -151,6 +167,8 @@ TILT ?= $(LOCALBIN)/tilt KIND ?= $(LOCALBIN)/kind ENVTEST ?= $(LOCALBIN)/setup-envtest HUSKY ?= $(LOCALBIN)/husky +NILAWAY ?= $(LOCALBIN)/nilaway +GOVULNC ?= $(LOCALBIN)/govulncheck ## Tool Versions KUSTOMIZE_VERSION ?= v5.1.1 @@ -160,6 +178,8 @@ CONTROLLER_TOOLS_VERSION ?= v0.13.0 TILT_VERSION ?= 0.33.6 KIND_VERSION ?= 0.20.0 HUSKY_VERSION ?= v0.2.16 +NILAWAY_VERSION ?= latest +GOVULNC_VERSION ?= v1.0.1 .PHONY: kustomize kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary. If wrong version is installed, it will be removed before downloading. @@ -217,6 +237,16 @@ husky: $(HUSKY) ## Download husky locally if necessary. $(HUSKY): $(LOCALBIN) GOBIN=$(LOCALBIN) go install github.com/automation-co/husky@$(HUSKY_VERSION) +.PHONY: nilaway +nilaway: $(NILAWAY) ## Download nilaway locally if necessary. +$(NILAWAY): $(LOCALBIN) + GOBIN=$(LOCALBIN) go install go.uber.org/nilaway/cmd/nilaway@$(NILAWAY_VERSION) + +.PHONY: govulncheck +govulncheck: $(GOVULNC) ## Download govulncheck locally if necessary. +$(GOVULNC): $(LOCALBIN) + GOBIN=$(LOCALBIN) go install golang.org/x/vuln/cmd/govulncheck@$(GOVULNC_VERSION) + .PHONY: clean clean: rm -rf $(LOCALBIN) diff --git a/api/v1alpha1/linodemachine_types.go b/api/v1alpha1/linodemachine_types.go index ef07a3472..4b6b40693 100644 --- a/api/v1alpha1/linodemachine_types.go +++ b/api/v1alpha1/linodemachine_types.go @@ -29,8 +29,10 @@ import ( // LinodeMachineSpec defines the desired state of LinodeMachine type LinodeMachineSpec struct { // ProviderID is the unique identifier as specified by the cloud provider. + // +optional ProviderID *string `json:"providerID,omitempty"` // InstanceID is the Linode instance ID for this machine. + // +optional InstanceID *int `json:"instanceID,omitempty"` // +kubebuilder:validation:Required @@ -66,6 +68,7 @@ type LinodeMachineSpec struct { // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable" Tags []string `json:"tags,omitempty"` // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable" + // +optional Metadata *InstanceMetadataOptions `json:"metadata,omitempty"` // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable" FirewallID int `json:"firewallId,omitempty"` @@ -83,9 +86,11 @@ type InstanceConfigInterfaceCreateOptions struct { Label string `json:"label,omitempty"` Purpose linodego.ConfigInterfacePurpose `json:"purpose,omitempty"` Primary bool `json:"primary,omitempty"` - SubnetID *int `json:"subnetId,omitempty"` - IPv4 *VPCIPv4 `json:"ipv4,omitempty"` - IPRanges []string `json:"ipRanges,omitempty"` + // +optional + SubnetID *int `json:"subnetId,omitempty"` + // +optional + IPv4 *VPCIPv4 `json:"ipv4,omitempty"` + IPRanges []string `json:"ipRanges,omitempty"` } // VPCIPv4 defines VPC IPV4 settings diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index f58f8c3e1..99fbaa08b 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -43,10 +43,11 @@ func validateClusterScopeParams(params ClusterScopeParams) error { if params.LinodeCluster == nil { return errors.New("linodeCluster is required when creating a ClusterScope") } + return nil } -func createLinodeClient(apiKey string) (*linodego.Client, error) { +func createLinodeClient(apiKey string) *linodego.Client { tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: apiKey}) oauth2Client := &http.Client{ @@ -55,7 +56,8 @@ func createLinodeClient(apiKey string) (*linodego.Client, error) { }, } linodeClient := linodego.NewClient(oauth2Client) - return &linodeClient, nil + + return &linodeClient } // NewClusterScope creates a new Scope from the supplied parameters. @@ -65,10 +67,7 @@ func NewClusterScope(apiKey string, params ClusterScopeParams) (*ClusterScope, e return nil, err } - linodeClient, err := createLinodeClient(apiKey) - if err != nil { - return nil, err - } + linodeClient := createLinodeClient(apiKey) helper, err := patch.NewHelper(params.LinodeCluster, params.Client) if err != nil { diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index 5f66b7e33..ef2f9f8fe 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -43,6 +43,7 @@ func validateMachineScopeParams(params MachineScopeParams) error { if params.LinodeMachine == nil { return errors.New("linodeMachine is required when creating a MachineScope") } + return nil } @@ -51,10 +52,7 @@ func NewMachineScope(apiKey string, params MachineScopeParams) (*MachineScope, e return nil, err } - linodeClient, err := createLinodeClient(apiKey) - if err != nil { - return nil, err - } + linodeClient := createLinodeClient(apiKey) helper, err := patch.NewHelper(params.LinodeMachine, params.Client) if err != nil { diff --git a/cmd/main.go b/cmd/main.go index d588c73c5..a5357b8e1 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -97,7 +97,7 @@ func main() { // after the manager stops then its usage might be unsafe. // LeaderElectionReleaseOnCancel: true, }) - if err != nil { + if mgr == nil || err != nil { setupLog.Error(err, "unable to start manager") os.Exit(1) } diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 7842971fd..49e976688 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -142,6 +143,10 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques if err != nil { log.Info("Failed to fetch cluster by label", "error", err.Error()) + return ctrl.Result{}, client.IgnoreNotFound(err) + } else if cluster == nil { + log.Info("Failed to find cluster by label") + return ctrl.Result{}, client.IgnoreNotFound(err) } @@ -157,20 +162,6 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, client.IgnoreNotFound(err) } - clusterScope, err := scope.NewClusterScope( - r.LinodeApiKey, - scope.ClusterScopeParams{ - Client: r.Client, - Cluster: cluster, - LinodeCluster: linodeCluster, - }, - ) - if err != nil { - log.Info("Failed to create cluster scope", "error", err.Error()) - - return ctrl.Result{}, fmt.Errorf("failed to create cluster scope: %w", err) - } - machineScope, err := scope.NewMachineScope( r.LinodeApiKey, scope.MachineScopeParams{ @@ -187,15 +178,16 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, fmt.Errorf("failed to create machine scope: %w", err) } - return r.reconcile(ctx, machineScope, clusterScope, log) + return r.reconcile(ctx, machineScope, log) } func (r *LinodeMachineReconciler) reconcile( ctx context.Context, machineScope *scope.MachineScope, - clusterScope *scope.ClusterScope, logger logr.Logger, ) (res ctrl.Result, err error) { + res = ctrl.Result{} + machineScope.LinodeMachine.Status.Ready = false machineScope.LinodeMachine.Status.FailureReason = nil machineScope.LinodeMachine.Status.FailureMessage = util.Pointer("") @@ -211,9 +203,7 @@ func (r *LinodeMachineReconciler) reconcile( r.Recorder.Event(machineScope.LinodeMachine, corev1.EventTypeWarning, string(failureReason), err.Error()) } - if patchErr := machineScope.PatchHelper.Patch(ctx, machineScope.LinodeMachine); patchErr != nil && - client.IgnoreNotFound(patchErr) != nil { - + if patchErr := machineScope.PatchHelper.Patch(ctx, machineScope.LinodeMachine); patchErr != nil && client.IgnoreNotFound(patchErr) != nil { logger.Error(patchErr, "failed to patch LinodeMachine") err = errors.Join(err, patchErr) @@ -224,28 +214,7 @@ func (r *LinodeMachineReconciler) reconcile( if !machineScope.LinodeMachine.ObjectMeta.DeletionTimestamp.IsZero() { failureReason = cerrs.DeleteMachineError - logger.Info("deleting machine") - - if machineScope.LinodeMachine.Spec.InstanceID != nil { - if err = machineScope.LinodeClient.DeleteInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { - logger.Info("Failed to delete Linode machine instance", "error", err.Error()) - - // Not found is not an error - if apiErr, ok := err.(*linodego.Error); ok && apiErr.Code != http.StatusNotFound { - return - } - - err = nil - } - } else { - logger.Info("Machine ID is missing, nothing to do") - } - - conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "instance deleted") - - machineScope.LinodeMachine.Spec.ProviderID = nil - machineScope.LinodeMachine.Spec.InstanceID = nil - controllerutil.RemoveFinalizer(machineScope.LinodeMachine, infrav1.GroupVersion.String()) + err = r.reconcileDelete(ctx, logger, machineScope) return } @@ -266,40 +235,7 @@ func (r *LinodeMachineReconciler) reconcile( logger = logger.WithValues("ID", *machineScope.LinodeMachine.Spec.InstanceID) - if linodeInstance, err = machineScope.LinodeClient.GetInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { - logger.Info("Failed to get Linode machine instance", "error", err.Error()) - - // Not found is not an error - if apiErr, ok := err.(*linodego.Error); ok && apiErr.Code == http.StatusNotFound { - conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, string(failureReason), clusterv1.ConditionSeverityWarning, "instance not found") - - err = nil - } - - return - } - - if _, ok := requeueInstanceStatuses[linodeInstance.Status]; ok { - if linodeInstance.Updated.Add(reconciler.DefaultMachineControllerWaitForRunningTimeout).After(time.Now()) { - logger.Info("Instance has one operaton running, re-queuing reconciliation", "status", linodeInstance.Status) - - res = ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay} - } else { - logger.Info("Instance has one operaton long running, skipping reconciliation", "status", linodeInstance.Status) - } - - return - } else if _, ok := skippedInstanceStatuses[linodeInstance.Status]; ok || linodeInstance.Status != linodego.InstanceRunning { - logger.Info("Instance has incompatible status, skipping reconciliation", "status", linodeInstance.Status) - - conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, string(linodeInstance.Status), clusterv1.ConditionSeverityInfo, "incompatible status") - - return - } - - conditions.MarkTrue(machineScope.LinodeMachine, clusterv1.ReadyCondition) - - r.Recorder.Event(machineScope.LinodeMachine, corev1.EventTypeNormal, string(clusterv1.ReadyCondition), "instance is running") + res, linodeInstance, err = r.reconcileUpdate(ctx, logger, machineScope) return } @@ -307,6 +243,12 @@ func (r *LinodeMachineReconciler) reconcile( // Create failureReason = cerrs.CreateMachineError + linodeInstance, err = r.reconcileCreate(ctx, machineScope, logger) + + return +} + +func (*LinodeMachineReconciler) reconcileCreate(ctx context.Context, machineScope *scope.MachineScope, logger logr.Logger) (linodeInstance *linodego.Instance, err error) { tags := []string{string(machineScope.LinodeCluster.UID), string(machineScope.LinodeMachine.UID)} filter := map[string]string{ "tags": strings.Join(tags, ","), @@ -343,13 +285,16 @@ func (r *LinodeMachineReconciler) reconcile( logger.Info("Failed to create Linode machine instance", "error", err.Error()) // Already exists is not an error - if apiErr, ok := err.(*linodego.Error); ok && apiErr.Code != http.StatusFound { + apiErr := linodego.Error{} + if errors.As(err, &apiErr) && apiErr.Code != http.StatusFound { return } err = nil - logger.Info("Linode instance already exists", "existing", linodeInstance.ID) + if linodeInstance != nil { + logger.Info("Linode instance already exists", "existing", linodeInstance.ID) + } } default: logger.Error(errors.New("multiple instances found"), "Panic! Multiple instances found. This might be a concurrency issue in the controller!!!", "filters", string(rawFilter)) @@ -357,7 +302,13 @@ func (r *LinodeMachineReconciler) reconcile( return } - logger = logger.WithValues("ID", linodeInstance.ID) + if linodeInstance == nil { + err = errors.New("missing instance") + + logger.Error(err, "Panic! Failed to create isntance") + + return + } machineScope.LinodeMachine.Status.Ready = true machineScope.LinodeMachine.Spec.InstanceID = &linodeInstance.ID @@ -374,6 +325,82 @@ func (r *LinodeMachineReconciler) reconcile( return } +func (r *LinodeMachineReconciler) reconcileUpdate(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (res reconcile.Result, linodeInstance *linodego.Instance, err error) { + if machineScope.LinodeMachine.Spec.InstanceID == nil { + err = errors.New("missing instance ID") + + return + } + + res = ctrl.Result{} + + if linodeInstance, err = machineScope.LinodeClient.GetInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { + logger.Info("Failed to get Linode machine instance", "error", err.Error()) + + // Not found is not an error + apiErr := linodego.Error{} + if errors.As(err, &apiErr) && apiErr.Code == http.StatusNotFound { + conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, string("missing"), clusterv1.ConditionSeverityWarning, "instance not found") + + err = nil + } + + return + } + + if _, ok := requeueInstanceStatuses[linodeInstance.Status]; ok { + if linodeInstance.Updated.Add(reconciler.DefaultMachineControllerWaitForRunningTimeout).After(time.Now()) { + logger.Info("Instance has one operaton running, re-queuing reconciliation", "status", linodeInstance.Status) + + res = ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay} + } else { + logger.Info("Instance has one operaton long running, skipping reconciliation", "status", linodeInstance.Status) + } + + return + } else if _, ok := skippedInstanceStatuses[linodeInstance.Status]; ok || linodeInstance.Status != linodego.InstanceRunning { + logger.Info("Instance has incompatible status, skipping reconciliation", "status", linodeInstance.Status) + + conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, string(linodeInstance.Status), clusterv1.ConditionSeverityInfo, "incompatible status") + + return + } + + conditions.MarkTrue(machineScope.LinodeMachine, clusterv1.ReadyCondition) + + r.Recorder.Event(machineScope.LinodeMachine, corev1.EventTypeNormal, string(clusterv1.ReadyCondition), "instance is running") + + return +} + +func (*LinodeMachineReconciler) reconcileDelete(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (err error) { + logger.Info("deleting machine") + + if machineScope.LinodeMachine.Spec.InstanceID != nil { + if err = machineScope.LinodeClient.DeleteInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { + logger.Info("Failed to delete Linode machine instance", "error", err.Error()) + + // Not found is not an error + apiErr := linodego.Error{} + if errors.As(err, &apiErr) && apiErr.Code != http.StatusNotFound { + return + } + + err = nil + } + } else { + logger.Info("Machine ID is missing, nothing to do") + } + + conditions.MarkFalse(machineScope.LinodeMachine, clusterv1.ReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "instance deleted") + + machineScope.LinodeMachine.Spec.ProviderID = nil + machineScope.LinodeMachine.Spec.InstanceID = nil + controllerutil.RemoveFinalizer(machineScope.LinodeMachine, infrav1.GroupVersion.String()) + + return +} + // SetupWithManager sets up the controller with the Manager. func (r *LinodeMachineReconciler) SetupWithManager(mgr ctrl.Manager) error { controller, err := ctrl.NewControllerManagedBy(mgr). diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go index f9d6696d1..dcbc46f00 100644 --- a/controller/linodemachine_controller_helpers.go +++ b/controller/linodemachine_controller_helpers.go @@ -37,7 +37,7 @@ func (r *LinodeMachineReconciler) linodeClusterToLinodeMachines(logger logr.Logg logger = logger.WithName("LinodeMachineReconciler").WithName("linodeClusterToLinodeMachines") return func(ctx context.Context, o client.Object) []ctrl.Request { - ctx, cancel := context.WithTimeout(context.Background(), reconciler.DefaultMappingTimeout) + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultMappingTimeout) defer cancel() linodeCluster, ok := o.(*infrav1.LinodeCluster) @@ -65,7 +65,7 @@ func (r *LinodeMachineReconciler) linodeClusterToLinodeMachines(logger logr.Logg return nil } - request, err := r.requestsForCluster(cluster.Namespace, cluster.Name) + request, err := r.requestsForCluster(ctx, cluster.Namespace, cluster.Name) if err != nil { logger.Info("Failed to create request for cluster", "error", err.Error()) @@ -80,6 +80,9 @@ func (r *LinodeMachineReconciler) requeueLinodeMachinesForUnpausedCluster(logger logger = logger.WithName("LinodeMachineReconciler").WithName("requeueLinodeMachinesForUnpausedCluster") return func(ctx context.Context, o client.Object) []ctrl.Request { + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultMappingTimeout) + defer cancel() + cluster, ok := o.(*clusterv1.Cluster) if !ok { logger.Info("Failed to cast object to Cluster") @@ -93,7 +96,7 @@ func (r *LinodeMachineReconciler) requeueLinodeMachinesForUnpausedCluster(logger return nil } - request, err := r.requestsForCluster(cluster.Namespace, cluster.Name) + request, err := r.requestsForCluster(ctx, cluster.Namespace, cluster.Name) if err != nil { logger.Info("Failed to create request for cluster", "error", err.Error()) @@ -104,10 +107,7 @@ func (r *LinodeMachineReconciler) requeueLinodeMachinesForUnpausedCluster(logger } } -func (r *LinodeMachineReconciler) requestsForCluster(namespace, name string) ([]ctrl.Request, error) { - ctx, cancel := context.WithTimeout(context.Background(), r.ReconcileTimeout) - defer cancel() - +func (r *LinodeMachineReconciler) requestsForCluster(ctx context.Context, namespace, name string) ([]ctrl.Request, error) { labels := map[string]string{clusterv1.ClusterNameLabel: name} machineList := clusterv1.MachineList{} diff --git a/controller/linodemachine_controller_test.go b/controller/linodemachine_controller_test.go index 045d017a7..61e1f907c 100644 --- a/controller/linodemachine_controller_test.go +++ b/controller/linodemachine_controller_test.go @@ -8,6 +8,7 @@ import ( infrav1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" "github.com/linode/linodego" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) { @@ -56,12 +57,12 @@ func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) { var buf bytes.Buffer enc := gob.NewEncoder(&buf) err := enc.Encode(createConfig) - assert.NoError(t, err, "Failed to encode InstanceCreateOptions") + require.NoError(t, err, "Failed to encode InstanceCreateOptions") var actualMachineSpec infrav1.LinodeMachineSpec dec := gob.NewDecoder(&buf) err = dec.Decode(&actualMachineSpec) - assert.NoError(t, err, "Failed to decode LinodeMachineSpec") + require.NoError(t, err, "Failed to decode LinodeMachineSpec") assert.Equal(t, machineSpec, actualMachineSpec) } diff --git a/controller/suite_test.go b/controller/suite_test.go index 002dfc319..f199e4f4a 100644 --- a/controller/suite_test.go +++ b/controller/suite_test.go @@ -44,6 +44,8 @@ var k8sClient client.Client var testEnv *envtest.Environment func TestControllers(t *testing.T) { + t.Parallel() + RegisterFailHandler(Fail) RunSpecs(t, "Controller Suite")