CLOUDSTACK-10103: Cloudian Connector for CloudStack #2284

rohityadavcloud · 2017-10-04T07:41:21Z

Several organizations use Cloudian as S3 provider, this implements the
Cloudian Management Console connector for CloudStack that can do the
following:

Provide ease in connector configuration using CloudStack global
settings
Perform SSO from CloudStack UI into Cloudian Management Console (CMC)
when the connector is enabled
Automatic provisioning and de-provisioning of CloudStack accounts and
domains as Cloudian users and groups respectively
During CloudStack UI logout, logout user from CMC
CloudStack account will be mapped to Cloudian Users, and CloudStack
domain will be mapped to Cloudian Groups.
The CloudStack admin account is mapped to Cloudian admin (user name
configurable).
The user/group provisioning will be from CloudStack to Cloudian only,
i.e. user/group addition/removal/updation/deactivation in Cloudian
portal (CMC) won't propagate the changes to CloudStack.

FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Cloudian+Connector+for+CloudStack

New APIs:

cloudianIsEnabled: API to check whether Cloudian Connector is enabled.
cloudianSsoLogin: Performs SSO for the logged-in, requesting user
and returns the URL that can be used to perform
SSO and log into CMC.

New Global Settings:

cloudian.connector.enabled (false)
If set to true, this enables the Cloudian Connector for CloudStack.
Restarting management server(s) is required.
cloudian.admin.host (s3-admin.cloudian.com)
The host where Cloudian Admin services are accessible.
cloudian.admin.port (19443)
The admin service port.
cloudian.admin.protocol (https)
The admin service API scheme/protocol.
cloudian.validate.ssl (true)
When set to true, this validates the certificate of the https-enabled
admin API service.
cloudian.admin.user (sysadmin)
The admin user's name when making (admin) API calls.
cloudian.admin.password (public)
The admin password used when making (admin) API calls.
cloudian.api.request.timeout (5)
The API request timeout in seconds used by the internal HTTP/s client.
cloudian.cmc.admin.user (admin)
The CMC admin user's name.
cloudian.cmc.host (cmc.cloudian.com)
The CMC host.
cloudian.cmc.port (8443)
The CMC service port.
cloudian.cmc.protocol (https)
The CMC service scheme/protocol.
cloudian.sso.key (ss0sh5r3dk3y)
The Single-Sign-On shared key.

rohityadavcloud · 2017-10-04T10:27:48Z

Code review requested @nvazquez @DaanHoogland @borisstoyanov /cc @PaulAngus @dagsonstebo

borisstoyanov · 2017-10-04T10:37:41Z

@blueorangutan package

blueorangutan · 2017-10-04T10:38:33Z

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-10-04T11:05:08Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1131

rohityadavcloud · 2017-10-04T11:24:14Z

@blueorangutan test

blueorangutan · 2017-10-04T11:24:33Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2017-10-04T21:41:50Z

Trillian test result (tid-1559)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37036 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2284-t1559-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 59 look OK, 3 have error(s)

Test	Result	Time (s)	Test File
test_01_vpc_remote_access_vpn	`Failure`	55.88	test_vpc_vpn.py
test_04_rvpc_privategw_static_routes	`Failure`	375.59	test_privategw_acl.py
test_03_delete_template	`Error`	5.08	test_templates.py
test_change_service_offering_for_vm_with_snapshots	Skipped	0.00	test_vm_snapshots.py
test_09_copy_delete_template	Skipped	0.01	test_templates.py
test_06_copy_template	Skipped	0.00	test_templates.py
test_static_role_account_acls	Skipped	0.03	test_staticroles.py
test_11_ss_nfs_version_on_ssvm	Skipped	0.02	test_ssvm.py
test_01_scale_vm	Skipped	0.00	test_scale_vm.py
test_01_primary_storage_iscsi	Skipped	0.08	test_primary_storage.py
test_vm_nic_adapter_vmxnet3	Skipped	0.00	test_nic_adapter_type.py
test_nested_virtualization_vmware	Skipped	0.00	test_nested_virtualization.py
test_06_copy_iso	Skipped	0.00	test_iso.py
test_list_ha_for_host_valid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host_invalid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host	Skipped	0.02	test_hostha_simulator.py
test_hostha_enable_feature_without_setting_provider	Skipped	0.02	test_hostha_simulator.py
test_hostha_enable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_disable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_invalid_provider	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_default_driver	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_recovering	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_fenced	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_degraded	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_available	Skipped	0.02	test_hostha_simulator.py
test_ha_multiple_mgmt_server_ownership	Skipped	0.02	test_hostha_simulator.py
test_ha_list_providers	Skipped	0.02	test_hostha_simulator.py
test_ha_enable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_disable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_configure_enabledisable_across_clusterzones	Skipped	0.02	test_hostha_simulator.py
test_configure_ha_provider_valid	Skipped	0.02	test_hostha_simulator.py
test_configure_ha_provider_invalid	Skipped	0.02	test_hostha_simulator.py
test_deploy_vgpu_enabled_vm	Skipped	0.04	test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support	Skipped	0.05	test_deploy_vgpu_enabled_vm.py

rohityadavcloud · 2017-10-05T06:26:46Z

Test LGTM, no regressions seen. Errors are known intermittent failures.

nvazquez · 2017-10-05T11:31:53Z

plugins/integrations/cloudian/pom.xml

+    <dependency>
+        <groupId>com.github.tomakehurst</groupId>
+        <artifactId>wiremock</artifactId>
+        <version>2.8.0</version>


Can we store version in a pom property?

Sure, I'll fix this.

nvazquez · 2017-10-05T11:50:19Z

plugins/integrations/cloudian/src/org/apache/cloudstack/cloudian/CloudianConnectorImpl.java

+        final CloudianClient client = getClient();
+        if (existingUser != null) {
+            final User accountUser = userDao.listByAccount(account.getId()).get(0);
+            final String fullName = String.format("%s %s (%s)", accountUser.getFirstname(), accountUser.getLastname(), account.getAccountName());


Can we delegate full name creation to a new method which takes an User as parameter to avoid code duplicates on full name creation?

Good idea, however, we're using the fullName prior to comparison and we're only updating if there is any mismatch is fields, so we need to keep it here.

nvazquez · 2017-10-05T12:12:19Z

plugins/integrations/cloudian/src/org/apache/cloudstack/cloudian/client/CloudianClient.java

+
+    public List<CloudianUser> listUsers(final String groupId) {
+        if (Strings.isNullOrEmpty(groupId)) {
+            return new ArrayList<>();


Any particular reason for returning an empty list instead of null for this method?

Yes, we would want to avoid NPEs, which is why we're returning empty arrays.

nvazquez

@rhtyd great work! I have left some few comments, other than that, code LGTM

rohityadavcloud · 2017-10-06T10:39:29Z

Community docs submitted: apache/cloudstack-docs#21

rohityadavcloud · 2017-10-06T10:45:29Z

Thanks @nvazquez I've addressed the issues you've mentioned now.

rohityadavcloud · 2017-10-06T11:03:21Z

Pinging for review - @DaanHoogland @karuturi @wido @GabrielBrascher @rafaelweingartner @ustcweizhou and others

DaanHoogland · 2017-10-09T12:47:15Z

Code LGTM. To bad we cannot have integration tests. Let's hope Cloudian will support it (with infra ;)

rafaelweingartner

@rhtyd I have seen some points that might benefit from improvements, what do you think?

rafaelweingartner · 2017-10-10T13:54:42Z

plugins/integrations/cloudian/src/org/apache/cloudstack/cloudian/client/CloudianClient.java

+    //////////////// Public APIs: Group /////////////////////
+    /////////////////////////////////////////////////////////
+
+    public boolean addGroup(final CloudianGroup group) {


What about throwing a runtime exception (a custom one) if it is not possible to add this group object?

It feels more natural for a method to throw an exception if it fails than to return a Boolean indicating its status.

The same applies for the remove, list (instead of returning null?) and update methods

Runtime exceptions are suitably thrown by checkResponseTimeOut and other methods. The boolean value may be used to ascertain whether the operation was successful or not. Throwing and catching exceptions will ultimately be similar pattern.

I agree that the result of a code that throws an exception and then catching it is the same.

I normally like the idea of reading the signature of the method and understanding what it does, that is why I care so much about naming and documentation stuff sometimes. Having said that, I like to divide methods into two groups, the ones that do something and the ones that return something; this makes things a little more clear when coding, and normally help me divide better the tasks I am coding. So, when I read methods such as save, remove, update and so on, they are in the imperative form, ordering “the system” to do something; those method signatures do not give me a hint regarding the result of the operation, and as such I would expect an exception if things go south. On the other hand, when I see methods get, retrieve, count and so on, they are asking for something, so I expect something to be returned.

I see the use of this style having methods that do something and return the state of the operation in C programs. The difference is that in C people normally use numbers as status results of methods, and then we have a broader range of different possible status to be informed to the piece of code that is using the method.

This point that I am touching relates to semantics (conventions). I have seen this standard in some part of our code base and I do not find them very intuitive (maybe a Javadoc would help?).

For instance, the addGroup method, it will return false if the group is null; it also returns false when response.getStatusLine().getStatusCode()is not the expected one; moreover, it returns false when the methodcheckResponseTimeOutdoes not re-throws an exception (when this solves to false(e instanceof ConnectTimeoutException || e instanceof SocketTimeoutException)`. This means that I cannot trust the result of the method (the true or false to indicate if everything is working there) if I want to properly deal errors and exceptions.

It is not that I am against the code the way it is; I only wanted to call attention to a point that can help us to reduce the barrier for newcomers. The more readable and understandable a code is at first sight, the easier it is for newcomers to start working with it.

P.S. sorry for the long text, I also believe the simple and small java docs can help making the methods easier to understand without needing to read code

@rafaelweingartner I disagree with several of your remarks, there is no guidelines or rule that would require everyone to throw an exception. Exception would mean something happened out of the ordinary against set rules/contracts, and per Cloudian API docs some APIs may return 204 (no content) to return a resource that does not exist or failed. A timeout is a valid exception, however failing to add a group or a user is expected (for example, the group already existed etc). The implementation of the Cloudian API client is driven by its uses per the feature, and currently, we only need to know whether a user/group is not synced and should be added.

The client implementation is not for public consumption as a client/library but restricted to the plugin to limit its scope, pragmatic implementation, and use. For general use, there exists Cloudian java sdks that can be used, however, including them with CloudStack release had licensing and other issues which is why a minimal client was implemented to cater to the limited feature scope.

I agree that there is no guideline that is why I touched this matter. My point regarding exceptions was that, if I read the method name when using it, I have no idea why it returns a boolean, I will have to check the implementation, which I do not find a good way of working, if we force everybody that is doing something in our code base to always check every single method they use because of a lack of documentation of lack of clarity in method naming.

Also, if the method returns false for instance, we just know that something is not successful, but we do not know what (group already there, some other exception, and so on).

As I said, I am not against it. I understand that these are personal ways someone designs a solution, and that is fair. I only wanted to show these points and try to understand why we are going towards one direction and not the other.

rafaelweingartner · 2017-10-10T13:57:58Z

plugins/integrations/cloudian/src/org/apache/cloudstack/cloudian/client/CloudianClient.java

+        try {
+            final HttpResponse response = get(String.format("/group?groupId=%s", groupId));
+            checkResponseOK(response);
+            if (response.getStatusLine().getStatusCode() == HttpStatus.SC_NO_CONTENT ||


what about putting the boolean condition response.getStatusLine().getStatusCode() == HttpStatus.SC_NO_CONTENT || response.getEntity() == null || response.getEntity().getContent() == null in a method? it repeats quite often. Then, it is even possible to write a unit test for it.

Okay, I'll refactor it.

Several organizations use Cloudian as S3 provider, this implements the Cloudian Management Console connector for CloudStack that can do the following: - Provide ease in connector configuration using CloudStack global settings - Perform SSO from CloudStack UI into Cloudian Management Console (CMC) when the connector is enabled - Automatic provisioning and de-provisioning of CloudStack accounts and domains as Cloudian users and groups respectively - During CloudStack UI logout, logout user from CMC - CloudStack account will be mapped to Cloudian Users, and CloudStack domain will be mapped to Cloudian Groups. - The CloudStack admin account is mapped to Cloudian admin (user name configurable). - The user/group provisioning will be from CloudStack to Cloudian only, i.e. user/group addition/removal/updation/deactivation in Cloudian portal (CMC) won't propagate the changes to CloudStack. FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Cloudian+Connector+for+CloudStack New APIs: - `cloudianIsEnabled`: API to check whether Cloudian Connector is enabled. - `cloudianSsoLogin`: Performs SSO for the logged-in, requesting user and returns the URL that can be used to perform SSO and log into CMC. New Global Settings: - cloudian.connector.enabled (false) If set to true, this enables the Cloudian Connector for CloudStack. Restarting management server(s) is required. - cloudian.admin.host (s3-admin.cloudian.com) The host where Cloudian Admin services are accessible. - cloudian.admin.port (19443) The admin service port. - cloudian.admin.protocol (https) The admin service API scheme/protocol. - cloudian.validate.ssl (true) When set to true, this validates the certificate of the https-enabled admin API service. - cloudian.admin.user (sysadmin) The admin user's name when making (admin) API calls. - cloudian.admin.password (public) The admin password used when making (admin) API calls. - cloudian.api.request.timeout (5) The API request timeout in seconds used by the internal HTTP/s client. - cloudian.cmc.admin.user (admin) The CMC admin user's name. - cloudian.cmc.host (cmc.cloudian.com) The CMC host. - cloudian.cmc.port (8443) The CMC service port. - cloudian.cmc.protocol (https) The CMC service scheme/protocol. - cloudian.sso.key (ss0sh5r3dk3y) The Single-Sign-On shared key. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

rohityadavcloud · 2017-10-12T05:55:31Z

plugins/integrations/cloudian/src/org/apache/cloudstack/cloudian/CloudianConnector.java

+     * Generates single-sign on URL for logged in user
+     * @return returns the SSO URL string
+     */
+    String generateSsoUrl();


@rafaelweingartner FYI, we've java docs for this interface that we may be consumed by other parts of the codebase. The Cloudian client does not have them because we don't have it to be generally used by other parts of the codebase.

Ah, I did not see if there was an interface, my bad...

rohityadavcloud · 2017-10-25T05:19:16Z

No regressions are seen, no outstanding issues.
With enough test and code LGTMs, I'll merge this. Thanks.

nitin-maharana · 2017-10-25T05:27:29Z

ui/plugins/cloudian/cloudian.css

@@ -0,0 +1,18 @@
+/*


@rhtyd, As there is no content in this file, Can we remove this or do you use it for any other purpose?

For sake of completion, all plugins have this file and was put for the cloudian UI plugin. require.js tries to GET this file, when this is missing it causes a HTTP 404, however, we'll need to test if it causes any rendering/load issue in the UI. You may remove this after performing this test.

rohityadavcloud force-pushed the cloudian-connector-master branch from 95e7fad to a075c6d Compare October 4, 2017 07:43

rohityadavcloud changed the title ~~[WIP] CLOUDSTACK-10103: Cloudian Connector for CloudStack~~ CLOUDSTACK-10103: Cloudian Connector for CloudStack Oct 5, 2017

nvazquez reviewed Oct 5, 2017

View reviewed changes

rohityadavcloud mentioned this pull request Oct 6, 2017

CLOUDSTACK-10103: Documentation for Cloudian Connector apache/cloudstack-docs#21

Merged

rohityadavcloud force-pushed the cloudian-connector-master branch from a075c6d to 029eaaf Compare October 6, 2017 10:45

rafaelweingartner reviewed Oct 10, 2017

View reviewed changes

rohityadavcloud force-pushed the cloudian-connector-master branch from 029eaaf to b618739 Compare October 11, 2017 12:35

rohityadavcloud commented Oct 12, 2017

View reviewed changes

rohityadavcloud merged commit b6dc40f into apache:master Oct 25, 2017

nitin-maharana reviewed Oct 25, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDSTACK-10103: Cloudian Connector for CloudStack #2284

CLOUDSTACK-10103: Cloudian Connector for CloudStack #2284

rohityadavcloud commented Oct 4, 2017

rohityadavcloud commented Oct 4, 2017

borisstoyanov commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

rohityadavcloud commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

rohityadavcloud commented Oct 5, 2017

nvazquez Oct 5, 2017

rohityadavcloud Oct 6, 2017

nvazquez Oct 5, 2017 •

edited

Loading

rohityadavcloud Oct 6, 2017

nvazquez Oct 5, 2017

rohityadavcloud Oct 6, 2017

nvazquez left a comment

rohityadavcloud commented Oct 6, 2017

rohityadavcloud commented Oct 6, 2017

rohityadavcloud commented Oct 6, 2017

DaanHoogland commented Oct 9, 2017

rafaelweingartner left a comment

rafaelweingartner Oct 10, 2017

rohityadavcloud Oct 11, 2017

rafaelweingartner Oct 11, 2017 •

edited

Loading

rohityadavcloud Oct 12, 2017

rafaelweingartner Oct 12, 2017

rafaelweingartner Oct 10, 2017

rohityadavcloud Oct 11, 2017

rohityadavcloud Oct 12, 2017

rafaelweingartner Oct 12, 2017

rohityadavcloud commented Oct 25, 2017

nitin-maharana Oct 25, 2017

rohityadavcloud Oct 25, 2017

CLOUDSTACK-10103: Cloudian Connector for CloudStack #2284

CLOUDSTACK-10103: Cloudian Connector for CloudStack #2284

Conversation

rohityadavcloud commented Oct 4, 2017

rohityadavcloud commented Oct 4, 2017

borisstoyanov commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

rohityadavcloud commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

blueorangutan commented Oct 4, 2017

rohityadavcloud commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvazquez Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvazquez left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Oct 6, 2017

rohityadavcloud commented Oct 6, 2017

rohityadavcloud commented Oct 6, 2017

DaanHoogland commented Oct 9, 2017

rafaelweingartner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelweingartner Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohityadavcloud commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvazquez Oct 5, 2017 •

edited

Loading

rafaelweingartner Oct 11, 2017 •

edited

Loading