Mike Kolesnik has posted comments on this change.
Change subject: WIP Support foreman SSL provider
......................................................................
Patch Set 6: (10 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetProviderCertificateChainQuery.java
Line 19:
Line 20: @Override
Line 21: protected void executeQueryCommand() {
Line 22: Provider hostProvider = getProvider();
Line 23: HostProviderProxy proxy = ((HostProviderProxy)
ProviderProxyFactory.getInstance().create(hostProvider));
Why do you cast to HostProviderProxy?
Line 24:
getQueryReturnValue().setReturnValue(chainToString(proxy.getCertificateChain()));
Line 25: }
Line 26:
Line 27: private String chainToString(List<? extends Certificate> chain) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/BaseProviderProxy.java
Line 41: @Override
Line 42: public List<? extends Certificate> getCertificateChain() {
Line 43: if (!isSecured()) {
Line 44: return null;
Line 45: } else {
You can remove the else since you already returned a value.
Line 46: HttpsURLConnection conn = null;
Line 47: final List<X509Certificate> chain = new
ArrayList<X509Certificate>();
Line 48: try {
Line 49: SSLContext ctx;
Line 49: SSLContext ctx;
Line 50: ctx = SSLContext.getInstance(SSL_PROTOCOL);
Line 51: ctx.init(
Line 52: null,
Line 53: new TrustManager[]{
Can we extract this to a method so that the current method will be less bloated?
Line 54: new X509TrustManager() {
Line 55: public X509Certificate[]
getAcceptedIssuers() {
Line 56: return new X509Certificate[] {};
Line 57: }
Line 95: }
Line 96: return null;
Line 97: }
Line 98:
Line 99: protected static void handleException(VdcBllErrors error,
Exception e) {
Since you're always sending VdcBllErrors.PROVIDER_FAILURE I don't see why you
need it as parameter at this point.
If someone would need this in the future, he can very easily refactor the code,
but for now it makes it less readable IMHO
Line 100: throw new VdcBLLException(error, e.getMessage());
Line 101: }
Line 102:
Line 103: protected boolean isSecured() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ImportProviderCertificateChainCommand.java
Line 26: import org.ovirt.engine.core.common.errors.VdcBllErrors;
Line 27: import org.ovirt.engine.core.compat.Guid;
Line 28: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 29:
Line 30: public class ImportProviderCertificateChainCommand<P extends
ProviderParameters> extends CommandBase<P> {
Can you please add javadoc that explains what this command does and when should
it be used?
Line 31:
Line 32: public ImportProviderCertificateChainCommand(Guid commandId) {
Line 33: super(commandId);
Line 34: }
Line 47:
Line 48: @Override
Line 49: protected boolean canDoAction() {
Line 50: ProviderValidator validator = new
ProviderValidator(getProvider());
Line 51: return validate(validator.providerIsSet());
If it's not set this would return ACTION_TYPE_FAILED_PROVIDER_DOESNT_EXIST
which I think translates to something of the same lines..
Perhaps it's better to have a @NotNull on the field in the ProviderParameters
Line 52: }
Line 53:
Line 54: @Override
Line 55: protected void executeCommand() {
Line 52: }
Line 53:
Line 54: @Override
Line 55: protected void executeCommand() {
Line 56: Provider hostProvider = getProvider();
This probably shouldn't be named hostProvider
Line 57: HostProviderProxy proxy = ((HostProviderProxy)
ProviderProxyFactory.getInstance().create(hostProvider));
Line 58: List<? extends Certificate> chain =
proxy.getCertificateChain();
Line 59: saveChainToTrustStore(chain);
Line 60: }
Line 53:
Line 54: @Override
Line 55: protected void executeCommand() {
Line 56: Provider hostProvider = getProvider();
Line 57: HostProviderProxy proxy = ((HostProviderProxy)
ProviderProxyFactory.getInstance().create(hostProvider));
Why are you casting to HostProviderProxy?
Line 58: List<? extends Certificate> chain =
proxy.getCertificateChain();
Line 59: saveChainToTrustStore(chain);
Line 60: }
Line 61:
Line 124: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__IMPORT);
Line 125:
addCanDoActionMessage(VdcBllMessages.VAR__TYPE__PROVIDER_CERTIFICATE_CHAIN);
Line 126: }
Line 127:
Line 128: private void handleException(VdcBllErrors error, Exception e) {
Since you're always sending
VdcBllErrors.PROVIDER_IMPORT_CERTIFICATE_CHAIN_ERROR I don't see why you need
it as parameter at this point.
If someone would need this in the future, he can very easily refactor the code,
but for now it makes it less readable IMHO
Line 129: throw new VdcBLLException(error, e.getMessage());
Line 130: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetProviderCertificateChainParameters.java
Line 3: import javax.validation.Valid;
Line 4:
Line 5: import org.ovirt.engine.core.common.businessentities.Provider;
Line 6:
Line 7: public class GetProviderCertificateChainParameters extends
VdcQueryParametersBase {
Can you please rename to ProviderQueryParameters so that this class can be used
in other queries as well?
Line 8:
Line 9: private static final long serialVersionUID = 308877238121233739L;
Line 10:
Line 11: @Valid
--
To view, visit http://gerrit.ovirt.org/15128
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I35343409d74a4f90aae726b46781f27ce08a981a
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
