Doron Fediuck has posted comments on this change.
Change subject: Trusted Compute Pools - Open Attestation integration with oVirt
engine proposal
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(6 inline comments)
Can you please point us to the design? This seems to be missing quite a few
issues.
Please see all relevant comments inline.
My major concern here is that as this is suggested it will surely cause a
performance issue when running multiple VMs.
We need a general understanding on how do you see it work.
Additionally you need to consider adding trust level support to templates and
ovf functionalities (import and export).
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 518: vmNICs =
getVmNetworkInterfaceDao().getAllForVm(getVm().getId());
Line 519: }
Line 520: return vmNICs;
Line 521: }
Line 522: private static boolean validateHostIsTrusted(VDS curVds) {
+1 on Itamar's comment. Opening http connections withing the business logic
looks wrong. I'd expect this to be in an AttestationBroker, similar to DB
broker or AD broker.
Also, this validates the host for every vm we want to run, which means the same
host may be verified many times even if we already know it's safe to be used.
So some caching here is much needed. It may also create a big performance hit
as every vm now needs to wait for a response from a remote server, so running
100 VMs will either kill the attestation server or take 100x30 sec's to start
running, which is 50 minutes..... 30 is the default tcp timeout.
Which leads me to ask you to make the connection timeout configurable, so if we
cannot connect within 2 seconds, this should fail. Most other hard-coded values
probably also should be configurable.
Line 523: String attestationWSURL, trustStorePath;
Line 524: attestationWSURL = Config.<String>
GetValue(ConfigValues.AttestationWebServicesUrl);
Line 525: trustStorePath=Config.<String>
GetValue(ConfigValues.TrustStore);
Line 526: DefaultHttpClient httpclient = new DefaultHttpClient();
Line 533: try {
Line 534: KeyStore trustStore =
KeyStore.getInstance(KeyStore.getDefaultType());
Line 535: FileInputStream instream = new FileInputStream(new
File(trustStorePath +"/TrustStore.jks"));
Line 536: trustStore.load(instream, null);
Line 537: SSLSocketFactory socketFactory = new
SSLSocketFactory(trustStore);
I'm quite sure such functionality already exists in the engine, why not re-use?
Line 538: Scheme sch = new Scheme("https", 8443, socketFactory);
Line 539:
httpclient.getConnectionManager().getSchemeRegistry().register(sch);
Line 540: HttpPost httpPost = new HttpPost(attestationWSURL
+"/PollHosts");
Line 541: httpPost.setEntity(new StringEntity("{\"hosts\":[\""
+hostname +"\"]}"));
Line 564: flag=true;
Line 565: System.out.println("host: " +hostname +" is " +
jParser.getText());
Line 566: }
Line 567: } catch (Exception e) {
Line 568: e.printStackTrace();
We do not printStackTrace in the engine.
1. Please make sure to catch the relevant exception you expect, and handle
accordingly.
2. Logging should be used with the relevant verbosity to document what went
wrong.
Line 569: throw new RuntimeException(e.toString());
Line 570: }finally {
Line 571: httpclient.getConnectionManager().shutdown();
Line 572: }
Line 567: } catch (Exception e) {
Line 568: e.printStackTrace();
Line 569: throw new RuntimeException(e.toString());
Line 570: }finally {
Line 571: httpclient.getConnectionManager().shutdown();
What happens if the above statement produces an exception?
Who do you expect to handle it and with which error code / explanation on how
to resolve it?
Line 572: }
Line 573: return flag;
Line 574: }
Line 575: private static final Log log =
LogFactory.getLog(VdsSelector.class);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 1471:
Line 1472: public void settrust_lvl(String trust_lvl) {
Line 1473: vmStatic.settrust_lvl(trust_lvl);
Line 1474: }
Line 1475:
Trailing white spaces are forbidden when working with git.
Line 1476:
Line 1477: @Override
Line 1478: public String toString() {
Line 1479: return "VM [" + getName() + "]";
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1337: AttestationWebServicesUrl(411),
Line 1338:
Line 1339: @TypeConverterAttribute(String.class)
Line 1340: @DefaultValueAttribute("/etc/pki/ovirt-engine/certs")
Line 1341: TrustStore(412),
This should be removed.
Please use the already existing TruststoreUrl.
Line 1342:
Line 1343: Invalid(65535);
Line 1344:
Line 1345: private int intValue;
--
To view, visit http://gerrit.ovirt.org/11237
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4de780cd46069638433255f3f9c994575f752e55
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
