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

[Bug] Folder name/uid update may fail due to conflict #1171

Closed
JameelB opened this issue Jul 20, 2023 · 1 comment · Fixed by #1256
Closed

[Bug] Folder name/uid update may fail due to conflict #1171

JameelB opened this issue Jul 20, 2023 · 1 comment · Fixed by #1256
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@JameelB
Copy link
Contributor

JameelB commented Jul 20, 2023

Describe the bug
If a user updates the name of a folder managed by a CR via other means, i.e. update the folder name in Grafana directly, the GrafanaFolder controller will attempt to update this. However, if another controller or user creates another folder with the same name as specified in the GrafanaFolder CR before the GrafanaFolder controller can update it, it will fail due to a conflict as it will attempt to update the UID of the new folder created but will fail as another folder already exists with that UID.

Once it's in this state, the GrafanaFolder controller can no longer update any other properties like folder permissions.

Note that the first folder that matches either the uid or the title is deemed as the existing folder to be updated (see code snippet below from grafanafolder_controller.go#L354-L370):

func (r *GrafanaFolderReconciler) Exists(client *grapi.Client, cr *v1beta1.GrafanaFolder) (bool, string, error) {
	title := cr.GetTitle()
	uid := string(cr.UID)

	folders, err := client.Folders()
	if err != nil {
		return false, "", err
	}

	for _, folder := range folders {
		if folder.UID == uid || strings.EqualFold(folder.Title, title) {
			return true, folder.UID, nil
		}
	}

	return false, "", nil
}

It may be the case that the folder first returned is the one with the correct uid. In this case, the GrafanaFolder controller will not attempt to update the title unless the CR was changed due to the following checks in grafanafolder_controller.go#L305-L311:

if !cr.Unchanged() || uid != remoteUID {
	// Update title and replace uid if needed
	err = grafanaClient.UpdateFolder(remoteUID, title, uid)
	if err != nil {
		return err
	}
}

Version
v5.2.0

To Reproduce

  1. Create the following CRs

    GrafanaDashboardExample.yaml
    apiVersion: grafana.integreatly.org/v1beta1
    kind: GrafanaDashboard
    metadata:
    name: grafanadashboard-sample
    spec:
        folder: test-folder
        resyncPeriod: 30s
        instanceSelector:
            matchLabels:
            dashboards: "grafana"
        json: >
            {
            "id": null,
            "title": "Simple Dashboard",
            "tags": [],
            "style": "dark",
            "timezone": "browser",
            "editable": true,
            "hideControls": false,
            "graphTooltip": 1,
            "panels": [],
            "time": {
                "from": "now-6h",
                "to": "now"
            },
            "timepicker": {
                "time_options": [],
                "refresh_intervals": []
            },
            "templating": {
                "list": []
            },
            "annotations": {
                "list": []
            },
            "refresh": "5s",
            "schemaVersion": 17,
            "version": 0,
            "links": []
            }
    GrafanaFolderExample.yaml
    apiVersion: grafana.integreatly.org/v1beta1
    kind: GrafanaFolder
    metadata:
    name: test-folder
    spec:
        instanceSelector:
            matchLabels:
            dashboards: "grafana"
        resyncPeriod: 2m
        permissions: |
            {
            "items": [
                {
                "role": "Admin",
                "permission": 4
                },
                {
                "role": "Editor",
                "permission": 1
                }
            ]}
  2. Wait for the resources to be created. It should create a folder called test-folder and a dashboard should be placed within it called Simple Dashboard.
    image

  3. Rename test-folder to test-folder1
    image

  4. Wait until the GrafanaDashboard is resynced. This will create a new folder called test-folder and move the Simple Dashboard from test-folder1 to test-folder
    image

  5. Wait for the GrafanaFolder controller to update the folder. An error in the operator logs should be shown:

    1.689848567731055e+09	ERROR	GrafanaFolderReconciler	error reconciling folder	{"controller": "grafanafolder", "controllerGroup": "grafana.integreatly.org", "controllerKind": "GrafanaFolder", "GrafanaFolder": {"name":"test-folder","namespace":"grafana"}, "namespace": "grafana", "name": "test-folder", "reconcileID": "9b6710b2-bc24-42c3-becd-5a945f8819b5", "folder": "test-folder", "grafana": "grafana", "error": "status: 409, body: {\"message\":\"a folder/dashboard with the same uid already exists\"}"}
    github.com/grafana-operator/grafana-operator/v5/controllers.(*GrafanaFolderReconciler).Reconcile
    

    The two folders are never updated to the correct state.

Expected behavior
The title should always be updated regardless of whether the CR was changed or not to ensure it's in the correct state.

If the conflict arises, we could do one of the following options:

  1. Grafana operator should delete a duplicate folder if empty.
    • Retrieve a list of the folders that matches the UID and title given in the GrafanaFolder CR.
    • Due to the name and uid uniqueness rule, this should only ever return 2 folders at maximum.
    • Attempt to delete one of the folders if empty.
      • It should prioritize deleting the incorrectly named one first to ensure that no other controllers/users will be able to create a folder with the desired name.
    • If not empty, the status of the GrafanaFolder CR should be updated with a message stating that duplicate folders are found and the incorrectly named folder must be deleted.
      • User can move off any existing manually created dashboards in that folder to others to empty it. The next reconcile will then delete this empty folder.
      • User may also delete the folder manually themselves.
    • This should limit any accidental deletion of dashboards which are not managed by the operator if they were located in one of the duplicated folders.
    • Once one of the folders are deleted, the next reconcile of the GrafanaFolder should update the remaining folder to the correct state successfully.
  2. Grafana operator should delete the incorrectly named folder regardless of whether it's empty or not.
    • Similar to Option 1 but without user intervention.
    • This could cause deletion of dashboards not managed by the operator if the folder was not empty.
  3. Enforce creation of folders via the GrafanaFolder CR.
    • The GrafanaDashboard controller should not create the folder as specified in the CR, instead it should only wait until that folder exists.
    • The GrafanaFolder CR should have the sole responsibility of creating folders.
    • This is a breaking change.
    • This would not solve the issue if the folder is renamed and a new folder was manually created with the desired name to replace it.

Suspect component/Location where the bug might be occurring

Runtime (please complete the following information):

  • OS: Mac
  • Grafana Operator Version: v5.2.0
  • Environment: OpenShift v4.12.13, Kubernetes v1.25.8+27e744f
  • Deployment type: Operator running locally
  • Other: N/A

Additional context

Workaround

To get around this issue, delete the incorrectly named folder. This ensures that no other controllers/users can create the folder with the same name. The GrafanaFolder controller should then be able to update this folder with the correct UID and permissions.

@JameelB JameelB added bug Something isn't working needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 20, 2023
@pb82 pb82 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 8, 2023
@JameelB
Copy link
Contributor Author

JameelB commented Sep 18, 2023

As discussed, i've noted that users should not update the name directly on Grafana and added the workaround in the Grafana Folder documentation.

The PR to add this is linked above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants