Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Implement file locking during CLI config writes #2357

Closed
1 of 10 tasks
rajathagasthya opened this issue May 12, 2022 · 4 comments · Fixed by #2882
Closed
1 of 10 tasks

Implement file locking during CLI config writes #2357

rajathagasthya opened this issue May 12, 2022 · 4 comments · Fixed by #2882
Labels

Comments

@rajathagasthya
Copy link
Member

rajathagasthya commented May 12, 2022

Bug description

Currently, there is no file locking mechanism when writing CLI config ~/.config/tanzu/config.yaml. This means multiple processes/plugins interacting with the CLI can issue commands that result in concurrent writes. This can get the config file into a bad state.

We've seen this issue a few times in internal tests where the config ended up looking like this (an invalid YAML):

apiVersion: config.tanzu.vmware.com/v1alpha1
clientOptions:
  cli:
    bomRepo: projects.registry.vmware.com/tkg
    compatibilityFilePath: tkg-compatibility
    discoverySources:
    - oci:
        image: projects.registry.vmware.com/tkg/packages/standalone/standalone-plugins:v0.21.0-dev-112-ga404323d_vmware.1
        name: default
    edition: tkg
  features:
    cluster:
      custom-nameservers: "false"
      dual-stack-ipv4-primary: "false"
      dual-stack-ipv6-primary: "false"
    global:
      context-aware-cli-for-plugins: "true"
      package-based-lcm-beta: "false"
    management-cluster:
      aws-instance-types-exclude-arm: "true"
      custom-nameservers: "false"
      dual-stack-ipv4-primary: "false"
      dual-stack-ipv6-primary: "false"
      export-from-confirm: "true"
      import: "false"
      network-separation-beta: "false"
      standalone-cluster-mode: "false"
kind: ClientConfig
metadata:
  creationTimestamp: null
lse"
      network-separation-beta: "false"
      standalone-cluster-mode: "false"
current: tkg-az-multi-mgmt-azure
kind: ClientConfig
metadata:
  creationTimestamp: null
servers:
- managementClusterOpts:
    context: tkg-az-multi-mgmt-azure-admin@tkg-az-multi-mgmt-azure
    path: /home/kubo/.kube-tkg/config
  name: tkg-az-multi-mgmt-azure
  type: managementcluster

To avoid this, we can do two things:

  1. It appears that we are updating the config during a get operation. A config get ideally shouldn't do a write, so we need find a different place to do those updates and let multiple processes read concurrently (with something like an RWLock).
  2. Implement file locking during store operation, so concurrent writes are safe.

There are probably some libraries in Go that lets us do file locking. There was also a proposal to make it part of the standard library, but it appears to be incomplete.

Affected product area (please put an X in all that apply)

  • APIs
  • Addons
  • CLI
  • Docs
  • IAM
  • Installation
  • Plugin
  • Security
  • Test and Release
  • User Experience

Expected behavior

Config writes should be concurrency-safe.

Steps to reproduce the bug

Version (include the SHA if the version is not obvious)

Environment where the bug was observed (cloud, OS, etc)

Relevant Debug Output (Logs, manifests, etc)

@vuil
Copy link
Contributor

vuil commented May 12, 2022

Thanks for posting the issues and providing the details.
I think item 2 is definitely necessary.
Item 1 as well, but just want to note even after addressing it, but depending how the file locking is done, it is likely we will still need some form of locking when reading the config as well.

@rajathagasthya
Copy link
Member Author

Thanks, yes, adjusted description to reflect that. I think we'd want something like an RWLock.

@rajathagasthya
Copy link
Member Author

https://pkg.go.dev/github.com/rogpeppe/go-internal/lockedfile seems like a good candidate to use.

@anujc25
Copy link
Contributor

anujc25 commented Jul 8, 2022

We have already implemented this type of locking while updating the tkg config file. Implementation is done here: https://github.com/vmware-tanzu/tanzu-framework/blob/main/pkg/v1/tkg/utils/lock.go

We can use this implementation to add lock to tanzu configuration as well needed specially when reading the tanzu config file update purpose. We might not need the lock when just reading the tanzu configuration.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
3 participants