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

fix: parse SGX driver url and compare checksum #2914

Merged

Conversation

Francis-Liu
Copy link
Contributor

Reason for Change:
When intel-sgx driver upgrades, old url would become invalid, thus breaking provision process. This change tries to determine what's the latest driver version by paring the checksum file.

Issue Fixed:
Prevent future issues as of #2743

Requirements:

Notes:
Tested on both Ubuntu 16.04 and 18.04

@acs-bot acs-bot added the size/M label Mar 17, 2020
@mboersma
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Francis-Liu Francis-Liu force-pushed the Francis-Liu/parse-sgx-checksum-url branch from 9892d80 to f8df155 Compare March 17, 2020 19:53
@acs-bot acs-bot added size/L and removed size/M labels Mar 17, 2020
@Francis-Liu
Copy link
Contributor Author

/azp run pr-e2e

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2914 in repo Azure/aks-engine

@Francis-Liu Francis-Liu force-pushed the Francis-Liu/parse-sgx-checksum-url branch from f8df155 to 852f59a Compare March 17, 2020 20:01
@mboersma
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #2914 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2914      +/-   ##
==========================================
- Coverage   72.50%   72.49%   -0.01%     
==========================================
  Files         141      141              
  Lines       25744    25847     +103     
==========================================
+ Hits        18666    18739      +73     
- Misses       5998     6021      +23     
- Partials     1080     1087       +7     
Impacted Files Coverage Δ
pkg/engine/templates_generated.go 33.68% <ø> (ø)
cmd/addpool.go 18.91% <0.00%> (-1.67%) ⬇️
pkg/armhelpers/httpMockClient.go 58.57% <0.00%> (-0.14%) ⬇️
pkg/api/addons.go 97.63% <0.00%> (+<0.01%) ⬆️
pkg/api/types.go 95.63% <0.00%> (+0.01%) ⬆️
cmd/scale.go 12.39% <0.00%> (+0.03%) ⬆️
pkg/api/components.go 96.89% <0.00%> (+0.33%) ⬆️
pkg/engine/params_k8s.go 84.21% <0.00%> (+0.56%) ⬆️
pkg/engine/transform/transform.go 72.45% <0.00%> (+0.69%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e54ece...4a0cb59. Read the comment docs.

paulcallen
paulcallen previously approved these changes Mar 17, 2020
read -ra tmp_array <<< "$(sha256sum ./"$sgx_driver")"
sgx_driver_sha256sum_real="${tmp_array[0]}"
if [ "$sgx_driver_sha256sum_real" != "$sgx_driver_sha256sum_expected" ]; then
# The checksum value is incorrect in download.01.org when the following line of code is written.
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we are short for space in the cloud-init that we deliver, so could we remove these comments?

In fact, if we're ignoring a mismatch, what is the purpose of logging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can remove the comments. Out of curiosity, could you elaborate on what short for space means? From what you suggested, it gives me an impression that the cloud-init script hit a limitation of text length.

This ignoring of checksum is temporary due the the checksum file is wrong at the moment. After Intel uploads a correct checksum file, I will submit another PR to return a checksum error.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing: Azure has a 64k limit for the customData property which delivers the cloud-init payload. All of the files that aks-engine dispatches to the VM are via cloud-init, and thus they all have to fit under 64k (translates to around 87k w/ gzip+base64-encoding)

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Mar 18, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Francis-Liu, jackfrancis

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

@jackfrancis jackfrancis merged commit ce4fc93 into Azure:master Mar 18, 2020
@welcome
Copy link

welcome bot commented Mar 18, 2020

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants