Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: add SGX driver installation on C-series VMs #318

Merged
merged 4 commits into from Jan 23, 2019

Conversation

dmitsh
Copy link
Contributor

@dmitsh dmitsh commented Jan 15, 2019

Reason for Change:
Added new ACC (Azure Confidential Compute) distro based on Ubuntu 16.04 image.
Added Intel SGX driver installation on C-series VMs.

Issue Fixed:

Fixes #317

Requirements:

Notes:

@welcome
Copy link

welcome bot commented Jan 15, 2019

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS-Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

@acs-bot acs-bot added the size/L label Jan 15, 2019
@codecov
Copy link

codecov bot commented Jan 15, 2019

Codecov Report

Merging #318 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #318      +/-   ##
=========================================
+ Coverage   53.16%   53.2%   +0.03%     
=========================================
  Files          95      95              
  Lines       14244   14255      +11     
=========================================
+ Hits         7573    7584      +11     
  Misses       6006    6006              
  Partials      665     665

@jackfrancis
Copy link
Member

thx @dmitsh!

pkg/api/common/helper.go Outdated Show resolved Hide resolved
@jackfrancis jackfrancis added this to In progress in backlog Jan 22, 2019
@tariq1890
Copy link
Contributor

Once review comments are addressed. I'll implement these changes in my branch and we should be able to merge this PR then :)

@dmitsh
Copy link
Contributor Author

dmitsh commented Jan 22, 2019

Thanks @jackfrancis ! Addressed.

@dmitsh
Copy link
Contributor Author

dmitsh commented Jan 22, 2019

Thanks @jackfrancis , @tariq1890 .
With the latest changes I've successfully deployed and validated SGX cluster.
The error in AKS Engine VHD Pipeline test seems to be irrelevant - a storage account access error?

@jackfrancis
Copy link
Member

lgtm

@tariq1890 feel free to incorporate into your fork and then merge

Copy link
Contributor

@tariq1890 tariq1890 left a comment

Choose a reason for hiding this comment

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

/lgtm Thanks @dmitsh :)

@tariq1890
Copy link
Contributor

/lgtm

@tariq1890
Copy link
Contributor

/approve

@acs-bot
Copy link

acs-bot commented Jan 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmitsh, tariq1890

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@acs-bot acs-bot merged commit 78832ef into Azure:master Jan 23, 2019
backlog automation moved this from In progress to Done Jan 23, 2019
@welcome
Copy link

welcome bot commented Jan 23, 2019

Congrats on merging your first pull request! 🎉🎉🎉

tariq1890 added a commit to tariq1890/aks-engine that referenced this pull request Jan 23, 2019
@CecileRobertMichon
Copy link
Contributor

Sorry I'm late to the party here but I was looking this change that I missed last week and trying to solve a merge conflict with my PR and I have a few questions:

  • Is this new distro/feature meant to be used by AKS-Engine users? If so can you add it to the documentation in a follow-up PR @dmitsh?
  • Any C-series VM SKUs need to use the ACC1604 distro correct? And vice-versa, that distro needs to be used with C series? If so, we should add validation to ensure that an apimodel with one or the other missing fails validation.
  • Just a small observation: the if $FULL_INSTALL_REQUIRED for installing the driver in custom script is irrelevant because that is only used with the AKS golden image, if using the ACC1604 distro image no software will be pre-installed so FULL_INSTALL_REQUIRED is always true.

@dmitsh
Copy link
Contributor Author

dmitsh commented Jan 29, 2019

Thank you @CecileRobertMichon . I will send another PR.
To your question about VM SKU, ACC1604 is a special distro for Ubuntu 16.04 that supports UEFI, which is used on Gen-2 VMs, which in turn supports SGX driver. So yes, we have to use ACC1604 for C-series and vise versa.
For Ubuntu 18.04 we will be using the standard image from the Azure Image Gallery, as it already supports UEFI.

@dmitsh dmitsh mentioned this pull request Feb 5, 2019
3 tasks
@dmitsh dmitsh deleted the ds-sgx-enabled branch February 5, 2019 01:25
juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019
* added SGX driver installation on C-series VMs

* addressed comments

* added error codes for SGX driver installation
@jjcollinge
Copy link

@dmitsh I see this PR enables the provisioning of ACC machines but doesn't appear to include any modifications to the vanilla Kubernetes distro to make it EPC aware. What is the expected behavior of the Kubernetes scheduler for SGX workloads? What happens when the EPC is full but Kubernetes doesn't know it?

@jjcollinge
Copy link

To answer my own question ^. Looks like this will be made available by a plugin running as a daemonset: https://azure.microsoft.com/en-us/blog/bringing-confidential-computing-to-kubernetes/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Enable SGX driver installation on C-series VMs
6 participants