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

Implement jsctl cluster backup command #64

Merged
merged 13 commits into from
Nov 25, 2022
Merged

Implement jsctl cluster backup command #64

merged 13 commits into from
Nov 25, 2022

Conversation

charlieegan3
Copy link
Contributor

@charlieegan3 charlieegan3 commented Nov 15, 2022

This implements a command to generate a cluster back up from the state in the K8S API.

Example output can be found in internal/kubernetes/backup/fixtures/backup.yaml, status and createdTime values come from the k8s yaml generation. These can be removed with a further round of redaction if we'd like.

@charlieegan3 charlieegan3 changed the base branch from main to cluster-clean-cmd November 15, 2022 15:30
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks Charlie, some great work on this again 👏🏼

I've left a couple comments most notably

  • I think this command too should go in experimental section (as again it will most likely be used in production cases)
  • I think we should back up cert-manager alpha/beta resources
  • (unlike what we discussed before) I think we should always just not back up ingress certs, at least I cannot see why it would ever work backing them up

I've not tried the backup yet, but will do when the alpha/beta backup is added if you agree with doing that

internal/command/clusters/backup.go Outdated Show resolved Hide resolved
internal/command/clusters/backup.go Outdated Show resolved Hide resolved
internal/command/clusters/backup.go Show resolved Hide resolved
internal/command/clusters/backup.go Outdated Show resolved Hide resolved
internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

I think this command too should go in experimental section

Addressed in 28f1816

internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
internal/command/clusters/backup.go Outdated Show resolved Hide resolved
@charlieegan3
Copy link
Contributor Author

I've addressed the other comments, I'm going to have a think about how we manage multiple API versions now.

@charlieegan3
Copy link
Contributor Author

As discussed privately, afacf30 I hope addressed the immediate concern for unsupported cert-manager API versions.

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks for making all the changes!

I've not ran all the commands yet, but took a look at the CRD version check and I think it's close 👍🏼

internal/kubernetes/backup/backup.go Outdated Show resolved Hide resolved
Base automatically changed from cluster-clean-cmd to main November 18, 2022 08:49
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

I've ran the command now and it seems to largely work as expected, a couple more things that might need fixing before we can merge:

  • none of the flags seem to work
  • if redirected to file, it gets prefixed with issuer GVK string so is not a valid yaml that could be applied

@charlieegan3
Copy link
Contributor Author

none of the flags seem to work

Oops, in e0bd753 I use the options in the back up function. I have also rebased this to fix the issue with stdout etc not being passed down correctly.

if redirected to file, it gets prefixed with issuer GVK string so is not a valid yaml that could be applied

e0bd753 also addresses this.

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this Charlie, I think the command looks great now.

I've ran a couple examples and most things look good.

I noticed that --format=json flag is not working. Do you think it'd be easy to get that fixed? It would be nice to have this flag as that way we can guide users to use jq (which I find to be a better tool than yq) when re-applying the necessary resources from backup.
Otherwise we could remove the flag, happy to merge this either way.

@charlieegan3
Copy link
Contributor Author

Hmm, that's a good spot 😬

I've updated the command so it works like this now:

 $ go run main.go experimental clusters backup
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  creationTimestamp: null
  name: example-com
  namespace: jetstack-secure
spec:
  dnsNames:
  - example.com
  issuerRef:
    group: cert-manager.io
    kind: Issuer
    name: ca-issuer
  secretName: example-com-tls
status: {}
...arlieegan/Code/jsctl $ go run main.go experimental clusters backup --format=json
{
  "apiVersion": "v1",
  "items": [
    {
      "kind": "Certificate",
      "apiVersion": "cert-manager.io/v1",
      "metadata": {
        "name": "example-com",
        "namespace": "jetstack-secure",
        "creationTimestamp": null
      },
      "spec": {
        "dnsNames": [
          "example.com"
        ],
        "secretName": "example-com-tls",
        "issuerRef": {
          "name": "ca-issuer",
          "kind": "Issuer",
          "group": "cert-manager.io"
        }
      },
      "status": {}
    }
  ],
  "kind": "List"
}...arlieegan/Code/jsctl $ go run main.go experimental clusters backup --format=things
unknown output format: things
exit status 1

Lmk if you have any thoughts on 4bb3468 which adds this change. I thought it made sense to wrap it in a k8s list so that it can be kubectl applied.

@irbekrm
Copy link
Contributor

irbekrm commented Nov 22, 2022

I thought it made sense to wrap it in a k8s list so that it can be kubectl applied.

Why did we need to make this into a list? Not thought about this in depth, but it seems like it'd be nice if both yaml and json would output the same structure.

At the moment the --experimental-issuers-backup-file flag does not parse a list type for any of the issuers (i.e IssuerList) so that'd need to be updated to be able to understand lists else a json output of this command could not be used for restore.

@charlieegan3
Copy link
Contributor Author

Why did we need to make this into a list?

I think that is required to allow it to be piped to kubectl apply -f -. YAML has --- but there's no equivalent in JSON files for kubectl afaik - other than wrapping in a v1 list. Note, this is an unversioned list, not an issuerlist.

At the moment the --experimental-issuers-backup-file flag does not parse a list type

hmm, I can update that to support this unversioned JSON list.

@irbekrm
Copy link
Contributor

irbekrm commented Nov 24, 2022

hmm, I can update that to support this unversioned JSON list.

yeah I've not thought about this in depth, but the output file should be usable to by the command that converts issuers.

@charlieegan3
Copy link
Contributor Author

I have made it possible to use a JSON backup file in 6344414

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this Charlie

I've tested the --experimental-issuers-backup-file flag with JSON output from this command and saw that all works as expected

/lgtm

@charlieegan3
Copy link
Contributor Author

Thanks Irbe!

@charlieegan3 charlieegan3 merged commit aca7474 into main Nov 25, 2022
@charlieegan3 charlieegan3 deleted the cluster-backup branch November 25, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants