Juan Hernandez has posted comments on this change.
Change subject: Trusted Compute Pools - Open Attestation integration with oVirt
engine proposal
......................................................................
Patch Set 2: (29 inline comments)
....................................................
File backend/manager/dbscripts/create_tables.sql
Line 279: migration_support INTEGER NOT NULL default 0,
Line 280: userdefined_properties VARCHAR(4000),
Line 281: predefined_properties VARCHAR(4000),
Line 282: min_allocated_mem INTEGER not null default 0, --indicates how much
memory at least VM need to run, value is in MB
Line 283: trust_lvl VARCHAR(20) default '',
It would suggest "trust_level", it is easier to read.
Line 284: CONSTRAINT PK_vm_static PRIMARY KEY(vm_guid)
Line 285: ) WITH OIDS;
Line 286:
Line 287:
....................................................
File backend/manager/dbscripts/upgrade/03_02_0340_add_trust_lvl_to_vm_static.sql
Line 1: select fn_db_add_column('vm_static', 'trust_lvl', 'varchar(255) default
''''');
The column is varchar(20) when the table is created, in create_tables.sql, but
varchar(255) here. Any reason for that mismatch?
....................................................
File backend/manager/modules/bll/pom.xml
Line 172: <dependency>
Line 173: <groupId>org.apache.httpcomponents</groupId>
Line 174: <artifactId>httpcore</artifactId>
Line 175: <version>4.1.2</version>
Line 176: </dependency>
We already use commons-httpclient version 3.1 in other parts of the
application. I understand that it is a bit old, but would it be possible to use
it instead of httpcomponents? If that is not possible or reasonable, would it
be possible to add these dependencies to the dependenciesManagement section to
the root POM, or at least a property for the version number?
Line 177:
Line 178: <dependency>
Line 179: <groupId>org.codehaus.jackson</groupId>
Line 180: <artifactId>jackson-core-asl</artifactId>
Line 178: <dependency>
Line 179: <groupId>org.codehaus.jackson</groupId>
Line 180: <artifactId>jackson-core-asl</artifactId>
Line 181: <version>1.9.4</version>
Line 182: </dependency>
This dependency is already in the dependeciesManagement section of the ROOT
pom, so the version should not be repeated here.
Line 183:
Line 184: </dependencies>
Line 185:
Line 186: <build>
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationCacheManager.java
Line 2:
Line 3: import java.util.HashMap;
Line 4: import java.util.Set;
Line 5:
Line 6: public class AttestationCacheManager {
I would suggest to make this a singleton:
private static final AttestationCacheManager instance =
AttestationCacheManager();
public AttestationCacheManager getInstance() {
return instance;
}
private AttestationCacheManager() {
// No more instances allowed.
}
The make all the methods instance methods instead of static methods.
If you do this then, in the future (not now), we can make it a proper singleton
using CDI the CDI annotation @ApplicationScoped.
Line 7:
Line 8: private static HashMap<String, AttestationValue> attestationValues
= new HashMap<String, AttestationValue>();
Line 9: private static long cacheTimeout = 36000000;//10h
Line 10:
Line 4: import java.util.Set;
Line 5:
Line 6: public class AttestationCacheManager {
Line 7:
Line 8: private static HashMap<String, AttestationValue> attestationValues
= new HashMap<String, AttestationValue>();
Would you consider using ConcurrentHashMap? That way you don't have to deal
with the potentially dangerous synchronization yourself.
Line 9: private static long cacheTimeout = 36000000;//10h
Line 10:
Line 11: public AttestationCacheManager(){
Line 12:
Line 11: public AttestationCacheManager(){
Line 12:
Line 13: }
Line 14:
Line 15: public static boolean isExisted(String key){
The name of this method makes me think that the it is intended to check if a
key existed in the past. Can you rename it to "exists"? I think that it checks
if the key exists now, in present.
Line 16: return attestationValues.containsKey(key);
Line 17: }
Line 18:
Line 19: public static synchronized void addToCache(AttestationValue value){
Line 17: }
Line 18:
Line 19: public static synchronized void addToCache(AttestationValue value){
Line 20: if (!attestationValues.containsKey(value.getHostName()))
Line 21: attestationValues.put(value.getHostName(), value);
If you use a concurrent map you can use:
attestationValues.putIfAbsent(value.getHostName(), value);
Line 22: }
Line 23:
Line 24: public static synchronized void removeFromCache(String key){
Line 25: if (attestationValues.containsKey(key)){
Line 23:
Line 24: public static synchronized void removeFromCache(String key){
Line 25: if (attestationValues.containsKey(key)){
Line 26: attestationValues.remove(key);
Line 27: }
Won't "attestationValues.remove(...)" be enough?
Line 28: }
Line 29:
Line 30: public static AttestationValue getValueFromCache(String key){
Line 31: AttestationValue value = attestationValues.get(key);
Line 31: AttestationValue value = attestationValues.get(key);
Line 32: return value;
Line 33: }
Line 34:
Line 35: public static boolean IsExpired(String key){
Can you rename to "isExpired"? Just to avoid methods starting with uppercase.
Line 36: AttestationValue value = getValueFromCache(key);
Line 37: if (value != null && value.getVtime() != null){
Line 38: if (System.currentTimeMillis() -
value.getVtime().getTime() <= cacheTimeout)
Line 39: return false;
Line 51: cacheValue.setTrustLvl(value.getTrustLvl());
Line 52: cacheValue.setVtime(value.getVtime());
Line 53: }
Line 54: }
Line 55: }
A general concert about caches: when are they cleaned? I mean, we won't
probably have enough hosts to cause a noticeable leak, but I think is good to
ask this question.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationHost.java
Line 33: import org.ovirt.engine.core.utils.log.LogFactory;
Line 34: import org.ovirt.engine.core.searchbackend.DateUtils;
Line 35: import org.ovirt.engine.core.common.businessentities.AttestationResult;
Line 36:
Line 37: public class AttestationHost {
Consider using a singleton instead of all static methods.
Also, I don't understand the name of the class. It makes me think that this
class represents a host that we use for attestation services, but it isn't. I
would suggest to rename it to "AttestationService" or something that reflects
better its purpose. That or add some comments explaining what is the purpose.
Line 38: private static final String POLL_URI = "PollHosts";
Line 39: private static final String HEADER_HOSTS = "hosts";
Line 40: private static final String HEADER_HOST_NAME = "host_name";
Line 41: private static final String HEADER_RESULT = "trust_lvl";
Line 44: private static final String CONTENT_TYPE = "application/json";
Line 45:
Line 46:
Line 47: public static AttestationResult validateHostIsTrusted(VDS vds) {
Line 48: System.out.println("validating....");
Use logging instead of System.out.
Line 49: AttestationResult result = AttestationResult.unknown;
Line 50: String key = vds.gethost_name();
Line 51: if (AttestationCacheManager.isExisted(key)){
Line 52: if (!AttestationCacheManager.IsExpired(key)){
Line 97: DefaultHttpClient httpclient = new DefaultHttpClient();
Line 98: try {
Line 99: URL truststoreUrl = new URL("file://" +
Config.resolveAttestationTrustStorePath());
Line 100: KeyStore trustStore =
KeyStore.getInstance(KeyStore.getDefaultType());
Line 101: FileInputStream instream = new
FileInputStream(truststoreUrl.getFile());
This input stream is never closed, that will generate a leak of file
descriptors.
Line 102: trustStore.load(instream, null);
Line 103: SSLSocketFactory socketFactory = new
SSLSocketFactory(trustStore);
Line 104: Scheme sch = new Scheme("https", PORT, socketFactory);
Line 105:
httpclient.getConnectionManager().getSchemeRegistry().register(sch);
Line 114: log.error(e.getMessage(), e);
Line 115: } catch (IOException e) {
Line 116: log.error(e.getMessage(), e);
Line 117: }finally {
Line 118: httpclient.getConnectionManager().shutdown();
Why do you need to shutdown the connection manager here? As far as I understand
no connection has been created yet, they will be created later. If this
shutdown is required I think it should be after sending the request, in method
attestHosts method.
Line 119: }
Line 120: return httpclient;
Line 121: }
Line 122:
Line 124: JsonFactory jfactory = new JsonFactory();
Line 125: JsonParser jParser;
Line 126: List<AttestationValue> values = new
ArrayList<AttestationValue>();
Line 127: try {
Line 128: jParser = jfactory.createJsonParser(str);
Don't we need to call the "close" method of the jParser object once finished?
Line 129: jParser.nextToken();
Line 130: while (jParser.nextToken() != JsonToken.END_OBJECT){
Line 131:
if(jParser.getCurrentName().equalsIgnoreCase(HEADER_HOSTS)){
Line 132: while(jParser.nextToken() != JsonToken.END_ARRAY &&
jParser.getCurrentToken() != JsonToken.END_OBJECT){
Line 168: toJsonString += "\"" +host +"\"," ;
Line 169: }
Line 170: toJsonString = toJsonString.substring(0,
toJsonString.length()-1);
Line 171: toJsonString += "]}";
Line 172: return toJsonString;
Consider using a StringBuilder to build this string.
Line 173: }
Line 174: private static final Log log =
LogFactory.getLog(AttestationHost.class);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationValue.java
Line 3:
Line 4: import org.ovirt.engine.core.common.businessentities.AttestationResult;
Line 5:
Line 6:
Line 7: public class AttestationValue {
I am missing the "equals" and "hashCode" methods needed by the hash tables
where these objects will be stored.
Line 8:
Line 9: private String hostName;
Line 10: private AttestationResult trustLvl;
Line 11: private Date vtime;
Line 6:
Line 7: public class AttestationValue {
Line 8:
Line 9: private String hostName;
Line 10: private AttestationResult trustLvl;
Consider renaming to "trustLevel", it is easier to read.
Line 11: private Date vtime;
Line 12:
Line 13: public AttestationValue(){
Line 14: trustLvl = AttestationResult.unknown;
Line 7: public class AttestationValue {
Line 8:
Line 9: private String hostName;
Line 10: private AttestationResult trustLvl;
Line 11: private Date vtime;
What is the meaning of this field? Consider renaming it or adding a comment to
explain the meaning.
Line 12:
Line 13: public AttestationValue(){
Line 14: trustLvl = AttestationResult.unknown;
Line 15: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 529: if (getVm().gettrust_lvl()==null
||!getVm().gettrust_lvl().equalsIgnoreCase("trusted")){
Line 530: flag=true;
Line 531: return flag;
Line 532: }
Line 533: try {
Isn't this implemented twice? Once here and another time in Attestation host?
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);
Line 561: }
Line 562: jParser.nextToken();
Line 563: if(jParser.getText().equalsIgnoreCase("trusted"))
Line 564: flag=true;
Line 565: System.out.println("host: " +hostname +" is " +
jParser.getText());
Use logging instead of System.out.
Line 566: }
Line 567: } catch (Exception e) {
Line 568: e.printStackTrace();
Line 569: throw new RuntimeException(e.toString());
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AttestationResult.java
Line 4: public enum AttestationResult {
Line 5: untrusted(0),
Line 6: trusted(1),
Line 7: unknown(2),
Line 8: timeout(3);
Consider making the values of the enum all uppercase.
I also think that the "timeout" value doesn't belong here. Maybe this enum
should be renamed to "TrustLevel" and then the timeout should be handled in a
different way.
Line 9:
Line 10: private int intValue;
Line 11: private static java.util.HashMap<Integer, AttestationResult>
mappings = new HashMap<Integer, AttestationResult>();
Line 12:
Line 27: public static AttestationResult forValue(int value) {
Line 28: return mappings.get(value);
Line 29: }
Line 30:
Line 31: public static int getIntFromString(String trust_lvl){
Whoever uses this method could as well use this:
AttestationResult.valueOf("TRUSTED").intValue()
Line 32: int value = 2;
Line 33: if (trust_lvl.equalsIgnoreCase("trusted"))
Line 34: value = 0;
Line 35: else if (trust_lvl.equalsIgnoreCase("untrusted"))
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 1464: public String getName() {
Line 1465: return getVmName();
Line 1466: }
Line 1467:
Line 1468: public String gettrust_lvl() {
Consider renaming to "getTrustLevel". If the AttestationResult enum is renamed
to TrustLevel then this method shuld return TrustLevel and not String.
Line 1469: return vmStatic.gettrust_lvl();
Line 1470: }
Line 1471:
Line 1472: public void settrust_lvl(String trust_lvl) {
Line 1468: public String gettrust_lvl() {
Line 1469: return vmStatic.gettrust_lvl();
Line 1470: }
Line 1471:
Line 1472: public void settrust_lvl(String trust_lvl) {
Consider renaming to "setTrustLevel".
Line 1473: vmStatic.settrust_lvl(trust_lvl);
Line 1474: }
Line 1475:
Line 1476:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java
Line 68: @Column(name = "host_cpu_flags", nullable = false)
Line 69: private boolean useHostCpuFlags = false;
Line 70:
Line 71: @Column(name = "trust_lvl")
Line 72: private String trust_lvl="";
Consider renaming to "trustLevel".
Line 73:
Line 74: public VmStatic() {
Line 75: setNumOfMonitors(1);
Line 76: initialized = false;
Line 254: public void setCpuPinning(String cpuPinning) {
Line 255: this.cpuPinning = cpuPinning;
Line 256: }
Line 257:
Line 258: public String gettrust_lvl() {
Consider renaming to "getTrustLevel".
Line 259: return trust_lvl;
Line 260: }
Line 261:
Line 262: public void settrust_lvl(String trust_lvl) {
Line 258: public String gettrust_lvl() {
Line 259: return trust_lvl;
Line 260: }
Line 261:
Line 262: public void settrust_lvl(String trust_lvl) {
Consider renaming to "setTrustLevel".
Line 263: this.trust_lvl = trust_lvl;
Line 264: }
Line 265:
Line 266: @Override
--
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <[email protected]>
Gerrit-Reviewer: Dave Chen <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
