From 3035f5eeeac4c6f4059172d03e1316c5798e2696 Mon Sep 17 00:00:00 2001 From: Matt Wise <768067+diranged@users.noreply.github.com> Date: Tue, 29 Nov 2022 09:37:31 -0800 Subject: [PATCH] fix: race condition causing .status.accessmessage to not be setup (#46) --- .github/actions/setup-go/action.yaml | 20 +--- .github/workflows/build.yaml | 7 +- .github/workflows/ci.yaml | 18 ++-- .github/workflows/main.yaml | 2 +- .github/workflows/publish.yaml | 77 +++++---------- .../{controller-release.yaml => release.yaml} | 5 +- .gitignore | 2 +- .goreleaser.yml | 96 +++++++++++++++++++ Custom.mk | 41 +++++--- DEVELOPMENT.md | 2 +- Dockerfile | 30 +----- api/v1alpha1/core_status.go | 4 +- controllers/base_controller.go | 53 ++++++++-- controllers/base_controller_test.go | 4 +- controllers/base_request_controller.go | 35 +++---- controllers/base_request_controller_test.go | 15 +-- controllers/builders/exec_access_builder.go | 13 ++- controllers/builders/interfaces.go | 2 +- controllers/builders/pod_access_builder.go | 30 +++--- controllers/const.go | 3 + controllers/exec_access_request_controller.go | 3 +- controllers/pod_access_request_controller.go | 9 +- .../pod_access_request_controller_test.go | 9 +- test/e2e/e2e_suite_test.go | 2 +- test/e2e/e2e_test.go | 16 ++++ test/utils/utils.go | 13 ++- 26 files changed, 299 insertions(+), 212 deletions(-) rename .github/workflows/{controller-release.yaml => release.yaml} (76%) create mode 100644 .goreleaser.yml diff --git a/.github/actions/setup-go/action.yaml b/.github/actions/setup-go/action.yaml index 34918195..018a6d08 100644 --- a/.github/actions/setup-go/action.yaml +++ b/.github/actions/setup-go/action.yaml @@ -7,23 +7,5 @@ runs: - name: Set up Go uses: actions/setup-go@v3 with: + cache: true go-version-file: "go.mod" - - - name: Find the Go Cache - id: go - shell: bash - run: | - echo "build-cache=$(go env GOCACHE)" >> $GITHUB_OUTPUT - echo "mod-cache=$(go env GOMODCACHE)" >> $GITHUB_OUTPUT - - - name: Cache the Go Build Cache - uses: actions/cache@v3 - with: - path: ${{ steps.go.outputs.build-cache }} - key: ${{ runner.os }}-build-${{ hashFiles('**/go.sum') }} - - - name: Cache Go Dependencies - uses: actions/cache@v3 - with: - path: ${{ steps.go.outputs.mod-cache }} - key: ${{ runner.os }}-mod-${{ hashFiles('**/go.sum') }} diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 71e249b3..dfb7aac6 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -15,8 +15,5 @@ jobs: - name: Set up Go uses: ./.github/actions/setup-go - - name: Build Binaries - run: make build cli - - - name: Build Docker Image - run: make docker-build + - name: Build Artifacts + run: make build diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 3e9ddff7..9c3876e1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -31,7 +31,6 @@ jobs: if: needs.detect-noop.outputs.noop != 'true' uses: ./.github/workflows/docgen.yaml - lint: needs: detect-noop if: needs.detect-noop.outputs.noop != 'true' @@ -43,16 +42,13 @@ jobs: uses: ./.github/workflows/test.yaml test-e2e: - needs: detect-noop + needs: build if: needs.detect-noop.outputs.noop != 'true' uses: ./.github/workflows/test-e2e.yaml - # Disabled: The cross-platform builds take 10+m... so limiting these builds - # to just `main` for now. - # - # publish-dry: - # needs: detect-noop - # if: needs.detect-noop.outputs.noop != 'true' - # uses: ./.github/workflows/publish.yaml - # with: - # push: false + publish-dry: + needs: build + if: needs.detect-noop.outputs.noop != 'true' + uses: ./.github/workflows/publish.yaml + with: + publish: false diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 0693709d..73b340ba 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -27,4 +27,4 @@ jobs: uses: ./.github/workflows/publish.yaml needs: [test-e2e, unit-tests, lint, build] with: - push: true + publish: true diff --git a/.github/workflows/publish.yaml b/.github/workflows/publish.yaml index f084a70c..a2e61af3 100644 --- a/.github/workflows/publish.yaml +++ b/.github/workflows/publish.yaml @@ -3,10 +3,7 @@ name: Reusable Workflow on: workflow_call: inputs: - push: - type: boolean - default: false - release: + publish: type: boolean default: false @@ -21,26 +18,19 @@ jobs: - name: Checkout code uses: actions/checkout@v3 with: + fetch-depth: 0 persist-credentials: false - name: Set up Go uses: ./.github/actions/setup-go - - name: Build Binaries - run: make build cli - - name: Setup QEMU uses: docker/setup-qemu-action@v2 with: platforms: all - - name: Cache Docker layers - uses: actions/cache@v3 - with: - path: /tmp/.buildx-cache - key: ${{ runner.os }}-buildx-${{ github.sha }} - restore-keys: | - ${{ runner.os }}-buildx- + - name: Build Artifacts + run: make build - name: Log in to the Container registry uses: docker/login-action@v2.1.0 @@ -49,50 +39,27 @@ jobs: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} - - name: Extract metadata (tags, labels) for Docker - id: meta - uses: docker/metadata-action@v4.1.1 - with: - images: | - ${{ env.IMAGE_REGISTRY }}/${{ env.IMAGE_NAME }} - tags: | - # set latest tag for default branch - type=raw,value=latest,enable={{is_default_branch}} - type=semver,pattern={{version}} - type=semver,pattern={{major}}.{{version}} - type=semver,pattern={{major}} - type=sha - type=ref,event=pr - type=ref,event=branch - - name: Setup Docker Buildx uses: docker/setup-buildx-action@v2 - - name: Build and push Docker image - uses: docker/build-push-action@v3.2.0 + - if: ${{ ! inputs.publish }} + uses: goreleaser/goreleaser-action@v2 with: - context: . - push: ${{ inputs.push }} - platforms: linux/amd64,linux/arm64 - tags: ${{ steps.meta.outputs.tags }} - labels: ${{ steps.meta.outputs.labels }} - cache-from: type=local,src=/tmp/.buildx-cache - cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max - - # Temp fix - # https://github.com/docker/build-push-action/issues/252 - # https://github.com/moby/buildkit/issues/1896 - - name: Move cache - run: | - rm -rf /tmp/.buildx-cache - mv /tmp/.buildx-cache-new /tmp/.buildx-cache + distribution: goreleaser + version: latest + args: release --snapshot --rm-dist + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Just to make the .goreleaser.yml pass when not using `make ..` targets. + IMG: img:local - - name: Upload binaries to release - if: ${{ inputs.release }} - uses: svenstaro/upload-release-action@v2 + - if: ${{ inputs.publish }} + uses: goreleaser/goreleaser-action@v2 with: - repo_token: ${{ secrets.GITHUB_TOKEN }} - file_glob: true - file: outputs/ozctl-* - tag: ${{ github.ref }} - overwrite: true + distribution: goreleaser + version: latest + args: release --rm-dist + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Just to make the .goreleaser.yml pass when not using `make ..` targets. + IMG: img:local diff --git a/.github/workflows/controller-release.yaml b/.github/workflows/release.yaml similarity index 76% rename from .github/workflows/controller-release.yaml rename to .github/workflows/release.yaml index c31394c1..af82f1b1 100644 --- a/.github/workflows/controller-release.yaml +++ b/.github/workflows/release.yaml @@ -1,4 +1,4 @@ -name: Controller Release +name: Release on: push: @@ -11,5 +11,4 @@ jobs: publish: uses: ./.github/workflows/publish.yaml with: - push: true - release: true + publish: true diff --git a/.gitignore b/.gitignore index 6a61aab7..8ed91aca 100644 --- a/.gitignore +++ b/.gitignore @@ -10,7 +10,7 @@ testbin/* Dockerfile.cross # Binary outputs -outputs/* +dist # docker-buildx .buildx_cache diff --git a/.goreleaser.yml b/.goreleaser.yml new file mode 100644 index 00000000..b6b8d898 --- /dev/null +++ b/.goreleaser.yml @@ -0,0 +1,96 @@ +before: + # https://goreleaser.com/customization/hooks/ + hooks: + - go mod tidy + +builds: + # Manager + - id: manager + main: ./ + binary: manager + env: [CGO_ENABLED=0] + goos: + - linux + goarch: + - amd64 + - arm64 + + # CLI + - id: cli + main: ./ozctl + binary: ozctl + env: [CGO_ENABLED=0] + goos: + - darwin + goarch: + - amd64 + - arm64 + +universal_binaries: + - id: cli + name_template: ozctl + replace: true + +dockers: + # Local image only used for `make docker-load` + - image_templates: ["{{ .Env.IMG }}"] + dockerfile: Dockerfile + use: buildx + skip_push: true + + # Potential Release - AMD64 + - image_templates: ["ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-amd64"] + dockerfile: Dockerfile + goarch: amd64 + use: buildx + build_flag_templates: + - --platform=linux/amd64 + - --label=org.opencontainers.image.title={{ .ProjectName }} + - --label=org.opencontainers.image.description={{ .ProjectName }} + - --label=org.opencontainers.image.url=https://github.com/diranged/{{ .ProjectName }} + - --label=org.opencontainers.image.source=https://github.com/diranged/{{ .ProjectName }} + - --label=org.opencontainers.image.version={{ .Version }} + - --label=org.opencontainers.image.created={{ time "2006-01-02T15:04:05Z07:00" }} + - --label=org.opencontainers.image.revision={{ .FullCommit }} + - --label=org.opencontainers.image.licenses=Apache-2.0 + + # Potential Release - ARM64 + - image_templates: ["ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-arm64v8"] + goarch: arm64 + dockerfile: Dockerfile + use: buildx + build_flag_templates: + - --platform=linux/arm64/v8 + - --label=org.opencontainers.image.title={{ .ProjectName }} + - --label=org.opencontainers.image.description={{ .ProjectName }} + - --label=org.opencontainers.image.url=https://github.com/diranged/{{ .ProjectName }} + - --label=org.opencontainers.image.source=https://github.com/diranged/{{ .ProjectName }} + - --label=org.opencontainers.image.version={{ .Version }} + - --label=org.opencontainers.image.created={{ time "2006-01-02T15:04:05Z07:00" }} + - --label=org.opencontainers.image.revision={{ .FullCommit }} + - --label=org.opencontainers.image.licenses=Apache-2.0 + +docker_manifests: + - id: + name_template: ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }} + image_templates: + - ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-amd64 + - ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-arm64v8 + + - name_template: ghcr.io/diranged/{{ .ProjectName }}:latest + image_templates: + - ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-amd64 + - ghcr.io/diranged/{{ .ProjectName }}:{{ .Version }}-arm64v8 + +# https://goreleaser.com/customization/changelog/ +changelog: + use: github + groups: + - title: Features + regexp: '^.*?feat(\([[:word:]]+\))??!?:.+$' + order: 0 + - title: Bug fixes + regexp: '^.*?fix(\([[:word:]]+\))??!?:.+$' + order: 1 + - title: Others + order: 999 diff --git a/Custom.mk b/Custom.mk index b7f624de..78b09b07 100644 --- a/Custom.mk +++ b/Custom.mk @@ -1,5 +1,11 @@ SOURCE := $(wildcard api/*/*.go controller/*.go ozctl/*.go ozctl/*/*.go) +ifeq (true,$(PUBLISH)) +GORELEASER_FLAGS := --rm-dist +else +GORELEASER_FLAGS := --skip-publish --snapshot --rm-dist +endif + ## Tool Binaries REVIVE_VER ?= v1.2.4 REVIVE ?= $(LOCALBIN)/revive @@ -10,12 +16,14 @@ GOFUMPT ?= $(LOCALBIN)/gofumpt GOLINES_VER ?= v0.11.0 GOLINES ?= $(LOCALBIN)/golines +GORELEASER_VER ?= v1.13.1 +GORELEASER ?= $(LOCALBIN)/goreleaser + GEN_CRD_API_DOCS_VER ?= v0.3.1-0.20220223025230-af7c5e0048a3 GEN_CRD_API_DOCS ?= $(LOCALBIN)/go-crd-api-reference-docs -.PHONY: docker-load -docker-load: - kind load docker-image $(IMG) -n $(KIND_CLUSTER_NAME) +goreleaser: + .PHONY: cover cover: @@ -33,6 +41,11 @@ lint: revive test-e2e: go test ./test/e2e/ -v -ginkgo.v +.PHONY: goreleaser +goreleaser: $(GORELEASER) +$(GORELEASER): + GOBIN=$(LOCALBIN) go install github.com/goreleaser/goreleaser@$(GORELEASER_VER) + .PHONY: gofumpt gofumpt: $(GOFUMPT) $(GOFUMPT): @@ -53,6 +66,18 @@ revive: $(REVIVE) ## Download revive locally if necessary. $(REVIVE): $(LOCALBIN) Custom.mk GOBIN=$(LOCALBIN) go install github.com/mgechev/revive@$(REVIVE_VER) +.PHONY: release +release: $(GORELEASER) + IMG=$(IMG) $(GORELEASER) release $(GORELEASER_FLAGS) + +.PHONY: build +build: $(GORELEASER) + PUBLISH=false $(MAKE) release + +.PHONY: docker-load +docker-load: + kind load docker-image $(IMG) -n $(KIND_CLUSTER_NAME) + gen-crd-api-reference-docs: $(GEN_CRD_API_DOCS) $(GEN_CRD_API_DOCS): GOBIN=$(LOCALBIN) go install github.com/ahmetb/gen-crd-api-reference-docs@$(GEN_CRD_API_DOCS_VER) @@ -65,13 +90,3 @@ godocs: $(GEN_CRD_API_DOCS) -template-dir $$(go env GOMODCACHE)/github.com/ahmetb/gen-crd-api-reference-docs@$(GEN_CRD_API_DOCS_VER)/template \ -out-file API.md \ -v 5 - -##@ Build CLI -.PHONY: cli -cli: outputs/ozctl-osx outputs/ozctl-osx-arm64 - -outputs/ozctl-osx: ozctl controllers api $(SOURCE) - GOOS=darwin GOARCH=amd64 LDFLAGS=$(RELEASE_LDFLAGS) go build -o $@ ./ozctl - -outputs/ozctl-osx-arm64: ozctl controllers api $(SOURCE) - GOOS=darwin GOARCH=arm64 LDFLAGS=$(RELEASE_LDFLAGS) go build -o $@ ./ozctl diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 42b23f11..de6ba875 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -35,7 +35,7 @@ $ kind create cluster install/upgrade the controller: ```sh -$ make docker-build docker-load manifests deploy +$ make release docker-load manifests deploy ... service/oz-controller-manager-metrics-service created deployment.apps/oz-controller-manager created diff --git a/Dockerfile b/Dockerfile index 8f9cca18..82219fd6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,33 +1,5 @@ -# Build the manager binary -FROM golang:1.19 as builder -ARG TARGETOS -ARG TARGETARCH - -WORKDIR /workspace -# Copy the Go Modules manifests -COPY go.mod go.mod -COPY go.sum go.sum -# cache deps before building and copying source so that we don't need to re-download as much -# and so that source changes don't invalidate our downloaded layer -RUN go mod download - -# Copy the go source -COPY main.go main.go -COPY api/ api/ -COPY controllers/ controllers/ - -# Build -# the GOARCH has not a default value to allow the binary be built according to the host where the command -# was called. For example, if we call make docker-build in a local env which has the Apple Silicon M1 SO -# the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, -# by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. -RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o manager main.go - -# Use distroless as minimal base image to package the manager binary -# Refer to https://github.com/GoogleContainerTools/distroless for more details FROM gcr.io/distroless/static:nonroot WORKDIR / -COPY --from=builder /workspace/manager . +COPY manager / USER 65532:65532 - ENTRYPOINT ["/manager"] diff --git a/api/v1alpha1/core_status.go b/api/v1alpha1/core_status.go index c3e0acb2..86f5e0ba 100644 --- a/api/v1alpha1/core_status.go +++ b/api/v1alpha1/core_status.go @@ -1,6 +1,8 @@ package v1alpha1 -import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) // ICoreStatus is used to define the core common status functions that all Status structs in this // API must adhere to. These common functions simplify the reconciler() functions so that they can diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 14733e3f..9ef936e1 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -6,6 +6,7 @@ import ( "strconv" api "github.com/diranged/oz/api/v1alpha1" + "github.com/diranged/oz/controllers/builders" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -13,7 +14,9 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" ) // BaseReconciler extends the default reconciler behaviors (client.Client+Scheme) and provide some @@ -53,11 +56,14 @@ func (r *BaseReconciler) SetReconciliationInterval() { // refetch uses the "consistent client" (non-caching) to retreive the latest state of the object into the // supplied object reference. This is critical to avoid "the object has been modified; please apply // your changes to the latest version and try again" errors when updating object status fields. -func (r *BaseReconciler) refetch(ctx context.Context, obj client.Object) error { - return r.APIReader.Get(ctx, types.NamespacedName{ +func (r *BaseReconciler) refetch(ctx context.Context, obj client.Object) (*client.Object, error) { + if err := r.APIReader.Get(ctx, types.NamespacedName{ Name: obj.GetName(), Namespace: obj.GetNamespace(), - }, obj) + }, obj); err != nil { + return nil, err + } + return &obj, nil } // UpdateStatus pushes the client.Object.Status field into Kubernetes if it has been updated, and @@ -66,17 +72,21 @@ func (r *BaseReconciler) refetch(ctx context.Context, obj client.Object) error { // // This wrapper makes it much easier to update the Status field of an object iteratively throughout // a reconciliation loop. -func (r *BaseReconciler) updateStatus(ctx context.Context, obj client.Object) error { +func (r *BaseReconciler) updateStatus(ctx context.Context, res api.ICoreResource) error { logger := r.getLogger(ctx) // Update the status, handle failure. - if err := r.Status().Update(ctx, obj); err != nil { + logger.V(2). + Info("Pre Obj Json", "resourceVersion", res.GetResourceVersion(), "json", builders.ObjectToJSON(res)) + if err := r.Status().Update(ctx, res); err != nil { logger.Error(err, "Failed to update status") return err } // Refetch the object when we're done to make sure we are working with the latest version - r.refetch(ctx, obj) + r.refetch(ctx, res) + logger.V(2). + Info("Post Obj Json", "resourceVersion", res.GetResourceVersion(), "json", builders.ObjectToJSON(res)) return nil } @@ -97,7 +107,7 @@ func (r *BaseReconciler) updateCondition( ) error { logger := r.getLogger(ctx) logger.V(1). - Info(fmt.Sprintf("Updating condition \"%s\" to \"%s\"", conditionType, conditionStatus)) + Info(fmt.Sprintf("Updating condition %s to %s", conditionType, conditionStatus)) meta.SetStatusCondition(res.GetStatus().GetConditions(), metav1.Condition{ Type: string(conditionType), @@ -149,3 +159,32 @@ func (r *BaseReconciler) getLogger(ctx context.Context) logr.Logger { } return r.logger } + +// ignoreStatusUpdatesAndDeletion filters out reconcile requests where only the +// Status was updated, or on Deletes. +// +// **Deletes** +// On Deletes, we don't need to do any cleanup because we make sure to use +// OwnerReferences that force Kubernetes to handle the cleanup for us. +// +// **Status Updates** +// Our Reconcile() loops make many updates mid-reconcile to the status fields +// of the objects. Doing this can cause all kinds of re-runs of the reconciler +// at a high rate - mostly when they are not desired. +// +// Using this predicate filter means that the Reconcile() loops must be well +// tested and include their own automatic requeue-after settings. +// +// https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/ +func ignoreStatusUpdatesAndDeletion() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + // Ignore updates to CR status in which case metadata.Generation does not change + return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() + }, + DeleteFunc: func(e event.DeleteEvent) bool { + // Evaluates to false if the object has been confirmed deleted. + return !e.DeleteStateUnknown + }, + } +} diff --git a/controllers/base_controller_test.go b/controllers/base_controller_test.go index 9ea8cc8d..3d17e8a0 100644 --- a/controllers/base_controller_test.go +++ b/controllers/base_controller_test.go @@ -24,8 +24,8 @@ type FakeBuilder struct { retErr error } -func (b *FakeBuilder) GenerateAccessResources() (statusString string, accessString string, err error) { - return b.retStatusString, b.retAccessString, b.retErr +func (b *FakeBuilder) GenerateAccessResources() (statusString string, err error) { + return b.retStatusString, b.retErr } var ( diff --git a/controllers/base_request_controller.go b/controllers/base_request_controller.go index f9213401..8f768bc0 100644 --- a/controllers/base_request_controller.go +++ b/controllers/base_request_controller.go @@ -159,38 +159,31 @@ func (r *BaseRequestReconciler) isAccessExpired(builder builders.IBuilder) (bool return false, nil } -// verifyAccessResources calls out to the Builder interface's GenerateAccessResources() method to build out +// verifyAccessResourcesBuilt calls out to the Builder interface's GenerateAccessResources() method to build out // all of the resources that are required for thie particular access request. The Status.Conditions field is // then updated with the ConditionAccessResourcesCreated condition appropriately. -func (r *BaseRequestReconciler) verifyAccessResources( +func (r *BaseRequestReconciler) verifyAccessResourcesBuilt( builder builders.IBuilder, -) (accessString string, err error) { +) error { logger := log.FromContext(builder.GetCtx()) logger.Info("Verifying that access resources are built") - statusString, accessString, err := builder.GenerateAccessResources() + statusString, err := builder.GenerateAccessResources() if err != nil { - builder.GetRequest().GetStatus().SetAccessMessage("") r.updateCondition( builder.GetCtx(), builder.GetRequest(), ConditionAccessResourcesCreated, metav1.ConditionFalse, string(metav1.StatusFailure), fmt.Sprintf("ERROR: %s", err)) - return accessString, err + return err } - - builder.GetRequest().GetStatus().SetAccessMessage(accessString) - if err := r.updateCondition( + return r.updateCondition( builder.GetCtx(), builder.GetRequest(), ConditionAccessResourcesCreated, metav1.ConditionTrue, string(metav1.StatusSuccess), - statusString); err != nil { - return accessString, err - } - - return accessString, nil + statusString) } // verifyAccessResourcesReady is a followup to the verifyAccessResources() @@ -198,33 +191,27 @@ func (r *BaseRequestReconciler) verifyAccessResources( // the way up and reached the "Running" phase. func (r *BaseRequestReconciler) verifyAccessResourcesReady( builder builders.IPodAccessBuilder, -) (accessString string, err error) { +) error { logger := log.FromContext(builder.GetCtx()) logger.Info("Verifying that access resources are ready") statusString, err := builder.VerifyAccessResources() if err != nil { - builder.GetRequest().GetStatus().SetAccessMessage("") r.updateCondition( builder.GetCtx(), builder.GetRequest(), ConditionAccessResourcesReady, metav1.ConditionFalse, "NotYetReady", fmt.Sprintf("%s", err)) - return accessString, err + return err } - builder.GetRequest().GetStatus().SetAccessMessage(accessString) - if err := r.updateCondition( + return r.updateCondition( builder.GetCtx(), builder.GetRequest(), ConditionAccessResourcesReady, metav1.ConditionTrue, string(metav1.StatusSuccess), - statusString); err != nil { - return accessString, err - } - - return accessString, nil + statusString) } // DeleteResource just deletes the resource immediately diff --git a/controllers/base_request_controller_test.go b/controllers/base_request_controller_test.go index c2caa9f1..d254dadc 100644 --- a/controllers/base_request_controller_test.go +++ b/controllers/base_request_controller_test.go @@ -82,14 +82,11 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder.retStatusString = "success" // Build the resources - accessStr, err := r.verifyAccessResources(builder) + err := r.verifyAccessResourcesBuilt(builder) // VERIFY: no error Expect(err).To(Not(HaveOccurred())) - // VERIFY: accessStr is valid - Expect(accessStr).To(Equal("here you go")) - // VERIFY: the conditions in the request object were updated Expect(meta.IsStatusConditionPresentAndEqual( request.Status.Conditions, @@ -111,14 +108,11 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { builder.retStatusString = "failure" // Build the resources - accessStr, err := r.verifyAccessResources(builder) + err := r.verifyAccessResourcesBuilt(builder) // VERIFY: error occurred Expect(err).To(HaveOccurred()) - // VERIFY: accessStr is valid - Expect(accessStr).To(Equal("an error happened")) - // VERIFY: the conditions in the request object were updated Expect(meta.IsStatusConditionPresentAndEqual( request.Status.Conditions, @@ -143,16 +137,13 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() { request.Name = "bogus" // Build the resources - accessStr, err := r.verifyAccessResources(builder) + err := r.verifyAccessResourcesBuilt(builder) // VERIFY: error occurred Expect(err).To(HaveOccurred()) Expect( err.Error(), ).To(Equal("execaccessrequests.crds.wizardofoz.co \"bogus\" not found")) - - // VERIFY: accessStr is valid - Expect(accessStr).To(Equal("here you go")) }) }) diff --git a/controllers/builders/exec_access_builder.go b/controllers/builders/exec_access_builder.go index 05ecc68b..01d9d6f6 100644 --- a/controllers/builders/exec_access_builder.go +++ b/controllers/builders/exec_access_builder.go @@ -43,23 +43,25 @@ var ( // string may go away. // // err: Any errors during the building and application of these resources. -func (b *ExecAccessBuilder) GenerateAccessResources() (statusString string, accessString string, err error) { +func (b *ExecAccessBuilder) GenerateAccessResources() (statusString string, err error) { + var accessString string + // Get the target Pod Name that the user is going to have access to targetPodName, err := b.getPodName() if err != nil { - return statusString, accessString, err + return statusString, err } // Get the Role, or error out role, err := b.createAccessRole(targetPodName) if err != nil { - return statusString, accessString, err + return statusString, err } // Get the Binding, or error out rb, err := b.createAccessRoleBinding() if err != nil { - return statusString, accessString, err + return statusString, err } statusString = fmt.Sprintf("Success. Role %s, RoleBinding %s created", role.Name, rb.Name) @@ -70,8 +72,9 @@ func (b *ExecAccessBuilder) GenerateAccessResources() (statusString string, acce ) b.Request.SetPodName(targetPodName) + b.Request.Status.SetAccessMessage(accessString) - return statusString, accessString, err + return statusString, err } // generatePodName is used to discover the target pod that the user is going to have access to. This diff --git a/controllers/builders/interfaces.go b/controllers/builders/interfaces.go index 3ff2a891..295d6d45 100644 --- a/controllers/builders/interfaces.go +++ b/controllers/builders/interfaces.go @@ -40,7 +40,7 @@ type IBuilder interface { GetTemplate() api.ITemplateResource // Generates all of the resources required to fulfill the access request. - GenerateAccessResources() (statusString string, accessString string, err error) + GenerateAccessResources() (statusString string, err error) // GetTargetRefResource returns a generic but populated client.Object resource from an Access // Template. Typically this is a Deployment, DaemonSet, etc. diff --git a/controllers/builders/pod_access_builder.go b/controllers/builders/pod_access_builder.go index bc79e74d..b70a1914 100644 --- a/controllers/builders/pod_access_builder.go +++ b/controllers/builders/pod_access_builder.go @@ -25,6 +25,12 @@ type PodAccessBuilder struct { Template *api.PodAccessTemplate } +// https://stackoverflow.com/questions/33089523/how-to-mark-golang-struct-as-implementing-interface +var ( + _ IBuilder = &PodAccessBuilder{} + _ IBuilder = (*PodAccessBuilder)(nil) +) + // GenerateAccessResources is the primary function called by the reconciler to this Builder object. This function // is responsible for building all of the temporary access resources, and returning back information about them // to the user. Any error causes this function to stop and fail. @@ -38,14 +44,15 @@ type PodAccessBuilder struct { // string may go away. // // err: Any errors during the building and application of these resources. -func (b *PodAccessBuilder) GenerateAccessResources() (statusString string, accessString string, err error) { +func (b *PodAccessBuilder) GenerateAccessResources() (statusString string, err error) { logger := log.FromContext(b.Ctx) + var accessString string // First, get the desired PodSpec. If there's a failure at this point, return it. podTemplateSpec, err := b.generatePodTemplateSpec() if err != nil { logger.Error(err, "Failed to generate PodSpec for PodAccessRequest") - return statusString, accessString, err + return statusString, err } // Run the PodSpec through the optional mutation config @@ -53,26 +60,26 @@ func (b *PodAccessBuilder) GenerateAccessResources() (statusString string, acces podTemplateSpec, err = mutator.PatchPodTemplateSpec(b.Ctx, podTemplateSpec) if err != nil { logger.Error(err, "Failed to mutate PodSpec for PodAccessRequest") - return statusString, accessString, err + return statusString, err } // Generate a Pod for the user to access pod, err := b.createPod(podTemplateSpec) if err != nil { logger.Error(err, "Failed to create Pod for AccessRequest") - return statusString, accessString, err + return statusString, err } // Get the Role, or error out role, err := b.createAccessRole(pod.GetName()) if err != nil { - return statusString, accessString, err + return statusString, err } // Get the Binding, or error out rb, err := b.createAccessRoleBinding() if err != nil { - return statusString, accessString, err + return statusString, err } statusString = fmt.Sprintf( @@ -87,26 +94,25 @@ func (b *PodAccessBuilder) GenerateAccessResources() (statusString string, acces pod.GetName(), ) - b.Request.Status.PodName = pod.GetName() + b.Request.SetPodName(pod.GetName()) + b.Request.Status.SetAccessMessage(accessString) - return statusString, accessString, err + return statusString, err } // VerifyAccessResources verifies that the Pod created in the // GenerateAccessResources() function is up and in the "Running" phase. func (b *PodAccessBuilder) VerifyAccessResources() (statusString string, err error) { - // logger := log.FromContext(b.Ctx) - // First, verify whether or not the PodName field has been set. If not, // then some part of the reconciliation has previously failed. - if b.Request.Status.PodName == "" { + if b.Request.GetPodName() == "" { return "No Pod Assigned Yet", errors.New("status.podName not yet set") } // Next, get the Pod. If the pod-get fails, then we need to return that failure. pod := &corev1.Pod{} err = b.APIReader.Get(b.Ctx, types.NamespacedName{ - Name: b.Request.Status.PodName, + Name: b.Request.GetPodName(), Namespace: b.Request.Namespace, }, pod) if err != nil { diff --git a/controllers/const.go b/controllers/const.go index d3937204..1ae2488e 100644 --- a/controllers/const.go +++ b/controllers/const.go @@ -40,6 +40,9 @@ const ( // resources" (eg, a Pod) are up and in the ready state. ConditionAccessResourcesReady OzResourceConditionTypes = "AccessResourcesReady" + // ConditionAccessMessage is used to record + ConditionAccessMessage OzResourceConditionTypes = "AccessMessage" + // ConditionTargetRefExists indicates whether or not an AccessTemplate is pointing to a valid // Controller. ConditionTargetRefExists OzResourceConditionTypes = "TargetRefExists" diff --git a/controllers/exec_access_request_controller.go b/controllers/exec_access_request_controller.go index 9d83954c..a64fd46f 100644 --- a/controllers/exec_access_request_controller.go +++ b/controllers/exec_access_request_controller.go @@ -129,7 +129,7 @@ func (r *ExecAccessRequestReconciler) Reconcile( // VERIFICATION: Make sure all of the access resources are built properly. On any failure, // set up a 30 second delay before the next reconciliation attempt. - _, err = r.verifyAccessResources(builder) + err = r.verifyAccessResourcesBuilt(builder) if err != nil { return ctrl.Result{}, err } @@ -204,5 +204,6 @@ func (r *ExecAccessRequestReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&api.ExecAccessRequest{}). + WithEventFilter(ignoreStatusUpdatesAndDeletion()). Complete(r) } diff --git a/controllers/pod_access_request_controller.go b/controllers/pod_access_request_controller.go index 43c4d380..28b22f89 100644 --- a/controllers/pod_access_request_controller.go +++ b/controllers/pod_access_request_controller.go @@ -73,7 +73,7 @@ func (r *PodAccessRequestReconciler) Reconcile( // it doesn't come back, we exit out beacuse it is likely the object has been deleted and we no longer need to // worry about it. logger.Info("Verifying PodAccessRequest exists") - resource, err := api.GetPodAccessRequest(ctx, r.Client, req.Name, req.Namespace) + resource, err := api.GetPodAccessRequest(ctx, r.APIReader, req.Name, req.Namespace) if err != nil { logger.Info(fmt.Sprintf("Failed to find PodAccessRequest %s, perhaps deleted.", req.Name)) return ctrl.Result{}, nil @@ -125,13 +125,13 @@ func (r *PodAccessRequestReconciler) Reconcile( // VERIFICATION: Make sure all of the access resources are built properly. On any failure, // set up a 30 second delay before the next reconciliation attempt. - _, err = r.verifyAccessResources(builder) + err = r.verifyAccessResourcesBuilt(builder) if err != nil { return ctrl.Result{}, err } // VERIFICATION: Make sure the access resources (pod) are actually up and ready - _, err = r.verifyAccessResourcesReady(builder) + err = r.verifyAccessResourcesReady(builder) if err != nil { // SPECIAL TREATMENT: An error here means, requeue this and run it // again in few seconds while we wait for the Pod to come up. It does @@ -145,7 +145,7 @@ func (r *PodAccessRequestReconciler) Reconcile( // TODO: Check the error type. If it's a non-failure, then be more // intelligent return ctrl.Result{ - RequeueAfter: time.Duration(time.Duration(PodWaitReconciliationInterval) * time.Second), + RequeueAfter: time.Duration(PodWaitReconciliationInterval) * time.Second, }, nil } @@ -197,5 +197,6 @@ func (r *PodAccessRequestReconciler) getTargetTemplate( func (r *PodAccessRequestReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&api.PodAccessRequest{}). + WithEventFilter(ignoreStatusUpdatesAndDeletion()). Complete(r) } diff --git a/controllers/pod_access_request_controller_test.go b/controllers/pod_access_request_controller_test.go index 279ec5fc..04d1104e 100644 --- a/controllers/pod_access_request_controller_test.go +++ b/controllers/pod_access_request_controller_test.go @@ -181,13 +181,20 @@ var _ = Describe("PodAccessRequestController", Ordered, func() { // Refetch our Request object... reconiliation has mutated its // .Status fields. - By("Refetching our Request status...") + By("Refetching our Request...") err = k8sClient.Get(ctx, types.NamespacedName{ Name: request.Name, Namespace: request.Namespace, }, request) Expect(err).To(Not(HaveOccurred())) + // Verify that the request access message was set + By("Verifying the Status.AccessMessage is set, but ready state is false") + Expect( + request.Status.AccessMessage, + ).To(MatchRegexp("kubectl exec -ti -n .* .* -- /bin/sh")) + Expect(request.Status.IsReady()).To(BeFalse()) + // Patch the pod status state to "Running" so we can simulate that // the pod is actually up now. By("Patching the Pod State...") diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 38d45f44..0936ccda 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -26,7 +26,7 @@ func TestE2E(t *testing.T) { var _ = BeforeSuite(func() { cmd := exec.Command("kubectl", "create", "ns", namespace) - cmd = exec.Command("make", "docker-build") + cmd = exec.Command("make", "release") _, err := utils.Run(cmd) ExpectWithOffset(1, err).NotTo(HaveOccurred()) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index fb12e226..d2778b96 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -182,6 +182,22 @@ var _ = Describe("oz-controller", Ordered, func() { return err }, time.Minute, time.Second).Should(Succeed()) } + + By("Checking AccessMessage is valid and not empty") + cmd := exec.Command( + "kubectl", + "get", + "-f", + request, + "-n", + namespace, + "-o=jsonpath={.status.accessMessage}", + ) + message, err := utils.Run(cmd) + Expect(err).To(Not(HaveOccurred())) + Expect( + message, + ).To(MatchRegexp("kubectl exec -ti -n oz-system deployment-example-.* -- /bin/sh")) }) }) }) diff --git a/test/utils/utils.go b/test/utils/utils.go index f0ffc266..54a8a21f 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -12,7 +12,7 @@ import ( ) // Run executes the provided command within this context -func Run(cmd *exec.Cmd) ([]byte, error) { +func Run(cmd *exec.Cmd) (string, error) { dir, _ := GetProjectDir() cmd.Dir = dir fmt.Fprintf(GinkgoWriter, "running dir: %s\n", cmd.Dir) @@ -28,10 +28,17 @@ func Run(cmd *exec.Cmd) ([]byte, error) { fmt.Fprintf(GinkgoWriter, "running: %s\n", command) output, err := cmd.CombinedOutput() if err != nil { - return output, fmt.Errorf("%s failed with error: (%v) %s", command, err, string(output)) + return string( + output, + ), fmt.Errorf( + "%s failed with error: (%v) %s", + command, + err, + string(output), + ) } - return output, nil + return string(output), nil } // GetProjectDir will return the directory where the project is