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
16 changes: 12 additions & 4 deletions internal/ocm/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,17 @@ func convertEnableEncryptionAtHostToCSBuilder(in coreapi.NodePoolPlatformProfile
return arohcpv1alpha1.NewAzureNodePoolEncryptionAtHost().State(state)
}

func buildCSOsDisk(osDisk coreapi.OSDiskProfile, storageAccountType, persistence string) *arohcpv1alpha1.AzureNodePoolOsDiskBuilder {
builder := arohcpv1alpha1.NewAzureNodePoolOsDisk().
SizeGibibytes(int(*osDisk.SizeGiB)).
StorageAccountType(storageAccountType).
Persistence(persistence)
if osDisk.EncryptionSetID != nil {
builder.SseEncryptionSetResourceId(osDisk.EncryptionSetID.String())
}
return builder
}

func convertClusterImageRegistryStateRPToCS(in coreapi.ClusterImageRegistryProfile) (string, error) {
switch in.State {
case metadataapi.ClusterImageRegistryStateDisabled:
Expand Down Expand Up @@ -629,10 +640,7 @@ func BuildCSNodePool(ctx context.Context, nodePool *coreapi.HCPOpenShiftClusterN
ResourceName(strings.ToLower(nodePool.Name)).
VMSize(nodePool.Properties.Platform.VMSize).
EncryptionAtHost(convertEnableEncryptionAtHostToCSBuilder(nodePool.Properties.Platform)).
OsDisk(arohcpv1alpha1.NewAzureNodePoolOsDisk().
SizeGibibytes(int(*nodePool.Properties.Platform.OSDisk.SizeGiB)).
StorageAccountType(csDiskStorageAccountType).
Persistence(csPersistence))).
OsDisk(buildCSOsDisk(nodePool.Properties.Platform.OSDisk, csDiskStorageAccountType, csPersistence))).
AvailabilityZone(nodePool.Properties.Platform.AvailabilityZone).
AutoRepair(nodePool.Properties.AutoRepair)
}
Expand Down
29 changes: 29 additions & 0 deletions internal/ocm/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,35 @@ func TestBuildCSNodePool(t *testing.T) {
),
),
},
{
name: "passes disk encryption set ID to CS",
hcpNodePool: getHCPNodePoolResource(
func(hsc *coreapi.HCPOpenShiftClusterNodePool) {
hsc.Properties.Platform.OSDisk.EncryptionSetID = metadataapi.Must(azcorearm.ParseResourceID(
"/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Compute/diskEncryptionSets/test-des"))
},
),
expectedCSNodePool: getBaseCSNodePoolBuilder().
AzureNodePool(arohcpv1alpha1.NewAzureNodePool().
ResourceName("").
VMSize("").
EncryptionAtHost(
arohcpv1alpha1.NewAzureNodePoolEncryptionAtHost().
State(csEncryptionAtHostStateDisabled),
).
OsDisk(arohcpv1alpha1.NewAzureNodePoolOsDisk().
SizeGibibytes(64).
StorageAccountType(string(metadataapi.DiskStorageAccountTypePremium_LRS)).
Persistence("persistent").
SseEncryptionSetResourceId("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Compute/diskEncryptionSets/test-des"),
),
),
},
{
name: "nil disk encryption set ID does not set SSE field",
hcpNodePool: getHCPNodePoolResource(),
expectedCSNodePool: getBaseCSNodePoolBuilder(),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
13 changes: 12 additions & 1 deletion test/e2e-setup/bicep/modules/customer-infra.bicep
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ param privateKeyVault bool = false
@description('Assign Key Vault Crypto Officer role to the deployer on the customer KeyVault that contains etcd encryption key')
param assignKeyVaultCryptoOfficer bool = false

@description('Enable soft delete on the customer Key Vault')
param enableKeyVaultSoftDelete bool = false

@description('Enable purge protection on the customer Key Vault (requires soft delete)')
param enableKeyVaultPurgeProtection bool = false

@description('Soft delete retention in days')
param keyVaultSoftDeleteRetentionInDays int = 7

//
// Variables
//
Expand Down Expand Up @@ -99,7 +108,9 @@ resource customerKeyVault 'Microsoft.KeyVault/vaults@2024-12-01-preview' = {
location: resourceGroup().location
properties: {
enableRbacAuthorization: true
enableSoftDelete: false
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
Comment on lines +111 to +113
tenantId: subscription().tenantId
publicNetworkAccess: privateKeyVault ? 'Disabled' : 'Enabled'
sku: {
Expand Down
175 changes: 175 additions & 0 deletions test/e2e/nodepool_osdisk_encryption.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
// Copyright 2026 Microsoft Corporation
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package e2e

import (
"context"
"strings"

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

azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"

hcpsdk20251223preview "github.com/Azure/ARO-HCP/test/sdk/v20251223preview/resourcemanager/redhatopenshifthcp/armredhatopenshifthcp"
"github.com/Azure/ARO-HCP/test/util/framework"
"github.com/Azure/ARO-HCP/test/util/labels"
"github.com/Azure/ARO-HCP/test/util/verifiers"
)

var _ = Describe("Nodepool OS Disk Encryption", func() {
It("should create a nodepool with customer-managed disk encryption via DES",
labels.RequireNothing,
labels.Critical,
labels.Positive,
labels.AroRpApiCompatible,
labels.MIContainers(1),
Comment on lines +38 to +39

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
labels.AroRpApiCompatible,
labels.MIContainers(1),
labels.AroRpApiCompatible,
labels.IntegrationOnly,
labels.MIContainers(1),

This should still fail in Staging and Production, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This should pass in all envs, including stage and prod, but I'm not able to confirm in INT because cluster identities in INT use the aro-hcp-int-msi-mock service principal, which grants everything diskEncryptionSets/read over the entire subscription (see role assignments, and the role definition, which gets permission over the whole sub)

In stage/prod, each identity gets its built-in role with minimally scoped permissions. Is there a way to run this e2e test in prod before merge to validate the fix?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

mvacula02 I don't believe /test stage-e2e-parallel prod-e2e-parallel will work, since the e2e test here depends on the ocm/internal changes being deployed there. Steve suggested these steps:

  1. add code to the RP to handle the new feature, behind an AFEC flag, such that it can be deployed to all envs without impacting production
  2. deploy RP
  3. write a test to use it, since the test subs (and only the test subs) have the AFEC
  4. once validated, remove requirement on afec and allow users

We may already have an existing AFEC flag we can use to deploy the RP changes safely, and then repurpose this PR to just be for the test later. I'll look into it, if that all sounds OK.

func(ctx context.Context) {
const (
customerClusterName = "des-encrypt"
customerNodePoolName = "des-np"
)

tc := framework.NewTestContext()

if tc.UsePooledIdentities() {
err := tc.AssignIdentityContainers(ctx, 1, framework.IdentityContainerAssignmentRetryInterval)
Expect(err).NotTo(HaveOccurred(), "failed to assign pooled identity containers")
}

By("creating a resource group")
resourceGroup, err := tc.NewResourceGroup(ctx, "des-encrypt", tc.Location())
Expect(err).NotTo(HaveOccurred(), "failed to create resource group")

By("creating cluster parameters")
clusterParams := framework.NewDefaultClusterParams20251223()
clusterParams.ClusterName = customerClusterName
managedResourceGroupName := framework.SuffixName(*resourceGroup.Name, "-managed", 64)
clusterParams.ManagedResourceGroupName = managedResourceGroupName

By("creating customer resources (infrastructure and managed identities)")
clusterParams, err = tc.CreateClusterCustomerResources20251223(ctx,
resourceGroup,
clusterParams,
map[string]interface{}{
"assignKeyVaultCryptoOfficer": true,
"enableKeyVaultSoftDelete": true,
"enableKeyVaultPurgeProtection": true,
"keyVaultSoftDeleteRetentionInDays": 7,
},
TestArtifactsFS,
framework.RBACScopeResourceGroup,
)
Expect(err).NotTo(HaveOccurred(), "failed to create cluster customer resources")

By("resolving service managed identity principal ID")
Expect(clusterParams.UserAssignedIdentitiesProfile).NotTo(BeNil(), "cluster params missing UserAssignedIdentitiesProfile")
Expect(clusterParams.UserAssignedIdentitiesProfile.ServiceManagedIdentity).NotTo(BeNil(), "cluster params missing ServiceManagedIdentity resource ID")

serviceMIResourceID, err := azcorearm.ParseResourceID(*clusterParams.UserAssignedIdentitiesProfile.ServiceManagedIdentity)
Expect(err).NotTo(HaveOccurred(), "failed to parse service managed identity resource ID")

subscriptionID, err := tc.SubscriptionID(ctx)
Expect(err).NotTo(HaveOccurred(), "failed to get subscription ID")

creds, err := tc.AzureCredential()
Expect(err).NotTo(HaveOccurred(), "failed to get Azure credentials")

msiClientFactory, err := armmsi.NewClientFactory(subscriptionID, creds, nil)
Expect(err).NotTo(HaveOccurred(), "failed to create MSI client factory")

serviceMI, err := msiClientFactory.NewUserAssignedIdentitiesClient().Get(ctx, serviceMIResourceID.ResourceGroupName, serviceMIResourceID.Name, nil)
Expect(err).NotTo(HaveOccurred(), "failed to get service managed identity")
Expect(serviceMI.Properties.PrincipalID).NotTo(BeNil(), "service managed identity has no principal ID")

By("resolving cluster-api-azure managed identity principal ID")
Expect(clusterParams.UserAssignedIdentitiesProfile.ControlPlaneOperators).NotTo(BeNil(), "cluster params missing ControlPlaneOperators")
clusterAPIAzureResourceIDStr := clusterParams.UserAssignedIdentitiesProfile.ControlPlaneOperators["cluster-api-azure"]
Expect(clusterAPIAzureResourceIDStr).NotTo(BeNil(), "cluster params missing cluster-api-azure identity")
clusterAPIAzureResourceID, err := azcorearm.ParseResourceID(*clusterAPIAzureResourceIDStr)
Expect(err).NotTo(HaveOccurred(), "failed to parse cluster-api-azure resource ID")
clusterAPIAzureMI, err := msiClientFactory.NewUserAssignedIdentitiesClient().Get(ctx, clusterAPIAzureResourceID.ResourceGroupName, clusterAPIAzureResourceID.Name, nil)
Expect(err).NotTo(HaveOccurred(), "failed to get cluster-api-azure managed identity")
Expect(clusterAPIAzureMI.Properties.PrincipalID).NotTo(BeNil(), "cluster-api-azure managed identity has no principal ID")

By("creating disk encryption set backed by KeyVault")
desResourceID, err := tc.CreateDiskEncryptionSet(ctx, *resourceGroup.Name, clusterParams.KeyVaultName, customerClusterName, tc.Location(), *serviceMI.Properties.PrincipalID, *clusterAPIAzureMI.Properties.PrincipalID)
Expect(err).NotTo(HaveOccurred(), "failed to create disk encryption set")

By("creating the HCP cluster")
err = tc.CreateHCPClusterFromParam20251223(ctx,
GinkgoLogr,
*resourceGroup.Name,
clusterParams,
nil,
framework.ClusterCreationTimeout,
)
Expect(err).NotTo(HaveOccurred(), "failed to create HCP cluster %s", customerClusterName)

By("creating the nodepool with disk encryption set")
nodePoolParams := framework.NewDefaultNodePoolParams20251223()
nodePoolParams.ClusterName = customerClusterName
nodePoolParams.NodePoolName = customerNodePoolName
nodePoolParams.EncryptionSetID = desResourceID

err = tc.CreateNodePoolFromParam20251223(ctx,
GinkgoLogr,
*resourceGroup.Name,
managedResourceGroupName,
customerClusterName,
nodePoolParams,
framework.NodePoolCreationTimeout,
)
Expect(err).NotTo(HaveOccurred(), "failed to create nodepool %s with DES", customerNodePoolName)

By("verifying nodepool ARM resource has encryptionSetId")
created, err := framework.GetNodePool20251223(ctx,
tc.Get20251223ClientFactoryOrDie(ctx).NewNodePoolsClient(),
*resourceGroup.Name,
customerClusterName,
customerNodePoolName,
)
Expect(err).NotTo(HaveOccurred(), "failed to get nodepool %s", customerNodePoolName)
Expect(created.Properties).ToNot(BeNil(), "nodepool Properties was nil")
Expect(created.Properties.ProvisioningState).ToNot(BeNil(), "nodepool ProvisioningState was nil")
Expect(*created.Properties.ProvisioningState).To(Equal(hcpsdk20251223preview.ProvisioningStateSucceeded), "nodepool %s should be Succeeded", customerNodePoolName)
Expect(created.Properties.Platform).ToNot(BeNil(), "nodepool Platform was nil")
Expect(created.Properties.Platform.OSDisk).ToNot(BeNil(), "nodepool OSDisk was nil")
Expect(created.Properties.Platform.OSDisk.EncryptionSetID).ToNot(BeNil(),
"nodepool OSDisk.EncryptionSetID should be set")
Expect(strings.EqualFold(*created.Properties.Platform.OSDisk.EncryptionSetID, desResourceID)).To(BeTrue(),
"nodepool OSDisk.EncryptionSetID should match the DES resource ID")

By("getting credentials to verify cluster health")
adminRESTConfig, err := tc.GetAdminRESTConfigForHCPCluster20240610(
ctx,
tc.Get20240610ClientFactoryOrDie(ctx).NewHcpOpenShiftClustersClient(),
*resourceGroup.Name,
customerClusterName,
framework.GetAdminRESTConfigTimeout,
)
Expect(err).NotTo(HaveOccurred(), "failed to get admin REST config")

By("verifying cluster health, node readiness, and VM OS disk encryption in parallel")
computeFactory := tc.GetARMComputeClientFactoryOrDie(ctx)
err = verifiers.VerifyHCPCluster(ctx, adminRESTConfig,
verifiers.VerifyNodeCount(customerClusterName, int(nodePoolParams.Replicas)),
verifiers.VerifyNodesReady(),
verifiers.VerifyVMOSDiskCustomerEncryption(computeFactory, managedResourceGroupName, customerNodePoolName, desResourceID),
)
Expect(err).NotTo(HaveOccurred(), "cluster verification failed")
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ Engineering should be able to retrieve kusto logs for a cluster and services
Customer should be able to create a cluster with default autoscaling and a nodepool with autoscaling enabled up to replica limits
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should be able to update nodepool replicas and autoscaling
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Customer should be able to create a cluster with default autoscaling and a nodep
Customer should respect cluster-wide node limits with nodepool autoscaling
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.20.z to 4.20.zLatest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Customer should be able to create a cluster with default autoscaling and a nodep
Customer should respect cluster-wide node limits with nodepool autoscaling
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.20.z to 4.20.zLatest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Customer should be able to create a cluster with default autoscaling and a nodep
Customer should respect cluster-wide node limits with nodepool autoscaling
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should be able to update nodepool replicas and autoscaling
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ Customer should be able to create a cluster with default autoscaling and a nodep
Customer should respect cluster-wide node limits with nodepool autoscaling
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.20.z to 4.20.zLatest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Customer should be able to create a cluster with default autoscaling and a nodep
Customer should respect cluster-wide node limits with nodepool autoscaling
Nodepool Ephemeral OS Disk should create a nodepool with ephemeral OS disk when autoRepair is enabled
Customer should be able to update node pool labels and taints
Nodepool OS Disk Encryption should create a nodepool with customer-managed disk encryption via DES
Customer should upgrade and update a nodepool from 4.20.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.21.z to 4.21.zLatest
Customer should upgrade and update a nodepool from 4.20.z to 4.20.zLatest
Expand Down
Loading