Skip to content

Conversation

@m1kola
Copy link
Contributor

@m1kola m1kola commented Jul 20, 2021

Which issue this PR addresses:

Work item №9586080.

Otherr PRs related to this work item:

What this PR does / why we need it:

This PR add a new installation step which replaces default StorageClass provided by OCP with a new one which uses disk encryption set (if one supplied by a customer).

Test plan for issue:

PR adds unit tests + Manual tests.

For instructions on how to create a cluster with SSE and encryption at host see #1569.

How to test that default PV is encrypted.

# Create a pod which uses a persistent volume claim without referencing any storage class
cat > test-pvc.yaml<< EOF
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: mypod-with-cmk-encryption-pvc
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 1Gi
---
kind: Pod
apiVersion: v1
metadata:
  name: mypod-with-cmk-encryption
spec:
  containers:
  - name: mypod-with-cmk-encryption
    image: nginx:1.15.5
    resources:
      requests:
        cpu: 100m
        memory: 128Mi
      limits:
        cpu: 250m
        memory: 256Mi
    volumeMounts:
    - mountPath: "/mnt/azure"
      name: volume
  volumes:
    - name: volume
      persistentVolumeClaim:
        claimName: mypod-with-cmk-encryption-pvc
EOF

# Apply the test pod configuration file and set the PVC UID as a variable to query in Azure later.
pvcUid="$(oc apply -f test-pvc.yaml -o jsonpath='{.items[0].metadata.uid}')"

# Determine the full Azure Disk name.
pvName="$(oc get pv pvc-$pvcUid -o jsonpath='{.spec.azureDisk.diskName}')"

az disk show -n $pvName -g $CLUSTER_RESOURCEGROUP -o json --query "[encryption]"

Result must be something like this:

[
  {
    "diskEncryptionSetId": "$DES_RESOURCE_ID",
    "type": "EncryptionAtRestWithCustomerKey"
  }
]

Is there any documentation that needs to be updated for this PR?

We need to update customer facing docs and CLI, but it is out of scope of this work.

@m1kola m1kola force-pushed the 9586080_default_storageclass branch 2 times, most recently from 6c29692 to e2daa4e Compare July 21, 2021 15:32
@jewzaam
Copy link
Contributor

jewzaam commented Jul 21, 2021

CI failures related to Azure/azure-cli#18950 it seems.

@m1kola m1kola force-pushed the 9586080_default_storageclass branch 2 times, most recently from bb1699d to f6dfa46 Compare July 22, 2021 12:00
@m1kola m1kola marked this pull request as ready for review July 22, 2021 12:01
@m1kola m1kola requested review from jewzaam and mjudeikis as code owners July 22, 2021 12:01
@troy0820 troy0820 added priority-medium Medium priority issue or pull request ready-for-review size-small Size small labels Jul 23, 2021
Makdaam
Makdaam previously approved these changes Jul 28, 2021
Copy link
Contributor

@Makdaam Makdaam left a comment

Choose a reason for hiding this comment

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

/lgtm
My only hangup was DiskEncryptionSetID validataion. But that's already covered in static validation of the pattern. It's a reasonable place to stop as any live check to see if the DiskEncryptionSet is usable suffers from time of check/time of use issues.

@m1kola m1kola force-pushed the 9586080_default_storageclass branch from f6dfa46 to 910a72a Compare July 28, 2021 09:32
@m1kola
Copy link
Contributor Author

m1kola commented Jul 28, 2021

Rebased on top of the master to fix python tests.

@troy0820 troy0820 added the LGTM Looks Good To Me label Jul 28, 2021
@mjudeikis mjudeikis merged commit b8fd99a into Azure:master Aug 3, 2021
@m1kola m1kola deleted the 9586080_default_storageclass branch August 3, 2021 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM Looks Good To Me priority-medium Medium priority issue or pull request ready-for-review size-small Size small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants