Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete Consul custom resources with consul-k8s uninstall #1623

Merged
merged 43 commits into from
Nov 3, 2022
Merged

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Oct 17, 2022

Changes proposed in this PR

  • All custom resources managed by Consul will be fetched before Helm uninstall is run.
  • These custom resources will be sent a deletion request with a timeout of 1 second and 5 retries if the custom resources are not deleted.
  • If custom resources remain after these 5 attempts, their finalizers will be patched to be an empty list, freeing them up to be deleted by Kubernetes.
  • Changed timeout to be a duration var instead of a string that gets parsed to a duration.
  • Fixed the "header" when installing.

How I've tested this PR

  • Unit tests to cover CR fetching, deletion, and patching.
  • Manually tested that CRs are successfully deleted in the following scenarios:
    • Consul deployed with controller.
    • Consul deployed with controller, all Consul servers deleted (making CR deletion impossible) and falling back to patching finalizers to get rid of CRs
    • Consul deployed without controller (but CRs exist from an earlier installation). Deletion of CRs fails without a controller and uninstall falls back to patching finalizers.

There is a case that this does not cover, but I feel it is too much of an edge case to go after. That is, the user deploys Consul with a controller, the user manually deletes the controller deployment. The user does not delete the mutatingwebhookconfiguration for the controller. This will cause the uninstall request to fail. If the user deletes mutatingwebhookconfiguration for the controller, uninstall will succeed.

How I expect reviewers to test this PR:

A good, simple run manual test is

Deploy Consul with this configuration:

  global:
    name: consul
    logLevel: debug
  controller:
    enabled: true
  server:
    replicas: 1

Then create a resource that will create a custom resource:

# server.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: server
spec:
  replicas: 3
  selector:
    matchLabels:
      app: server
  template:
    metadata:
      name: server
      labels:
        app: server
      annotations:
        consul.hashicorp.com/connect-inject: "true"
    spec:
      containers:
        - name: server
          image: docker.mirror.hashicorp.services/hashicorp/http-echo:latest
          args:
            - -text="hello world"
            - -listen=:8080
          ports:
            - containerPort: 8080
              name: http
          resources:
            requests:
              memory: "64Mi"
              cpu: "250m"
            limits:
              memory: "128Mi"
              cpu: "500m"
      serviceAccountName: server
      terminationGracePeriodSeconds: 0
---
apiVersion: v1
kind: Service
metadata:
  name: server
spec:
  selector:
    app: server
  ports:
    - name: http
      port: 80
      targetPort: 8080
---
apiVersion: v1
kind: ServiceAccount
metadata:
  name: server
---
apiVersion: consul.hashicorp.com/v1alpha1
kind: ServiceDefaults
metadata:
  name: server
spec:
  protocol: http

Then perform consul-k8s uninstall using this branch's CLI.

Checklist

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@t-eckert t-eckert force-pushed the te/delete-crds branch 7 times, most recently from 8262db7 to 8e0956e Compare October 21, 2022 02:06
@t-eckert t-eckert force-pushed the te/delete-crds branch 2 times, most recently from 2318167 to a2ea1f0 Compare October 25, 2022 17:22
@t-eckert t-eckert force-pushed the te/delete-crds branch 3 times, most recently from 744f53c to 88e4c2c Compare October 26, 2022 21:21
@t-eckert t-eckert marked this pull request as ready for review October 27, 2022 16:51
@t-eckert t-eckert force-pushed the te/delete-crds branch 2 times, most recently from 60ba1bf to bdd714d Compare October 27, 2022 17:16
@t-eckert t-eckert changed the base branch from main to te/use-hc-go-actions-build October 27, 2022 18:28
@t-eckert t-eckert requested a review from jmurret October 31, 2022 21:41
Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

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

nice work. I have some nits and just the mention of those two remaining uninstall tests that should validate the new output. approving and will let you figure that all out. 🎉

cli/common/utils.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Great work on this, Thomas! I'm leaving some comments in-line, but I don't think they are too significant. Good work!

.github/workflows/test.yml Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall_test.go Outdated Show resolved Hide resolved
cli/cmd/uninstall/uninstall.go Show resolved Hide resolved
cli/common/utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants