From c68d7e2fb88f4f238f3d36f098dc856eca504bdd Mon Sep 17 00:00:00 2001 From: bartoszmajsak Date: Fri, 28 Jul 2023 13:32:09 +0200 Subject: [PATCH] feat(linter): introduces linter to check for problematic code This way we don not have to spend time on it during code reviews and can focus on features instead. golangci-lint is set of linters which is highly customizable. This PR enables it both as a make target and GitHub action. I fixed all the problems so we can start fresh. --- .github/workflows/lint.yml | 36 ++++ .golangci.yml | 112 ++++++++++++ Makefile | 32 ++-- controllers/controller_suite_test.go | 19 +- controllers/converter.go | 13 +- controllers/enable_authorino.go | 58 +++--- controllers/enable_mesh.go | 51 +++--- controllers/mesh_env_config.go | 5 +- controllers/namespace_updater.go | 33 ++-- controllers/project_mesh_controller.go | 27 +-- controllers/project_mesh_controller_test.go | 185 +++++++------------- main.go | 24 +-- version/version.go | 1 + 13 files changed, 365 insertions(+), 231 deletions(-) create mode 100644 .github/workflows/lint.yml create mode 100644 .golangci.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..8c141d9 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,36 @@ +name: linters +on: + push: + branches: + - master + pull_request: +jobs: + go-lint: + name: golangci-lint + runs-on: ubuntu-20.04 + env: + REPOSITORY: ${{ github.repository }} + steps: + - name: Set up Go env + uses: actions/setup-go@v4 + with: + go-version: 1.19 + - uses: actions/checkout@v3 + with: + fetch-depth: 1 + path: go/src/github.com/${{ env.REPOSITORY }} + - name: Set $GOPATH + run: | + echo "GOPATH=${{ github.workspace }}/go" >> $GITHUB_ENV + echo "${{ github.workspace }}/go/bin" >> $GITHUB_PATH + shell: bash + - name: Prepare codebase for linter (generates deps, stubs) + run: | + cd go/src/github.com/${{ env.REPOSITORY }} + make deps generate + shell: bash + - name: golangci-lint + uses: golangci/golangci-lint-action@v3 + with: + version: v1.53 + working-directory: go/src/github.com/${{ env.REPOSITORY }} diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..a46776f --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,112 @@ +linters-settings: + govet: + check-shadowing: true + golint: + min-confidence: 0 + gocyclo: + min-complexity: 16 + cyclop: + max-complexity: 16 + dupl: + threshold: 128 + funlen: + lines: 128 + statements: 64 + goconst: + min-len: 4 + min-occurrences: 3 + depguard: + list-type: blacklist + packages: + - github.com/sirupsen/logrus + - sigs.k8s.io/controller-runtime/pkg/log + - sigs.k8s.io/controller-runtime/pkg/log/zap + - sigs.k8s.io/controller-runtime/pkg/runtime/log + misspell: + locale: US + ignore-words: + - istio + - k8s + lll: + line-length: 180 + goimports: + local-prefixes: github.com/maistra/odh-project-controller + gocritic: + enabled-tags: + - performance + - style + - experimental + disabled-checks: + - wrapperFunc + - commentFormatting # https://github.com/go-critic/go-critic/issues/755 + - hugeParam # seems to be premature optimization based on https://github.com/Maistra/istio-workspace/pull/378#discussion_r392208906 + nestif: + min-complexity: 8 + unused: + check-exported: true + gocognit: + min-complexity: 20 + wrapcheck: + ignoreSigs: + - .Errorf( + - errors.New( + - errors.Unwrap( + - .Wrap( + - .Wrapf( + - .WithMessage( + - errors.WrapIfWithDetails + - errors.WithDetails( + - errors.WrapWithDetails( + - errors.WrapIf( + - errors.NewWithDetails( + +linters: + enable-all: true + disable: + - depguard + - exhaustruct + - exhaustivestruct + - forbidigo + - gofmt # We use goimports and when using them both leads to contradicting errors + - gofumpt + - gomnd + - paralleltest + - prealloc + +run: + deadline: 10m + +output: + format: colored-line-number + print-issued-lines: true + print-linter-name: true + +issues: + max-issues-per-linter: 0 + max-same-issues: 0 + exclude-rules: + - path: test/ + linters: + - goconst + - gocyclo + - golint + - errcheck + - dupl + - gosec + - revive + - stylecheck + - wrapcheck + # Exclude particular linters for tests files. + - path: _test\.go + linters: + - gochecknoglobals + - gocyclo + - errcheck + - dupl + - gosec + - revive + - wrapcheck + - path: _suite_test\.go + linters: + - revive + - unused diff --git a/Makefile b/Makefile index 81ed107..d4b480f 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,7 @@ PACKAGE_NAME:=github.com/opendatahub-io/$(PROJECT_NAME) SHELL = /usr/bin/env bash -o pipefail .PHONY: all -all: tools test build +all: tools lint test build ##@ General .PHONY: help @@ -26,16 +26,19 @@ generate: tools ## Generates required resources for the controller to work prope SRC_DIRS:=./controllers ./test SRCS:=$(shell find ${SRC_DIRS} -name "*.go") -.PHONY: fmt -fmt: $(SRCS) ## Formats the code. - $(LOCALBIN)/goimports -l -w -e $(SRC_DIRS) -.PHONY: vet -vet: ## Run go vet against code. - go vet ./... +.PHONY: format +format: $(SRCS) ## Removes unneeded imports and formats source code + $(call header,"Formatting code") + $(LOCALBIN)/goimports -l -w -e $(SRC_DIRS) $(TEST_DIRS) + +.PHONY: lint +lint: tools ## Concurrently runs a whole bunch of static analysis tools + $(call header,"Running a whole bunch of static analysis tools") + $(LOCALBIN)/golangci-lint run --fix --sort-results .PHONY: test -test: generate fmt vet +test: generate test: test-unit+kube-envtest ## Run all tests. You can also select a category by running e.g. make test-unit or make test-kube-envtest ENVTEST_K8S_VERSION = 1.26 # refers to the version of kubebuilder assets to be downloaded by envtest binary. @@ -73,14 +76,14 @@ deps: go mod download && go mod tidy .PHONY: build -build: deps generate fmt vet go-build ## Build manager binary. +build: deps format generate go-build ## Build manager binary. .PHONY: go-build go-build: ${GOBUILD} go build -ldflags "${LDFLAGS}" -o bin/manager main.go .PHONY: run -run: generate fmt vet ## Run a controller from your host. +run: format generate ## Run a controller from your host. go run ./main.go ##@ Container images @@ -122,7 +125,7 @@ $(shell mkdir -p $(LOCALBIN)) tools: deps tools: $(LOCALBIN)/controller-gen $(LOCALBIN)/kustomize ## Installs required tools in local ./bin folder tools: $(LOCALBIN)/setup-envtest $(LOCALBIN)/ginkgo -tools: $(LOCALBIN)/goimports +tools: $(LOCALBIN)/goimports $(LOCALBIN)/golangci-lint KUSTOMIZE_VERSION ?= v5.0.1 $(LOCALBIN)/kustomize: @@ -145,5 +148,10 @@ $(LOCALBIN)/ginkgo: GOBIN=$(LOCALBIN) go install -mod=readonly github.com/onsi/ginkgo/v2/ginkgo $(LOCALBIN)/goimports: - $(call header,"Installing goimports") + $(call header,"Installing $(notdir $@)") GOBIN=$(LOCALBIN) go install -mod=readonly golang.org/x/tools/cmd/goimports + +LINT_VERSION=v1.53.3 +$(LOCALBIN)/golangci-lint: + $(call header,"Installing $(notdir $@)") + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(LOCALBIN) $(LINT_VERSION) \ No newline at end of file diff --git a/controllers/controller_suite_test.go b/controllers/controller_suite_test.go index 57b507d..c8c5682 100644 --- a/controllers/controller_suite_test.go +++ b/controllers/controller_suite_test.go @@ -6,19 +6,16 @@ import ( "testing" "time" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" "github.com/opendatahub-io/odh-project-controller/controllers" "github.com/opendatahub-io/odh-project-controller/test/labels" + "go.uber.org/zap/zapcore" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" maistramanifests "maistra.io/api/manifests" - - "go.uber.org/zap/zapcore" ctrl "sigs.k8s.io/controller-runtime" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -39,7 +36,7 @@ func TestController(t *testing.T) { RunSpecs(t, "Controller & Webhook Suite") } -var _ = BeforeSuite(func() { +var _ = SynchronizedBeforeSuite(func() { if !Label(labels.EvnTest).MatchesLabelFilter(GinkgoLabelFilter()) { return } @@ -91,21 +88,23 @@ var _ = BeforeSuite(func() { defer GinkgoRecover() Expect(mgr.Start(ctx)).To(Succeed(), "Failed to start manager") }() -}) +}, func() {}) -var _ = AfterSuite(func() { +var _ = SynchronizedAfterSuite(func() { if !Label(labels.EvnTest).MatchesLabelFilter(GinkgoLabelFilter()) { return } By("Tearing down the test environment") cancel() Expect(envTest.Stop()).To(Succeed()) -}) +}, func() {}) func loadCRDs() []*v1.CustomResourceDefinition { smmYaml, err := maistramanifests.ReadManifest("maistra.io_servicemeshmembers.yaml") Expect(err).NotTo(HaveOccurred()) + crd := &v1.CustomResourceDefinition{} + err = controllers.ConvertToStructuredResource(smmYaml, crd) Expect(err).NotTo(HaveOccurred()) diff --git a/controllers/converter.go b/controllers/converter.go index d106597..2d1c91d 100644 --- a/controllers/converter.go +++ b/controllers/converter.go @@ -4,21 +4,24 @@ import ( "bytes" "github.com/manifestival/manifestival" + "github.com/pkg/errors" "k8s.io/client-go/kubernetes/scheme" ) func ConvertToStructuredResource(yamlContent []byte, out interface{}, opts ...manifestival.Option) error { reader := bytes.NewReader(yamlContent) - m, err := manifestival.ManifestFrom(manifestival.Reader(reader), opts...) + + manifest, err := manifestival.ManifestFrom(manifestival.Reader(reader), opts...) if err != nil { - return err + return errors.Wrap(err, "failed reading manifest") } s := scheme.Scheme RegisterSchemes(s) - err = s.Convert(&m.Resources()[0], out, nil) - if err != nil { - return err + + if err := s.Convert(&manifest.Resources()[0], out, nil); err != nil { + return errors.Wrap(err, "failed converting manifest") } + return nil } diff --git a/controllers/enable_authorino.go b/controllers/enable_authorino.go index cb40504..21f108f 100644 --- a/controllers/enable_authorino.go +++ b/controllers/enable_authorino.go @@ -2,68 +2,77 @@ package controllers import ( "context" - _ "embed" + _ "embed" // needed for go:embed directive "reflect" "regexp" "strings" authorino "github.com/kuadrant/authorino/api/v1beta1" + "github.com/pkg/errors" v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" ) -type reconcileFunc func(ctx context.Context, ns *v1.Namespace) error +type reconcileFunc func(ctx context.Context, namespace *v1.Namespace) error -func (r *OpenshiftServiceMeshReconciler) reconcileAuthConfig(ctx context.Context, ns *v1.Namespace) error { - log := r.Log.WithValues("feature", "authorino", "namespace", ns.Name) +func (r *OpenshiftServiceMeshReconciler) reconcileAuthConfig(ctx context.Context, namespace *v1.Namespace) error { + log := r.Log.WithValues("feature", "authorino", "namespace", namespace.Name) - desiredAuthConfig, err := r.createAuthConfig(ns, ns.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost]) + desiredAuthConfig, err := r.createAuthConfig(namespace, namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost]) if err != nil { log.Error(err, "Failed creating AuthConfig object") + return err } - foundAuthConfig := &authorino.AuthConfig{} + justCreated := false + foundAuthConfig := &authorino.AuthConfig{} err = r.Get(ctx, types.NamespacedName{ Name: desiredAuthConfig.Name, - Namespace: ns.Name, + Namespace: namespace.Name, }, foundAuthConfig) if err != nil { if apierrs.IsNotFound(err) { log.Info("Creating Authorino AuthConfig") - // Create the AuthConfig in the Openshift cluster + err = r.Create(ctx, desiredAuthConfig) if err != nil && !apierrs.IsAlreadyExists(err) { log.Error(err, "Unable to create Authorino AuthConfig") - return err + + return errors.Wrap(err, "unable to create AuthConfig") } + justCreated = true } else { log.Error(err, "Unable to fetch the AuthConfig") - return err + + return errors.Wrap(err, "unable to fetch AuthConfig") } } // Reconcile the Authorino AuthConfig if it has been manually modified if !justCreated && !CompareAuthConfigs(*desiredAuthConfig, *foundAuthConfig) { log.Info("Reconciling Authorino AuthConfig") - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { if err := r.Get(ctx, types.NamespacedName{ Name: desiredAuthConfig.Name, - Namespace: ns.Name, + Namespace: namespace.Name, }, foundAuthConfig); err != nil { - return err + return errors.Wrapf(err, "failed getting AuthConfig %s in namespace %s", desiredAuthConfig.Name, namespace.Name) } + foundAuthConfig.Spec = desiredAuthConfig.Spec foundAuthConfig.ObjectMeta.Labels = desiredAuthConfig.ObjectMeta.Labels - return r.Update(ctx, foundAuthConfig) - }) - if err != nil { + + return errors.Wrap(r.Update(ctx, foundAuthConfig), "failed updating AuthConfig") + }); err != nil { log.Error(err, "Unable to reconcile the Authorino AuthConfig") - return err + + return errors.Wrap(err, "unable to reconcile the Authorino AuthConfig") } } @@ -73,7 +82,7 @@ func (r *OpenshiftServiceMeshReconciler) reconcileAuthConfig(ctx context.Context //go:embed template/authconfig.yaml var authConfigTemplate []byte -func (r *OpenshiftServiceMeshReconciler) createAuthConfig(ns *v1.Namespace, hosts ...string) (*authorino.AuthConfig, error) { +func (r *OpenshiftServiceMeshReconciler) createAuthConfig(namespace *v1.Namespace, hosts ...string) (*authorino.AuthConfig, error) { authHosts := make([]string, len(hosts)) for i := range hosts { authHosts[i] = ExtractHostName(hosts[i]) @@ -84,18 +93,20 @@ func (r *OpenshiftServiceMeshReconciler) createAuthConfig(ns *v1.Namespace, host return authConfig, err } - authConfig.SetName(ns.Name + "-protection") - authConfig.SetNamespace(ns.Name) + authConfig.SetName(namespace.Name + "-protection") + authConfig.SetNamespace(namespace.Name) + keyValue, err := getAuthorinoLabel() if err != nil { return nil, err } + authConfig.Labels[keyValue[0]] = keyValue[1] authConfig.Spec.Hosts = authHosts // Reflects oauth-proxy SAR settings - authConfig.Spec.Authorization[0].KubernetesAuthz.ResourceAttributes = &authorino.Authorization_KubernetesAuthz_ResourceAttributes{ - Namespace: authorino.StaticOrDynamicValue{Value: ns.Name}, + authConfig.Spec.Authorization[0].KubernetesAuthz.ResourceAttributes = &authorino.Authorization_KubernetesAuthz_ResourceAttributes{ //nolint:nosnakecase //reason external library + Namespace: authorino.StaticOrDynamicValue{Value: namespace.Name}, Group: authorino.StaticOrDynamicValue{Value: "kubeflow.org"}, Resource: authorino.StaticOrDynamicValue{Value: "notebooks"}, Verb: authorino.StaticOrDynamicValue{Value: "get"}, @@ -116,13 +127,16 @@ func CompareAuthConfigs(a1, a2 authorino.AuthConfig) bool { // If given string does not start with http(s) prefix it will be returned as is. func ExtractHostName(s string) string { r := regexp.MustCompile(`^(https?://)`) + withoutProtocol := r.ReplaceAllString(s, "") if s == withoutProtocol { return s } + index := strings.Index(withoutProtocol, "/") if index == -1 { return withoutProtocol } + return withoutProtocol[:index] } diff --git a/controllers/enable_mesh.go b/controllers/enable_mesh.go index cba86a2..3365faf 100644 --- a/controllers/enable_mesh.go +++ b/controllers/enable_mesh.go @@ -5,71 +5,79 @@ import ( "reflect" "strconv" - "k8s.io/apimachinery/pkg/runtime/schema" - routev1 "github.com/openshift/api/route/v1" - "k8s.io/apimachinery/pkg/labels" - "sigs.k8s.io/controller-runtime/pkg/client" - + "github.com/pkg/errors" v1 "k8s.io/api/core/v1" - apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" maistrav1 "maistra.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // Reconcile will manage the creation, update and deletion of the MeshMember for created the namespace. -func (r *OpenshiftServiceMeshReconciler) reconcileMeshMember(ctx context.Context, ns *v1.Namespace) error { - log := r.Log.WithValues("feature", "mesh", "namespace", ns.Name) +func (r *OpenshiftServiceMeshReconciler) reconcileMeshMember(ctx context.Context, namespace *v1.Namespace) error { + log := r.Log.WithValues("feature", "mesh", "namespace", namespace.Name) - desiredMeshMember := newServiceMeshMember(ns) + desiredMeshMember := newServiceMeshMember(namespace) foundMember := &maistrav1.ServiceMeshMember{} justCreated := false + err := r.Get(ctx, types.NamespacedName{ Name: desiredMeshMember.Name, - Namespace: ns.Name, + Namespace: namespace.Name, }, foundMember) if err != nil { if apierrs.IsNotFound(err) { log.Info("Adding namespace to the mesh") + err = r.Create(ctx, desiredMeshMember) if err != nil && !apierrs.IsAlreadyExists(err) { log.Error(err, "Unable to create ServiceMeshMember") - return err + + return errors.Wrap(err, "unable to create ServiceMeshMember") } + justCreated = true } else { log.Error(err, "Unable to fetch the ServiceMeshMember") - return err + + return errors.Wrap(err, "unable to fetch the ServiceMeshMember") } } // Reconcile the membership spec if it has been manually modified if !justCreated && !compareMeshMembers(*desiredMeshMember, *foundMember) { log.Info("Reconciling ServiceMeshMember") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { if err := r.Get(ctx, types.NamespacedName{ Name: desiredMeshMember.Name, - Namespace: ns.Namespace, + Namespace: namespace.Name, }, foundMember); err != nil { - return err + return errors.Wrapf(err, "failed getting ServieMeshMember %s in namespace %s", desiredMeshMember.Name, namespace.Name) } + foundMember.Spec = desiredMeshMember.Spec foundMember.ObjectMeta.Labels = desiredMeshMember.ObjectMeta.Labels - return r.Update(ctx, foundMember) + + return errors.Wrap(r.Update(ctx, foundMember), "failed updating ServiceMeshMember") }) + if err != nil { log.Error(err, "Unable to reconcile the ServiceMeshMember") - return err + + return errors.Wrap(err, "unable to reconcile the ServiceMeshMember") } } return nil } -func newServiceMeshMember(ns *v1.Namespace) *maistrav1.ServiceMeshMember { +func newServiceMeshMember(namespace *v1.Namespace) *maistrav1.ServiceMeshMember { controlPlaneName := getControlPlaneName() meshNamespace := getMeshNamespace() @@ -77,7 +85,7 @@ func newServiceMeshMember(ns *v1.Namespace) *maistrav1.ServiceMeshMember { TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ Name: "default", // The name MUST be default, per the maistra docs - Namespace: ns.Name, + Namespace: namespace.Name, }, Spec: maistrav1.ServiceMeshMemberSpec{ ControlPlaneRef: maistrav1.ServiceMeshControlPlaneRef{ @@ -97,6 +105,7 @@ func serviceMeshIsNotEnabled(meta metav1.ObjectMeta) bool { serviceMeshAnnotation := meta.Annotations[AnnotationServiceMesh] if serviceMeshAnnotation != "" { enabled, _ := strconv.ParseBool(serviceMeshAnnotation) + return !enabled } @@ -112,15 +121,17 @@ func (r *OpenshiftServiceMeshReconciler) findIstioIngress(ctx context.Context) ( Namespace: meshNamespace, }); err != nil { r.Log.Error(err, "Unable to find matching gateway") - return routev1.RouteList{}, err + + return routev1.RouteList{}, errors.Wrap(err, "unable to find matching gateway") } if len(routes.Items) == 0 { route := &routev1.Route{} + return routes, apierrs.NewNotFound(schema.GroupResource{ Group: route.GroupVersionKind().Group, Resource: route.ResourceVersion, - }, "No routes found") + }, "no-route-matching-label") } return routes, nil diff --git a/controllers/mesh_env_config.go b/controllers/mesh_env_config.go index db14718..50f3282 100644 --- a/controllers/mesh_env_config.go +++ b/controllers/mesh_env_config.go @@ -25,20 +25,23 @@ func getMeshNamespace() string { func getAuthorinoLabel() ([]string, error) { label := getEnvOr(AuthorinoLabelSelector, "authorino/topic=odh") keyValue := strings.Split(label, "=") + if len(keyValue) != 2 { return nil, errors.Errorf("Expected authorino label to be in key=value format, got [%s]", label) } + return keyValue, nil } func getAuthAudience() []string { aud := getEnvOr(AuthAudience, "https://kubernetes.default.svc") audiences := strings.Split(aud, ",") + for i := range audiences { audiences[i] = strings.TrimSpace(audiences[i]) } - return audiences + return audiences } func getEnvOr(key, defaultValue string) string { diff --git a/controllers/namespace_updater.go b/controllers/namespace_updater.go index 0e79007..81611a9 100644 --- a/controllers/namespace_updater.go +++ b/controllers/namespace_updater.go @@ -4,37 +4,35 @@ import ( "context" "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - + "github.com/pkg/errors" v1 "k8s.io/api/core/v1" - apierrs "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (r *OpenshiftServiceMeshReconciler) addGatewayAnnotations(ctx context.Context, ns *v1.Namespace) error { - if ns.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] != "" && - ns.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] != "" && - ns.ObjectMeta.Annotations[AnnotationPublicGatewayName] != "" { +func (r *OpenshiftServiceMeshReconciler) addGatewayAnnotations(ctx context.Context, namespace *v1.Namespace) error { + if namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] != "" && + namespace.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] != "" && + namespace.ObjectMeta.Annotations[AnnotationPublicGatewayName] != "" { // If annotation is present we have nothing to do return nil } routes, err := r.findIstioIngress(ctx) if err != nil { - if !apierrs.IsNotFound(err) { - return err - } - // TODO rethink if we shouldn't just fail here - r.Log.Info("Unable to find matching istio ingress gateway. Some things might not work.") - return nil + r.Log.Error(err, "Unable to find matching istio ingress gateway.") + + return err } - ns.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] = ExtractHostName(routes.Items[0].Spec.Host) - ns.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] = fmt.Sprintf("%s.%s.svc.cluster.local", routes.Items[0].Spec.To.Name, getMeshNamespace()) + namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] = ExtractHostName(routes.Items[0].Spec.Host) + namespace.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] = fmt.Sprintf("%s.%s.svc.cluster.local", routes.Items[0].Spec.To.Name, getMeshNamespace()) + gateway := extractGateway(routes.Items[0].ObjectMeta) if gateway != "" { - ns.ObjectMeta.Annotations[AnnotationPublicGatewayName] = gateway + namespace.ObjectMeta.Annotations[AnnotationPublicGatewayName] = gateway } - return r.Client.Update(ctx, ns) + + return errors.Wrap(r.Client.Update(ctx, namespace), "failed updating namespace with annotations") } func extractGateway(meta metav1.ObjectMeta) string { @@ -42,6 +40,7 @@ func extractGateway(meta metav1.ObjectMeta) string { if gwName == "" { return "" } + gateway := gwName gwNs := meta.Labels[LabelMaistraGatewayNamespace] diff --git a/controllers/project_mesh_controller.go b/controllers/project_mesh_controller.go index 2dbb1c4..7490c58 100644 --- a/controllers/project_mesh_controller.go +++ b/controllers/project_mesh_controller.go @@ -4,13 +4,13 @@ import ( "context" "regexp" - "github.com/kuadrant/authorino/api/v1beta1" - "github.com/go-logr/logr" + "github.com/kuadrant/authorino/api/v1beta1" + "github.com/pkg/errors" v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/errors" + k8serrs "k8s.io/apimachinery/pkg/util/errors" maistrav1 "maistra.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,30 +36,33 @@ func (r *OpenshiftServiceMeshReconciler) Reconcile(ctx context.Context, req ctrl reconcilers := []reconcileFunc{r.addGatewayAnnotations, r.reconcileMeshMember, r.reconcileAuthConfig} - ns := &v1.Namespace{} - err := r.Get(ctx, req.NamespacedName, ns) - if err != nil { + namespace := &v1.Namespace{} + if err := r.Get(ctx, req.NamespacedName, namespace); err != nil { if apierrs.IsNotFound(err) { log.Info("Stopping reconciliation") + return ctrl.Result{}, nil } - return ctrl.Result{}, err + + return ctrl.Result{}, errors.Wrap(err, "failed getting namespace") } - if IsReservedNamespace(ns.Name) || serviceMeshIsNotEnabled(ns.ObjectMeta) { + if IsReservedNamespace(namespace.Name) || serviceMeshIsNotEnabled(namespace.ObjectMeta) { log.Info("Skipped") + return ctrl.Result{}, nil } var errs []error for _, reconciler := range reconcilers { - errs = append(errs, reconciler(ctx, ns)) + errs = append(errs, reconciler(ctx, namespace)) } - return ctrl.Result{}, errors.NewAggregate(errs) + return ctrl.Result{}, k8serrs.NewAggregate(errs) } func (r *OpenshiftServiceMeshReconciler) SetupWithManager(mgr ctrl.Manager) error { + //nolint:wrapcheck //reason there is no point in wrapping it return ctrl.NewControllerManagedBy(mgr). For(&v1.Namespace{}). Owns(&maistrav1.ServiceMeshMember{}). @@ -69,6 +72,6 @@ func (r *OpenshiftServiceMeshReconciler) SetupWithManager(mgr ctrl.Manager) erro var reservedNamespaceRegex = regexp.MustCompile(`^(openshift|istio-system)$|^(kube|openshift)-.*$`) -func IsReservedNamespace(ns string) bool { - return reservedNamespaceRegex.MatchString(ns) +func IsReservedNamespace(namepace string) bool { + return reservedNamespaceRegex.MatchString(namepace) } diff --git a/controllers/project_mesh_controller_test.go b/controllers/project_mesh_controller_test.go index 35bbc42..4d6a5b2 100644 --- a/controllers/project_mesh_controller_test.go +++ b/controllers/project_mesh_controller_test.go @@ -5,18 +5,15 @@ import ( "os" "time" - "github.com/opendatahub-io/odh-project-controller/test" - - routev1 "github.com/openshift/api/route/v1" - - . "github.com/opendatahub-io/odh-project-controller/test/cluster" - "github.com/opendatahub-io/odh-project-controller/test/labels" - authorino "github.com/kuadrant/authorino/api/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/opendatahub-io/odh-project-controller/controllers" - v1 "k8s.io/api/core/v1" + "github.com/opendatahub-io/odh-project-controller/test" + . "github.com/opendatahub-io/odh-project-controller/test/cluster" + "github.com/opendatahub-io/odh-project-controller/test/labels" + openshiftv1 "github.com/openshift/api/route/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" maistrav1 "maistra.io/api/core/v1" @@ -31,24 +28,53 @@ const ( var _ = When("Namespace is created", Label(labels.EvnTest), func() { var ( - testNs *v1.Namespace + istioNs, + testNs *corev1.Namespace objectCleaner *Cleaner - routeWeight = int32(100) + route *openshiftv1.Route ) BeforeEach(func() { objectCleaner = CreateCleaner(cli, envTest.Config, timeout, interval) + istioNs = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-system", + }, + } + + weight := int32(100) + route = &openshiftv1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "odh-gateway", + Namespace: "istio-system", + Labels: map[string]string{ + "app": "odh-dashboard", + controllers.LabelMaistraGatewayName: "odh-gateway", + controllers.LabelMaistraGatewayNamespace: "opendatahub", + }, + }, + Spec: openshiftv1.RouteSpec{ + Host: "istio.io", + To: openshiftv1.RouteTargetReference{ + Name: "istio-ingressgateway", + Weight: &weight, + }, + }, + } + + Expect(cli.Create(context.Background(), istioNs)).To(Succeed()) + Expect(cli.Create(context.Background(), route)).To(Succeed()) }) AfterEach(func() { - objectCleaner.DeleteAll(testNs) + objectCleaner.DeleteAll(istioNs, route, testNs) }) Context("enabling service mesh", func() { It("should register it in the mesh if annotation is set to true", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "meshified-ns", Annotations: map[string]string{ @@ -78,7 +104,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should not register it in the mesh if annotation is absent", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "not-meshified-namespace", }, @@ -102,9 +128,9 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { }) }) - It("should create an SMM with default name and ns when no env vars", func() { + It("should create an SMM with default name and ns when no env vars defined", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "meshified-ns", Annotations: map[string]string{ @@ -134,9 +160,9 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { }) }) - It("should create an SMM with specified env name and ns when env vars", func() { + It("should create an SMM with specified name defined in env var", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "meshified-ns", Annotations: map[string]string{ @@ -144,9 +170,10 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { }, }, } + _ = os.Setenv(controllers.ControlPlaneEnv, "minimal") defer os.Unsetenv(controllers.ControlPlaneEnv) - _ = os.Setenv(controllers.MeshNamespaceEnv, "system-of-istio") + _ = os.Setenv(controllers.MeshNamespaceEnv, "istio-system") defer os.Unsetenv(controllers.MeshNamespaceEnv) // when @@ -166,7 +193,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { WithPolling(interval). Should(Succeed()) Expect(member.Spec.ControlPlaneRef.Name).To(Equal("minimal")) - Expect(member.Spec.ControlPlaneRef.Namespace).To(Equal("system-of-istio")) + Expect(member.Spec.ControlPlaneRef.Namespace).To(Equal("istio-system")) }) }) }) @@ -175,7 +202,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should configure authorization using defaults for ns belonging to the mesh", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "meshified-and-authorized-ns", Annotations: map[string]string{ @@ -184,34 +211,9 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { }, } - istioNs := &v1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "istio-system", - }, - } - - route := &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "odh-gateway", - Namespace: "istio-system", - Labels: map[string]string{ - "app": "odh-dashboard", - }, - }, - Spec: routev1.RouteSpec{ - Host: "istio.io", - To: routev1.RouteTargetReference{ - Name: "odh-gateway", - Weight: &routeWeight, - }, - }, - } - - defer objectCleaner.DeleteAll(route, istioNs) + defer objectCleaner.DeleteAll(testNs) // when - Expect(cli.Create(context.Background(), istioNs)).To(Succeed()) - Expect(cli.Create(context.Background(), route)).To(Succeed()) Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then @@ -233,14 +235,13 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Expect(actualAuthConfig.Labels).To(Equal(expectedAuthConfig.Labels)) Expect(actualAuthConfig.Name).To(Equal(testNs.GetName() + "-protection")) - // TODO should extend assertions to auth rules Expect(actualAuthConfig.Spec.Hosts).To(Equal(expectedAuthConfig.Spec.Hosts)) }) }) It("should configure authorization using env vars for ns belonging to the mesh", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "meshified-and-authorized-ns", Annotations: map[string]string{ @@ -249,30 +250,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { }, } - istioNs := &v1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "istio-system", - }, - } - - route := &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "odh-gateway", - Namespace: "istio-system", - Labels: map[string]string{ - "app": "odh-dashboard", - }, - }, - Spec: routev1.RouteSpec{ - Host: "istio.io", - To: routev1.RouteTargetReference{ - Name: "odh-gateway", - Weight: &routeWeight, - }, - }, - } - - defer objectCleaner.DeleteAll(route, istioNs) + defer objectCleaner.DeleteAll(testNs) // when _ = os.Setenv(controllers.AuthorinoLabelSelector, "app=rhods") @@ -280,8 +258,6 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { _ = os.Setenv(controllers.AuthAudience, "opendatahub.io,foo , bar") defer os.Unsetenv(controllers.AuthAudience) - Expect(cli.Create(context.Background(), istioNs)).To(Succeed()) - Expect(cli.Create(context.Background(), route)).To(Succeed()) Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then @@ -311,7 +287,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should not configure authorization rules if namespace is not part of the mesh", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "not-meshified-nor-authorized-namespace", }, @@ -338,48 +314,9 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Context("propagating service mesh gateway info", func() { - var ( - istioNs *v1.Namespace - route *routev1.Route - ) - - BeforeEach(func() { - istioNs = &v1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "istio-system", - }, - } - route = &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "odh-gateway", - Namespace: "istio-system", - Labels: map[string]string{ - "app": "odh-dashboard", - controllers.LabelMaistraGatewayName: "odh-gateway", - controllers.LabelMaistraGatewayNamespace: "opendatahub", - }, - }, - Spec: routev1.RouteSpec{ - Host: "istio.io", - To: routev1.RouteTargetReference{ - Name: "istio-ingressgateway", - Kind: "Service", - Weight: &routeWeight, - }, - }, - } - - Expect(cli.Create(context.Background(), istioNs)).To(Succeed()) - Expect(cli.Create(context.Background(), route)).To(Succeed()) - }) - - AfterEach(func() { - objectCleaner.DeleteAll(route, istioNs) - }) - It("should add just gateway name to the namespace if there is no gateway namespace defined", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "plain-meshified-ns", Annotations: map[string]string{ @@ -396,9 +333,10 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then - actualTestNs := &v1.Namespace{} + actualTestNs := &corev1.Namespace{} Eventually(func() string { _ = cli.Get(context.Background(), types.NamespacedName{Name: testNs.Name}, actualTestNs) + return actualTestNs.Annotations[controllers.AnnotationPublicGatewayName] }). WithTimeout(timeout). @@ -408,7 +346,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should add fully qualified gateway name to the namespace", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "plain-meshified-ns", Annotations: map[string]string{ @@ -421,9 +359,10 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then - actualTestNs := &v1.Namespace{} + actualTestNs := &corev1.Namespace{} Eventually(func() string { _ = cli.Get(context.Background(), types.NamespacedName{Name: testNs.Name}, actualTestNs) + return actualTestNs.Annotations[controllers.AnnotationPublicGatewayName] }). WithTimeout(timeout). @@ -433,7 +372,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should add external gateway host to the namespace", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "plain-meshified-ns", Annotations: map[string]string{ @@ -446,9 +385,10 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then - actualTestNs := &v1.Namespace{} + actualTestNs := &corev1.Namespace{} Eventually(func() string { _ = cli.Get(context.Background(), types.NamespacedName{Name: testNs.Name}, actualTestNs) + return actualTestNs.Annotations[controllers.AnnotationPublicGatewayExternalHost] }). WithTimeout(timeout). @@ -458,7 +398,7 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { It("should add internal gateway host to the namespace", func() { // given - testNs = &v1.Namespace{ + testNs = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "plain-meshified-ns", Annotations: map[string]string{ @@ -471,9 +411,10 @@ var _ = When("Namespace is created", Label(labels.EvnTest), func() { Expect(cli.Create(context.Background(), testNs)).To(Succeed()) // then - actualTestNs := &v1.Namespace{} + actualTestNs := &corev1.Namespace{} Eventually(func() string { _ = cli.Get(context.Background(), types.NamespacedName{Name: testNs.Name}, actualTestNs) + return actualTestNs.Annotations[controllers.AnnotationPublicGatewayInternalHost] }). WithTimeout(timeout). diff --git a/main.go b/main.go index 0464c47..b1e48f6 100644 --- a/main.go +++ b/main.go @@ -2,33 +2,34 @@ package main import ( "flag" + "os" + "github.com/opendatahub-io/odh-project-controller/controllers" "github.com/opendatahub-io/odh-project-controller/version" - "os" + "k8s.io/apimachinery/pkg/runtime" + // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" - - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" - //+kubebuilder:scaffold:imports ) +//nolint:gochecknoglobals //reason: used only here var ( - scheme = runtime.NewScheme() - setupLog = ctrl.Log.WithName("setup") + scheme = runtime.NewScheme() + setupLog = ctrl.Log.WithName("setup") + metricsAddr string + enableLeaderElection bool + probeAddr string ) -func init() { +func init() { //nolint:gochecknoinits //reason this way we ensure schemes are always registered before we start anything controllers.RegisterSchemes(scheme) } func main() { - var metricsAddr string - var enableLeaderElection bool - var probeAddr string flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") flag.BoolVar(&enableLeaderElection, "leader-elect", false, @@ -59,6 +60,7 @@ func main() { ctrlLog := ctrl.Log.WithName("controllers"). WithName("odh-project") ctrlLog.Info("creating controller instance", "version", version.Version, "commit", version.Commit, "build-time", version.BuildTime) + if err = (&controllers.OpenshiftServiceMeshReconciler{ Client: mgr.GetClient(), Log: ctrlLog, @@ -72,12 +74,14 @@ func main() { setupLog.Error(err, "unable to set up health check") os.Exit(1) } + if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { setupLog.Error(err, "unable to set up ready check") os.Exit(1) } setupLog.Info("Starting manager") + if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") os.Exit(1) diff --git a/version/version.go b/version/version.go index bd22253..6637c66 100644 --- a/version/version.go +++ b/version/version.go @@ -1,5 +1,6 @@ package version +//nolint:gochecknoglobals //reason these are used for binary version output only var ( // Version hold a semantic version of the running binary. Version = "v0.0.0"