Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ require (
github.com/containerd/errdefs v0.1.0 // indirect
github.com/cyberphone/json-canonicalization v0.0.0-20231217050601-ba74d44ecf5f // indirect
github.com/distribution/reference v0.6.0 // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.8.0 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/go-jose/go-jose/v4 v4.0.2 // indirect
Expand Down
30 changes: 30 additions & 0 deletions velero-plugins/clients/clients.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
imagev1 "github.com/openshift/client-go/image/clientset/versioned/typed/image/v1"
ocpirconfigv1 "github.com/openshift/client-go/imageregistry/clientset/versioned/typed/imageregistry/v1"
routev1 "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
securityv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
"k8s.io/client-go/discovery"
appsv1 "k8s.io/client-go/kubernetes/typed/apps/v1"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
Expand Down Expand Up @@ -41,6 +42,10 @@ var routeClientError error
var buildClient *buildv1.BuildV1Client
var buildClientError error

// SCC client
var securityClient securityv1.SecurityV1Interface
var securityClientError error

var inClusterConfig *rest.Config

func SetInClusterConfig(config *rest.Config) {
Expand Down Expand Up @@ -245,6 +250,30 @@ func newOCPImageRegistryConfigClient() (*ocpirconfigv1.ImageregistryV1Client, er
return client, nil
}

// this setup allows for fake client overrides in unit tests
var SecurityClient = getSecurityClient

// SecurityClient returns an openshift SecurityV1Client
func getSecurityClient() (securityv1.SecurityV1Interface, error) {
if securityClient == nil && securityClientError == nil {
securityClient, securityClientError = newSecurityClient()
}
return securityClient, securityClientError
Comment on lines +257 to +261
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and the specific lines mentioned
fd -t f -p "clients\.go" velero-plugins/clients/ | head -5

Repository: openshift/openshift-velero-plugin

Length of output: 110


🏁 Script executed:

# Check the actual content around lines 257-261
head -n 270 velero-plugins/clients/clients.go | tail -n 30

Repository: openshift/openshift-velero-plugin

Length of output: 949


🏁 Script executed:

# Search for unsynchronized lazy init patterns
rg -n -C2 'if .*== nil && .*== nil \{' velero-plugins/clients/clients.go

Repository: openshift/openshift-velero-plugin

Length of output: 2448


🏁 Script executed:

# Check for sync package imports and usage
rg -n 'sync\.Once|sync\.Mutex|"sync"' velero-plugins/clients/clients.go

Repository: openshift/openshift-velero-plugin

Length of output: 59


🏁 Script executed:

# Find all client getter functions to check consistency
rg -n 'func get.*Client\(\)' velero-plugins/clients/clients.go

Repository: openshift/openshift-velero-plugin

Length of output: 147


Fix race condition in lazy-initialized client getters across the file.

Lines 258-259 show an unsynchronized check-then-initialize pattern that is vulnerable to concurrent access races. The same issue exists in at least 9 other client getter functions (CoreClient, ImageClient, DiscoveryClient, RouteClient, BuildClient, AppsClient, OCPAppsClient, OCPConfigClient, OCPImageRegistryConfigClient). When multiple goroutines concurrently call these functions, they can race between the nil check and the initialization assignment, resulting in nondeterministic state.

Use sync.Once to synchronize initialization across all client getter functions. The sync package is not currently imported and must be added.

🔧 Proposed fix (use sync.Once for thread-safe one-time init)
 import (
+	"sync"
 	ocpappsv1 "github.com/openshift/client-go/apps/clientset/versioned/typed/apps/v1"
 	buildv1 "github.com/openshift/client-go/build/clientset/versioned/typed/build/v1"
 	ocpconfigv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
@@
 // SCC client
 var securityClient securityv1.SecurityV1Interface
 var securityClientError error
+var securityClientOnce sync.Once
@@
 func getSecurityClient() (securityv1.SecurityV1Interface, error) {
-	if securityClient == nil && securityClientError == nil {
-		securityClient, securityClientError = newSecurityClient()
-	}
+	securityClientOnce.Do(func() {
+		securityClient, securityClientError = newSecurityClient()
+	})
 	return securityClient, securityClientError
 }
@@
 func init() {
@@
 	securityClient, securityClientError = nil, nil
+	securityClientOnce = sync.Once{}
 }

Apply the same pattern systematically to all 9+ remaining client getters.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@velero-plugins/clients/clients.go` around lines 257 - 261, The
lazy-initialized client getters (e.g., getSecurityClient, getCoreClient,
getImageClient, getDiscoveryClient, getRouteClient, getBuildClient,
getAppsClient, getOCPAppsClient, getOCPConfigClient,
getOCPImageRegistryConfigClient) use an unsynchronized nil-check-then-init
pattern that races under concurrent calls; replace that pattern by introducing a
sync.Once per client (e.g., securityClientOnce) and move the newXxxClient() call
into the Once.Do closure so initialization runs exactly once; add the "sync"
import and ensure each getter returns the stored client and error after the
Once.Do to preserve behavior.

}

func newSecurityClient() (securityv1.SecurityV1Interface, error) {
config, err := GetInClusterConfig()
if err != nil {
return nil, err
}
client, err := securityv1.NewForConfig(config)
if err != nil {
return nil, err
}

return client, nil
}

func init() {
coreClient, coreClientError = nil, nil
imageClient, imageClientError = nil, nil
Expand All @@ -253,4 +282,5 @@ func init() {
buildClient, buildClientError = nil, nil
ocpAppsClient, ocpAppsClientError = nil, nil
appsClient, appsClientError = nil, nil
securityClient, securityClientError = nil, nil
}
5 changes: 5 additions & 0 deletions velero-plugins/common/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ const (
PVOriginalReclaimPolicy string = "migration.openshift.io/orig-reclaim-policy" // Original PersistentVolumeReclaimPolicy
)

// SCC related annotations
const (
SCCPodAnnotation string = "openshift.io/scc" // SCC annotation on pods
)

// DC-related labels/annotations
const (
DCPodDeploymentLabel string = "deployment" // identifies associated RC
Expand Down
47 changes: 46 additions & 1 deletion velero-plugins/pod/backup.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
package pod

import (
"context"
"encoding/json"

"github.com/konveyor/openshift-velero-plugin/velero-plugins/clients"
"github.com/konveyor/openshift-velero-plugin/velero-plugins/common"
"github.com/sirupsen/logrus"
v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
"github.com/vmware-tanzu/velero/pkg/plugin/velero"
corev1API "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
)

// BackupPlugin is a backup item action plugin for Velero
Expand All @@ -27,6 +32,8 @@ func (p *BackupPlugin) AppliesTo() (velero.ResourceSelector, error) {
func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
p.Log.Info("[pod-backup] Entering Pod backup plugin")

var additionalItems []velero.ResourceIdentifier
var err error
pod := corev1API.Pod{}
itemMarshal, _ := json.Marshal(item)
json.Unmarshal(itemMarshal, &pod)
Expand All @@ -39,9 +46,47 @@ func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *v1.Backup) (ru
annotations[common.DCIncludesDMFix] = "true"
pod.Annotations = annotations

if sccName, exists := pod.Annotations[common.SCCPodAnnotation]; exists && len(sccName) > 0 {
p.Log.Infof("[pod-backup] Pod %s has SCC annotation with value: %s", pod.Name, sccName)

sccIdentifier, localerr := p.addSCC(sccName)
if localerr == nil {
additionalItems = append(additionalItems, sccIdentifier)
} else {
// continuing backup despite error, SCCs can be deleted after Pod creation
if errors.IsNotFound(localerr) {
p.Log.Warnf("[pod-backup] SCC %s for pod %s not found: %s", sccName, pod.Name, localerr.Error())
} else {
p.Log.Errorf("[pod-backup] Error adding SCC %s for pod %s: %s", sccName, pod.Name, localerr.Error())
// error will go out of scope otherwise
err = localerr
}
}
}

var out map[string]interface{}
objrec, _ := json.Marshal(pod)
json.Unmarshal(objrec, &out)
item.SetUnstructuredContent(out)
return item, nil, nil
return item, additionalItems, err
}

func (p *BackupPlugin) addSCC(sccName string) (velero.ResourceIdentifier, error) {
securityClient, err := clients.SecurityClient()
if err != nil {
return velero.ResourceIdentifier{}, err
}

scc, err := securityClient.SecurityContextConstraints().Get(context.Background(), sccName, metav1.GetOptions{})
if err != nil {
return velero.ResourceIdentifier{}, err
}
// resource plural can be retrieved from the openshift api but more trouble than it is worth
return velero.ResourceIdentifier{
GroupResource: schema.GroupResource{
Group: "security.openshift.io",
Resource: "securitycontextconstraints",
},
Name: scc.Name,
}, nil
}
188 changes: 188 additions & 0 deletions velero-plugins/pod/backup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
package pod

import (
"context"
"testing"

"github.com/konveyor/openshift-velero-plugin/velero-plugins/clients"
"github.com/konveyor/openshift-velero-plugin/velero-plugins/common"
securityv1 "github.com/openshift/api/security/v1"
fakeSecurityClient "github.com/openshift/client-go/security/clientset/versioned/fake"
security "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
"github.com/vmware-tanzu/velero/pkg/plugin/velero"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
ktesting "k8s.io/client-go/testing"
)

type ErrorType string

const (
NotFoundError ErrorType = "NotFound"
AccessDenied ErrorType = "AccessDenied"
NoError ErrorType = "NoError"
)

func getNewSecurityClient(errorType ErrorType, withSecurityClientError error, objects ...*securityv1.SecurityContextConstraints) func() (security.SecurityV1Interface, error) {
// the fake client has a weird bug where if the object is added to NewSimpleClientSet the resource value is set wrong
// as a result the object will not be found
// [key 0]: schema.GroupVersionResource {Group: "security.openshift.io", Version: "v1", Resource: "securitycontextconstraintses"} <--- should be securitycontextconstraints
cs := fakeSecurityClient.NewSimpleClientset()

// add error if enabled
switch errorType {
case NotFoundError:
cs.Fake.PrependReactor("get", "securitycontextconstraints", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, errors.NewNotFound(schema.GroupResource{Group: "security.openshift.io", Resource: "securitycontextconstraints"}, "test-scc")
})
case AccessDenied:
cs.Fake.PrependReactor("get", "securitycontextconstraints", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, errors.NewForbidden(schema.GroupResource{Group: "security.openshift.io", Resource: "securitycontextconstraints"}, "test-scc", nil)
})
case NoError:
// do nothing, client will return objects as normal
}

return func() (security.SecurityV1Interface, error) {
client := cs.SecurityV1()
for _, object := range objects {
_, localError := client.SecurityContextConstraints().Create(context.Background(), object, metav1.CreateOptions{})
if localError != nil {
return nil, localError
}
}
return client, withSecurityClientError
}
}

func TestExecute_BackupPod(t *testing.T) {
originalSecurityClient := clients.SecurityClient
t.Cleanup(func() {
clients.SecurityClient = originalSecurityClient
})

scc := &securityv1.SecurityContextConstraints{
ObjectMeta: metav1.ObjectMeta{
Name: "test-scc",
},
}

pod := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pod",
Namespace: "test-namespace",
Annotations: map[string]string{
common.SCCPodAnnotation: "test-scc",
},
},
}

tests := []struct {
name string
annotations map[string]string
inducedErrorType ErrorType
expectedSccCount int
shouldErr bool
expectedIdentifiers []velero.ResourceIdentifier
securityClientError error
}{
{
name: "SCC exists and is added to additional items",
inducedErrorType: NoError,
expectedSccCount: 1,
shouldErr: false,
expectedIdentifiers: []velero.ResourceIdentifier{
{
Name: "test-scc",
GroupResource: schema.GroupResource{
Group: "security.openshift.io",
Resource: "securitycontextconstraints",
},
},
},
annotations: map[string]string{
common.SCCPodAnnotation: "test-scc",
},
},
{
name: "SCC does not exist and error is handled",
inducedErrorType: NotFoundError,
expectedSccCount: 0,
shouldErr: false,
expectedIdentifiers: []velero.ResourceIdentifier{},
annotations: map[string]string{
common.SCCPodAnnotation: "test-scc",
},
},
{
name: "error getting SCC for any other reason",
inducedErrorType: AccessDenied,
expectedSccCount: 0,
shouldErr: true,
expectedIdentifiers: []velero.ResourceIdentifier{},
annotations: map[string]string{
common.SCCPodAnnotation: "test-scc",
},
},
{
name: "pod has no annotations",
inducedErrorType: NoError,
expectedSccCount: 0,
shouldErr: false,
expectedIdentifiers: []velero.ResourceIdentifier{},
annotations: nil,
},
{
name: "security client initialize error",
inducedErrorType: NoError,
expectedSccCount: 0,
shouldErr: true,
expectedIdentifiers: []velero.ResourceIdentifier{},
securityClientError: assert.AnError,
annotations: map[string]string{
common.SCCPodAnnotation: "test-scc",
},
},
}

for _, test := range tests {
pod.Annotations = test.annotations

objs := []*securityv1.SecurityContextConstraints{scc}

podData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pod)
assert.NoError(t, err)
unstructuredPod := unstructured.Unstructured{Object: podData}
clients.SecurityClient = getNewSecurityClient(test.inducedErrorType, test.securityClientError, objs...)

backupPlugin := &BackupPlugin{
Log: logrus.WithField("plugin", "pod-backup-test"),
}

_, items, err := backupPlugin.Execute(&unstructuredPod, &velerov1.Backup{})
if test.shouldErr {
assert.Error(t, err, "Test %s NOT errored when should %v", test.name, err)
} else {
assert.NoError(t, err, "Test %s errored when should succeed %v", test.name, err)
}
assert.Len(t, items, test.expectedSccCount, "Test %s Expected %d additional items for the SCC", test.name, test.expectedSccCount)
assert.ElementsMatch(t, items, test.expectedIdentifiers, "Test %s expected additional items to match expected identifiers", test.name)
}
}

func TestAppliesTo(t *testing.T) {
backupPlugin := &BackupPlugin{
Log: logrus.WithField("plugin", "pod-backup-test"),
}

resourceSelector, err := backupPlugin.AppliesTo()
assert.NoError(t, err)
assert.Contains(t, resourceSelector.IncludedResources, "pods", "Expected resource selector to include pods")
}
31 changes: 1 addition & 30 deletions velero-plugins/serviceaccount/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/konveyor/openshift-velero-plugin/velero-plugins/clients"
apisecurity "github.com/openshift/api/security/v1"
security "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
"github.com/sirupsen/logrus"
v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
"github.com/vmware-tanzu/velero/pkg/plugin/velero"
Expand Down Expand Up @@ -37,10 +36,6 @@ func (p *BackupPlugin) AppliesTo() (velero.ResourceSelector, error) {
}, nil
}

// This should be moved to clients package in future
var securityClient *security.SecurityV1Client
var securityClientError error

// Execute copies local registry images into migration registry
func (p *BackupPlugin) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
p.Log.Info("[serviceaccount-backup] Entering ServiceAccount backup plugin")
Expand Down Expand Up @@ -83,7 +78,7 @@ func sccsForSA(log logrus.FieldLogger, item runtime.Unstructured, backup *v1.Bac

// UpdateSCCMap fill scc map with service account as key and SCCs slice as value
func (c *SCCCache) UpdateSCCMap() error {
sClient, err := SecurityClient()
sClient, err := clients.SecurityClient()
if err != nil {
return err
}
Expand Down Expand Up @@ -144,27 +139,3 @@ func addSaNameToMap(nsMap map[string][]apisecurity.SecurityContextConstraints, s

nsMap[saName] = append(nsMap[saName], scc)
}

// This should be moved to clients package in future

// SecurityClient returns an openshift AppsV1Client
func SecurityClient() (*security.SecurityV1Client, error) {
if securityClient == nil && securityClientError == nil {
securityClient, securityClientError = newSecurityClient()
}
return securityClient, securityClientError
}

// This should be moved to clients package in future
func newSecurityClient() (*security.SecurityV1Client, error) {
config, err := clients.GetInClusterConfig()
if err != nil {
return nil, err
}
client, err := security.NewForConfig(config)
if err != nil {
return nil, err
}

return client, nil
}