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 pkg/clioptions/clusterdiscovery/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ func LoadConfig(state *ClusterState) (*ClusterConfiguration, error) {
}
config.ConfigFile = tmpFile.Name()
case state.PlatformStatus.IBMCloud != nil:
config.ProviderName = "ibmcloud"
// Determine if Managed IBM Cloud cluster (ROKS)
if *state.ControlPlaneTopology == configv1.ExternalTopologyMode {
config.IsIBMROKS = true
Expand Down
7 changes: 5 additions & 2 deletions pkg/clioptions/clusterdiscovery/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ import (
// Initialize nutanix as a provider
_ "github.com/openshift/origin/test/extended/util/nutanix"

// Initialize nutanix as a provider
// Initialize ibmcloud as a provider
_ "github.com/openshift/origin/test/extended/util/ibmcloud"

// Initialize external as a provider
_ "github.com/openshift/origin/test/extended/util/external"

// these are loading important global flags that we need to get and set
Expand Down Expand Up @@ -144,7 +147,7 @@ func DecodeProvider(providerTypeOrJSON string, dryRun, discover bool, clusterSta
}
fallthrough

case "azure", "aws", "baremetal", "gce", "vsphere", "alibabacloud", "external":
case "azure", "aws", "baremetal", "gce", "vsphere", "alibabacloud", "ibmcloud", "external":
if clusterState == nil {
clientConfig, err := e2e.LoadConfig(true)
if err != nil {
Expand Down
187 changes: 187 additions & 0 deletions test/extended/storage/const.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
package storage

// ProvisionerType represents the type of CSI provisioner
type ProvisionerType string

const (
// ProvisionerTypeBlock represents block storage provisioners (AWS EBS,Azure Disk, etc.)
ProvisionerTypeBlock ProvisionerType = "block"
// ProvisionerTypeFile represents file storage provisioners (AWS EFS, Azure File, etc.)
ProvisionerTypeFile ProvisionerType = "file"
)

// FeatureSupport defines which features a provisioner supports
type FeatureSupport struct {
SupportsBYOK bool
}

// ProvisionerInfo contains information about a CSI provisioner
type ProvisionerInfo struct {
Name string
Type ProvisionerType
Features FeatureSupport
EncryptionKeyName string // The parameter name for encryption key in StorageClass
ManagedStorageClassNames []string // List of managed/preset storage classes name
}

// PlatformConfig contains configuration for a cloud platform
type PlatformConfig struct {
Provisioners []ProvisionerInfo
DefaultStorageClass string
}

// Platforms maps cloud platforms to their configuration
var Platforms = map[string]PlatformConfig{
"aws": {
Provisioners: []ProvisionerInfo{
{
Name: "ebs.csi.aws.com",
Type: ProvisionerTypeBlock,
Features: FeatureSupport{
SupportsBYOK: true,
},
EncryptionKeyName: "kmsKeyId",
ManagedStorageClassNames: []string{"gp2-csi", "gp3-csi"},
},
{
Name: "efs.csi.aws.com",
Type: ProvisionerTypeFile,
Features: FeatureSupport{},
EncryptionKeyName: "",
ManagedStorageClassNames: []string{"efs-sc"},
},
},
DefaultStorageClass: "gp3-csi",
},
"gce": {
Provisioners: []ProvisionerInfo{
{
Name: "pd.csi.storage.gke.io",
Type: ProvisionerTypeBlock,
Features: FeatureSupport{
SupportsBYOK: true,
},
EncryptionKeyName: "disk-encryption-kms-key",
ManagedStorageClassNames: []string{"standard-csi", "ssd-csi"},
},
{
Name: "filestore.csi.storage.gke.io",
Type: ProvisionerTypeFile,
Features: FeatureSupport{},
EncryptionKeyName: "",
ManagedStorageClassNames: []string{"filestore-csi"},
},
},
DefaultStorageClass: "standard-csi",
},
"azure": {
Provisioners: []ProvisionerInfo{
{
Name: "disk.csi.azure.com",
Type: ProvisionerTypeBlock,
Features: FeatureSupport{
SupportsBYOK: true,
},
EncryptionKeyName: "diskEncryptionSetID",
ManagedStorageClassNames: []string{"managed-csi"},
},
{
Name: "file.csi.azure.com",
Type: ProvisionerTypeFile,
Features: FeatureSupport{},
EncryptionKeyName: "",
ManagedStorageClassNames: []string{"azurefile-csi"},
},
},
DefaultStorageClass: "managed-csi",
},
"ibmcloud": {
Provisioners: []ProvisionerInfo{
{
Name: "vpc.block.csi.ibm.io",
Type: ProvisionerTypeBlock,
Features: FeatureSupport{
SupportsBYOK: true,
},
EncryptionKeyName: "encryptionKey",
ManagedStorageClassNames: []string{
"ibmc-vpc-block-10iops-tier",
"ibmc-vpc-block-5iops-tier",
"ibmc-vpc-block-custom"},
},
},
DefaultStorageClass: "ibmc-vpc-block-10iops-tier",
},
}

// GetProvisionersByPlatform returns all provisioners for a given platform
func GetProvisionersByPlatform(platform string) []ProvisionerInfo {
if config, ok := Platforms[platform]; ok {
return config.Provisioners
}
return nil
}

// GetProvisionerByName finds a provisioner by its name across all platforms
func GetProvisionerByName(provisioner string) *ProvisionerInfo {
for _, config := range Platforms {
for _, p := range config.Provisioners {
if p.Name == provisioner {
return &p
}
}
}
return nil
}

// GetBYOKProvisioners returns provisioners that support BYOK for a given platform
func GetBYOKProvisioners(platform string) []ProvisionerInfo {
var result []ProvisionerInfo
provisioners := GetProvisionersByPlatform(platform)
for _, prov := range provisioners {
if prov.Features.SupportsBYOK {
result = append(result, prov)
}
}
return result
}

// GetProvisionersByType returns provisioners of a specific type for a given platform
func GetProvisionersByType(platform string, provType ProvisionerType) []ProvisionerInfo {
var result []ProvisionerInfo
provisioners := GetProvisionersByPlatform(platform)
for _, prov := range provisioners {
if prov.Type == provType {
result = append(result, prov)
}
}
return result
}

// GetProvisionerNames returns just the names of provisioners for a given platform
func GetProvisionerNames(platform string) []string {
provisioners := GetProvisionersByPlatform(platform)
names := make([]string, len(provisioners))
for i, p := range provisioners {
names[i] = p.Name
}
return names
}

// GetBYOKProvisionerNames returns names of provisioners that support BYOK for a given platform
func GetBYOKProvisionerNames(platform string) []string {
byokProvisioners := GetBYOKProvisioners(platform)
names := make([]string, len(byokProvisioners))
for i, p := range byokProvisioners {
names[i] = p.Name
}
return names
}

// GetDefaultStorageClass returns the default storage class for a given platform
func GetDefaultStorageClass(platform string) string {
if config, ok := Platforms[platform]; ok {
return config.DefaultStorageClass
}
return ""
}
139 changes: 139 additions & 0 deletions test/extended/storage/csi_byok.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package storage

import (
"context"
"fmt"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
e2e "k8s.io/kubernetes/test/e2e/framework"
admissionapi "k8s.io/pod-security-admission/api"

exutil "github.com/openshift/origin/test/extended/util"
)

var _ = g.Describe(`[sig-storage][Feature:BYOK][Jira:"Storage"]`, func() {
defer g.GinkgoRecover()

var (
oc = exutil.NewCLIWithPodSecurityLevel("csi-byok", admissionapi.LevelPrivileged)
cloudProvider = ""
featureProviderSupportProvisioners = []string{}
)

g.BeforeEach(func() {
// Detect cloud provider and set supported provisioners
cloudProvider = e2e.TestContext.Provider
featureProviderSupportProvisioners = GetBYOKProvisionerNames(cloudProvider)

if len(featureProviderSupportProvisioners) == 0 {
g.Skip("Skip for scenario non-supported provisioner!!!")
}

})

g.It("managed storage classes should be set with the specified encryption key", func() {

for _, provisioner := range featureProviderSupportProvisioners {

// Get provisioner info from const
provisionerInfo := GetProvisionerByName(provisioner)
if provisionerInfo == nil {
g.Skip("Provisioner not found in configuration: " + provisioner)
}

// Skipped for test clusters not installed with the BYOK
byokKeyID := getByokKeyIDFromClusterCSIDriver(oc, provisioner)
if len(byokKeyID) == 0 {
g.Skip("Skipped: the cluster is not byok cluster, no key settings in clustercsidriver/" + provisioner)
}
Comment on lines +48 to +51
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n test/extended/storage/csi_byok.go

Repository: openshift/origin

Length of output: 5921


🏁 Script executed:

rg -n "getByokKeyIDFromClusterCSIDriver" test/extended/storage/csi_byok.go

Repository: openshift/origin

Length of output: 214


Differentiate "not BYOK-configured" from API/read failures.

When ClusterCSIDriver fetch fails, the function returns "" and the test skips instead of failing. This masks real infrastructure issues and regressions—API errors should cause test failure, not silent skips.

🔧 Proposed fix
 import (
 	"context"
 	"fmt"
 
 	g "github.com/onsi/ginkgo/v2"
 	o "github.com/onsi/gomega"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	e2e "k8s.io/kubernetes/test/e2e/framework"
 	admissionapi "k8s.io/pod-security-admission/api"
@@
-			byokKeyID := getByokKeyIDFromClusterCSIDriver(oc, provisioner)
-			if len(byokKeyID) == 0 {
+			byokKeyID, byokConfigured, err := getByokKeyIDFromClusterCSIDriver(oc, provisioner)
+			o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("failed to get ClusterCSIDriver/%s", provisioner))
+			if !byokConfigured {
 				g.Skip("Skipped: the cluster is not byok cluster, no key settings in clustercsidriver/" + provisioner)
 			}
@@
-func getByokKeyIDFromClusterCSIDriver(oc *exutil.CLI, provisioner string) string {
+func getByokKeyIDFromClusterCSIDriver(oc *exutil.CLI, provisioner string) (string, bool, error) {
 	clusterCSIDriver, err := oc.AdminOperatorClient().OperatorV1().ClusterCSIDrivers().Get(context.Background(), provisioner, metav1.GetOptions{})
 	if err != nil {
-		e2e.Logf("Failed to get ClusterCSIDriver %s: %v", provisioner, err)
-		return ""
+		if apierrors.IsNotFound(err) {
+			return "", false, nil
+		}
+		return "", false, err
 	}
@@
 	driverConfig := clusterCSIDriver.Spec.DriverConfig
 	if driverConfig.DriverType == "" {
-		return ""
+		return "", false, nil
 	}
@@
 	case "AWS":
 		if driverConfig.AWS != nil {
-			return driverConfig.AWS.KMSKeyARN
+			return driverConfig.AWS.KMSKeyARN, true, nil
 		}
@@
 	case "IBMCloud":
 		if driverConfig.IBMCloud != nil {
-			return driverConfig.IBMCloud.EncryptionKeyCRN
+			return driverConfig.IBMCloud.EncryptionKeyCRN, true, nil
 		}
 	}
 
-	return ""
+	return "", false, nil
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/extended/storage/csi_byok.go` around lines 48 - 51, The test currently
treats an empty result from getByokKeyIDFromClusterCSIDriver as "not BYOK" and
skips, which hides API/read failures; change getByokKeyIDFromClusterCSIDriver to
return (string, error) instead of just string, propagate the error to the caller
in the test (where byokKeyID := getByokKeyIDFromClusterCSIDriver(oc,
provisioner) is called), and update the test to fail (e.g., g.Fatal/g.Fatalf)
when err != nil while only calling g.Skip when err == nil and the returned key
ID is empty; update all call sites accordingly so real API errors surface as
test failures.


g.By("Get preset storageClass names from configuration")
presetStorageClassNames := getPresetStorageClassNamesByProvisioner(provisioner)
if len(presetStorageClassNames) == 0 {
g.Skip(fmt.Sprintf("No preset storage classes configured for provisioner %s. Expected one of: %v",
provisioner, provisionerInfo.ManagedStorageClassNames))
}

g.By("Verify all preset storage classes have encryption key configured")
for _, scName := range presetStorageClassNames {
g.By(fmt.Sprintf("Verifying storage class: %s", scName))

sc, err := oc.AdminKubeClient().StorageV1().StorageClasses().Get(context.Background(), scName, metav1.GetOptions{})
if err != nil {
g.Skip(fmt.Sprintf("Storage class %s not found in cluster: %v", scName, err))
}
Comment on lines +64 to +67
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's see the broader context of this test function
head -n 100 test/extended/storage/csi_byok.go | tail -n 80

Repository: openshift/origin

Length of output: 3168


🏁 Script executed:

# Let's also check the full test function that contains lines 64-67
sed -n '1,100p' test/extended/storage/csi_byok.go | cat -n

Repository: openshift/origin

Length of output: 4283


🏁 Script executed:

# Search for similar storage class retrieval patterns in test files
rg "StorageClasses\(\)\.Get" test/ -A 3 -B 1

Repository: openshift/origin

Length of output: 2315


Fail when expected managed StorageClasses are absent.

Skipping on StorageClasses().Get() error hides regressions in managed class creation. Since presetStorageClassNames is populated from configuration, missing classes indicate a provisioning issue, not an environmental skip condition. This pattern contradicts the test's assertion that "managed storage classes should be set with the specified encryption key"—if they don't exist, that's a test failure. Other storage tests in the suite (e.g., test/extended/storage/storageclass.go) use e2e.Failf() for similar retrieval errors, establishing the correct pattern.

Proposed fix
 				sc, err := oc.AdminKubeClient().StorageV1().StorageClasses().Get(context.Background(), scName, metav1.GetOptions{})
-				if err != nil {
-					g.Skip(fmt.Sprintf("Storage class %s not found in cluster: %v", scName, err))
-				}
+				o.Expect(err).NotTo(o.HaveOccurred(),
+					fmt.Sprintf("expected managed storage class %s to exist", scName))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/extended/storage/csi_byok.go` around lines 64 - 67, The current test
silently skips when oc.AdminKubeClient().StorageV1().StorageClasses().Get(...)
returns an error (using g.Skip), which masks failures; change the behavior to
fail the test instead — replace the g.Skip(...) call for the
StorageClasses().Get error with a test failure call (e.g., e2e.Failf or the test
suite's Failf) and include the storage class name and error in the failure
message; ensure this change is applied around the retrieval of scName so missing
managed storage classes cause a test failure rather than a skip.


// Double check the storage class is properly configured
o.Expect(sc.Provisioner).Should(o.Equal(provisioner),
fmt.Sprintf("Storage class %s provisioner mismatch", sc.Name))

// Verify BYOK key parameter exists
storedKeyID, exists := sc.Parameters[provisionerInfo.EncryptionKeyName]
if !exists {
g.Fail(fmt.Sprintf("Storage class %s does not have BYOK key parameter %s configured",
sc.Name, provisionerInfo.EncryptionKeyName))
}
o.Expect(storedKeyID).Should(o.Equal(byokKeyID),
fmt.Sprintf("Storage class %s has different BYOK key than ClusterCSIDriver", sc.Name))
}
}
})
})

func getByokKeyIDFromClusterCSIDriver(oc *exutil.CLI, provisioner string) string {
clusterCSIDriver, err := oc.AdminOperatorClient().OperatorV1().ClusterCSIDrivers().Get(context.Background(), provisioner, metav1.GetOptions{})
if err != nil {
e2e.Logf("Failed to get ClusterCSIDriver %s: %v", provisioner, err)
return ""
}

driverConfig := clusterCSIDriver.Spec.DriverConfig
if driverConfig.DriverType == "" {
return ""
}

// Extract key ID based on driver type using struct fields
switch driverConfig.DriverType {
case "AWS":
if driverConfig.AWS != nil {
return driverConfig.AWS.KMSKeyARN
}
case "Azure":
if driverConfig.Azure != nil && driverConfig.Azure.DiskEncryptionSet != nil {
// Build the full disk encryption set ID
des := driverConfig.Azure.DiskEncryptionSet
return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/diskEncryptionSets/%s",
des.SubscriptionID, des.ResourceGroup, des.Name)
}
case "GCP":
if driverConfig.GCP != nil && driverConfig.GCP.KMSKey != nil {
// For GCP, return the full KMS key reference or just the key ring based on what's needed
kmsKey := driverConfig.GCP.KMSKey
location := kmsKey.Location
if location == "" {
location = "global"
}
return fmt.Sprintf("projects/%s/locations/%s/keyRings/%s/cryptoKeys/%s",
kmsKey.ProjectID, location, kmsKey.KeyRing, kmsKey.Name)
}
case "IBMCloud":
if driverConfig.IBMCloud != nil {
return driverConfig.IBMCloud.EncryptionKeyCRN
}
}

return ""
}

func getPresetStorageClassNamesByProvisioner(provisioner string) []string {
// Get provisioner info to get managed storage class names from static config
provisionerInfo := GetProvisionerByName(provisioner)
if provisionerInfo == nil {
e2e.Logf("Provisioner not found in configuration: %s", provisioner)
return []string{}
}
return provisionerInfo.ManagedStorageClassNames
}