[GitHub] geode pull request #664: Fix for GEODE-3292
GitHub user jujoramos opened a pull request: https://github.com/apache/geode/pull/664 Fix for GEODE-3292 Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost - ManagementAgent sets the 'pulse.host' system property whenever a 'jmx-manager-bind-address' is configured. - PULSE gets the hostname of the local JMX Manager through the 'pulse.host' system property, reverting to 'localhost' by default. Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jujoramos/geode feature/GEODE-3292 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/664.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #664 commit 0c3e15f619ddd08f02ba8e2a8818f22d308c8bde Author: Juan Jose Ramos Cassella Date: 2017-07-28T09:55:20Z GEODE-3292: Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost - ManagementAgent sets the 'pulse.host' system property whenever a 'jmx-manager-bind-address' is configured. - PULSE gets the hostname of the local JMX Manager through the 'pulse.host' system property, reverting to 'localhost' by default. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #664: Fix for GEODE-3292
Github user jujoramos commented on the issue: https://github.com/apache/geode/pull/664 Hey @jaredjstewart, Thanks for looking at this. I'll write the tests, no worries at all. Cheers. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #664: Fix for GEODE-3292
Github user jujoramos commented on the issue: https://github.com/apache/geode/pull/664 Hello @jaredjstewart, Just pushed the requested changes. - Renamed `PulseVerificationTest` to `PulseConnectivityTest`. - Removed `loginWithIncorrectPassword` test method, it is the same as the one in `PulseSecurityTest`. - Added integration tests to check the connectivity between PULSE and the local jmx-manager, both when the default and non-default bind-address is used. - Added `PulseAppListenerTest` unit test to verify that the repository is correctly configured by the `PulseAppListener` when PULSE is running in embedded mode. Cheers. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
Github user jujoramos commented on a diff in the pull request: https://github.com/apache/geode/pull/664#discussion_r131071353 --- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java --- @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.tools.pulse; + +import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS; +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.InetAddress; +import java.util.Properties; + +import org.apache.geode.test.dunit.rules.EmbeddedPulseRule; +import org.apache.geode.test.dunit.rules.LocatorStarterRule; +import org.apache.geode.test.junit.categories.IntegrationTest; +import org.apache.geode.tools.pulse.internal.data.Cluster; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.experimental.runners.Enclosed; +import org.junit.runner.RunWith; + +@RunWith(Enclosed.class) --- End diff -- Yup, you're right, I didn't see the security manager within the rule and just looked at the method implementation. The login test doesn't seem to be functionality related to the connectivity test, that's why I removed it completely and aggregated the tests in a single class with two inner classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
Github user jujoramos commented on a diff in the pull request: https://github.com/apache/geode/pull/664#discussion_r131071365 --- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java --- @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.tools.pulse; + +import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS; +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.InetAddress; +import java.util.Properties; + +import org.apache.geode.test.dunit.rules.EmbeddedPulseRule; +import org.apache.geode.test.dunit.rules.LocatorStarterRule; +import org.apache.geode.test.junit.categories.IntegrationTest; +import org.apache.geode.tools.pulse.internal.data.Cluster; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.experimental.runners.Enclosed; +import org.junit.runner.RunWith; + +@RunWith(Enclosed.class) +@Category(IntegrationTest.class) +public class PulseConnectivityTest { + @ClassRule + public static LocatorStarterRule locator = new LocatorStarterRule().withJMXManager(); + + // Test Connectivity for Default Configuration + @Category(IntegrationTest.class) + public static class DefaultBindAddressTest { +@Rule +public EmbeddedPulseRule pulse = new EmbeddedPulseRule(); + +@BeforeClass +public static void beforeClass() throws Exception { --- End diff -- True, but I'm not using `withAutoStart()` for a good reason: the two inner classes need to setup the locator differently (the `jmx-manager-bind-address` value changes). I thought it's easier to read a single functional test class annotated with `@RunWith(Enclosed.class)` internally split in different inner classes than write a new test class to test exactly the same but changing a single locator property. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
Github user jujoramos commented on a diff in the pull request: https://github.com/apache/geode/pull/664#discussion_r131071385 --- Diff: geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java --- @@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) { } System.setProperty(PULSE_EMBEDDED_PROP, "true"); + System.setProperty(PULSE_HOST_PROP, "" + config.getJmxManagerBindAddress()); --- End diff -- I'll rewrite the tests and push the changes again, thanks for the suggestions!. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
Github user jujoramos commented on a diff in the pull request: https://github.com/apache/geode/pull/664#discussion_r131367740 --- Diff: geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java --- @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + * + */ + +package org.apache.geode.tools.pulse.internal; + +import javax.servlet.ServletContextEvent; +import static org.mockito.Mockito.*; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestRule; + +import org.apache.geode.test.junit.categories.UnitTest; +import org.apache.geode.tools.pulse.internal.data.PulseConstants; +import org.apache.geode.tools.pulse.internal.data.Repository; + +@Category(UnitTest.class) +public class PulseAppListenerTest { + private Repository repository; + private PulseAppListener appListener; + + @Rule + public final TestRule restoreSystemProperties = new RestoreSystemProperties(); + + @Before + public void setUp() { +repository = Repository.get(); +appListener = new PulseAppListener(); + } + + @Test + public void embeddedModeDefaultPropertiesRepositoryInitializationTest() { +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true"); +appListener.contextInitialized(mock(ServletContextEvent.class)); + +Assert.assertEquals(false, repository.getJmxUseLocator()); +Assert.assertEquals(false, repository.isUseSSLManager()); +Assert.assertEquals(false, repository.isUseSSLLocator()); +Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, repository.getPort()); +Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, repository.getHost()); + + } + + @Test + public void embeddedModeNonDefaultPropertiesRepositoryInitializationTest() { +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, "true"); +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, ""); +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, "nonDefaultBindAddress"); +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER, +Boolean.TRUE.toString()); +System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR, +Boolean.TRUE.toString()); + +appListener.contextInitialized(mock(ServletContextEvent.class)); + +Assert.assertEquals(false, repository.getJmxUseLocator()); +Assert.assertEquals(true, repository.isUseSSLManager()); +Assert.assertEquals(true, repository.isUseSSLLocator()); +Assert.assertEquals("", repository.getPort()); +Assert.assertEquals("nonDefaultBindAddress", repository.getHost()); + } + + @After + public void tearDown() { +if (repository != null) { --- End diff -- Yes, I thought about using the existing rule but it implies adding a dependency from `geode-web` to `geode-assembly` as the rule is defined there, or duplicating the rule source code, that's why I didn't do it. Which approach would be best?. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode pull request #664: Fix for GEODE-3292
Github user jujoramos commented on a diff in the pull request: https://github.com/apache/geode/pull/664#discussion_r131378087 --- Diff: geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java --- @@ -15,26 +15,31 @@ package org.apache.geode.tools.pulse; +import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS; import static org.assertj.core.api.Assertions.assertThat; -import org.apache.geode.test.dunit.rules.EmbeddedPulseRule; -import org.apache.geode.test.dunit.rules.HttpClientRule; -import org.apache.geode.test.dunit.rules.LocatorStarterRule; -import org.apache.geode.test.junit.categories.IntegrationTest; -import org.apache.geode.tools.pulse.internal.data.Cluster; +import java.io.File; +import java.net.InetAddress; +import java.util.Properties; + import org.apache.http.HttpResponse; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.junit.BeforeClass; +import org.apache.geode.test.dunit.rules.EmbeddedPulseRule; +import org.apache.geode.test.dunit.rules.HttpClientRule; +import org.apache.geode.test.dunit.rules.LocatorStarterRule; +import org.apache.geode.test.junit.categories.IntegrationTest; +import org.apache.geode.tools.pulse.internal.data.Cluster; -@Category(IntegrationTest.class) -public class PulseVerificationTest { +public abstract class AbstractPulseConnectivityTest { --- End diff -- Thanks for the tips, I couldn't find anything related to this intention within [Writing Tests](https://cwiki.apache.org/confluence/display/GEODE/Writing+tests) but I'm okay anyway. I'll give it one more try in my next commit later today, change things as you please :-). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #664: Fix for GEODE-3292
Github user jujoramos commented on the issue: https://github.com/apache/geode/pull/664 @jinmeiliao: thanks for reviewing this :-). One more quick question and I'll leave you alone: do I need to change the status of [GEODE-3292](https://issues.apache.org/jira/browse/GEODE-3292) to `RESOLVED`?. Or that's done when the pull request is merged into `develop`?. Couldn't find a clear answer from [Code Contributions](https://cwiki.apache.org/confluence/display/GEODE/Code+contributions) and [Developer Workflow](https://cwiki.apache.org/confluence/display/GEODE/Developer+Workflow). Cheers. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---