feat: copy labels from Disk to Snapshot

[opdude]

It can be very useful to be able to track your snapshots using the same labels are you have on your original disks, this PR copies over all of the labels of the disk and adds a new permission requirement of compute.snapshots.setLabels to allow for that.

[lacannagabriel]

this will be approved? really relevant to have in the plugin, to have this labels a good practice. @reasonerjt @blackpiglet

[davidholsgrove]

+1 went looking for this today, would be a great feature in the plugin

[firuznz]

+1 we need to trace snapshots

[kaovilai]

please create issue if there isnt one on tanzu/velero repo and add changelogs under this path

velero-plugin-for-gcp/hack/release-tools/changelog.sh

Line 23 in 2c4e73b

CHANGELOG_PATH='changelogs/unreleased'

format name <prNumvber>-<ghUsername>

example: https://github.com/vmware-tanzu/velero/blob/f1dcb7ba11f566b7a828c3fa4dcfb66490858337/changelogs/unreleased/8812-Lyndon-Li

[davidholsgrove]

Hi, any chance this can be updated and merged? Having labels applied to our snapshots would be very beneficial.

[opdude]

Sorry this one got lost in my inbox, issue is created here, updated with the release notes, and rebased.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.39%. Comparing base (a9bc7bb) to head (45934b7).

Files with missing lines	Patch %	Lines
velero-plugin-for-gcp/volume_snapshotter.go	0.00%	26 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   28.39%   27.39%   -1.01%     
==========================================
  Files           4        4              
  Lines         574      595      +21     
==========================================
  Hits          163      163              
- Misses        399      420      +21     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[blackpiglet]

@opdude
Thanks for your contribution.
The change looks good to me.

One thing I need to mention here is that Velero team lost the GCP resource to verify the PR already, so please ensure the change works in your environment.

[opdude]

We've been running my patch in production since this PR was created, I'd say it works :). But as stated before it requires the updated permissions which you may want to warn users about.

[kaovilai]

Would it be possible to gracefully fallback to not doing this? Or do some kind of kubectl can-i equivalent?

[kaovilai]

Not a blocker, just perhaps not included in a patch version as this seems to be breaking change.

[opdude]

Would it be possible to gracefully fallback to not doing this? Or do some kind of kubectl can-i equivalent?

I'm not sure there is a GCP equivalent unfortunately. Possibly the best options are either to change the version so it can be in whatever you deem necessary for a breaking change or offer the ability to turn this on/off?

Not sure what would be best for you.

[kaovilai]

You could wrap the snapshot creation with err handling, and catch lack of label perm, then create snapshot without labels.

[kaovilai]

Here's a code example for implementing graceful error handling when the compute.snapshots.setLabels permission is missing:

func (b *VolumeSnapshotter) createSnapshot(snapshotName, volumeID, volumeAZ string, tags map[string]string) (string, error) {
    // ... existing code ...

    snapshot := &compute.Snapshot{
        Name:         snapshotName,
        Description:  getSnapshotTags(tags, disk.Description, b.log),
        SourceDisk:   disk.SelfLink,
        SnapshotType: b.snapshotType,
        Labels:       disk.Labels,
    }

    if b.snapshotLocation != "" {
        snapshot.StorageLocations = []string{b.snapshotLocation}
    }

    // Try creating snapshot with labels
    op, err := b.gce.Snapshots().Insert(b.projectID, snapshot).Do()
    
    // If we get a permission error for labels, retry without them
    if err != nil && isLabelPermissionError(err) {
        b.log.WithError(err).Warn("Missing compute.snapshots.setLabels permission, creating snapshot without labels")
        
        // Retry without labels
        snapshot.Labels = nil
        op, err = b.gce.Snapshots().Insert(b.projectID, snapshot).Do()
        if err != nil {
            return "", errors.WithStack(err)
        }
    } else if err != nil {
        return "", errors.WithStack(err)
    }

    // ... rest of the existing code ...
}

// Helper function to detect label permission errors
func isLabelPermissionError(err error) bool {
    if err == nil {
        return false
    }
    
    // Check for specific GCP permission error
    // This might need adjustment based on actual error format
    errStr := err.Error()
    return strings.Contains(errStr, "compute.snapshots.setLabels") || 
           (strings.Contains(errStr, "permission") && strings.Contains(errStr, "label"))
}

The same pattern should be applied to createRegionSnapshot() as well. This approach ensures:

1. Backward compatibility - existing deployments without the new permission continue to work
2. Feature adoption - users with the permission get the label copying benefit
3. Clear logging - operators know when labels aren't being copied due to permissions
4. No breaking changes - can be safely included in a patch release

This graceful degradation approach allows for a smooth migration path where users can add the permission when ready without breaking existing functionality.

[opdude]

Sorry for the delay here. I've made the changes and tested on our setup both with and without the permissions.