Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KNOX-2186 - Advanced service discovery configuration handling #238

Merged
merged 2 commits into from Jan 17, 2020

Conversation

smolnar82
Copy link
Contributor

What changes were proposed in this pull request?

Implemented CM specific advanced service discovery feature as described in KNOX-2186.

How was this patch tested?

Updated and ran JUnit tests:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 18:37 min (Wall Clock)
[INFO] Finished at: 2020-01-16T11:47:14+01:00
[INFO] Final Memory: 421M/2310M
[INFO] ------------------------------------------------------------------------

Additionally, I tested new features manually. I've used the following two files:
/Users/smolnar/test/knoxGateway/conf/descriptors/testDescriptor.cm

<configuration>
  <property>
    <name>topology1</name>
    <value>
        discoveryType=ClouderaManager;
        discoveryAddress=http://host:123;
        discoveryUser=user;
        discoveryPasswordAlias=alias;
        cluster=Cluster 1;
        providerConfigRef=topology1-provider;
        app:knoxauth:param1.name=param1.value;
        HIVE:url=http://localhost:456;
        HIVE:version=1.0;
        HIVE:httpclient.connectionTimeout=5m;
        HIVE:httpclient.socketTimeout=100m;
        MY_CUSTOM_SERVICE:url=https://custom_host:custom_port
    </value>
  </property>
  <property>
    <name>topology2</name>
    <value>
        providerConfigRef=topology2-provider;
        ATLAS_API:url=http://localhost:456;
        ATLAS_API:httpclient.connectionTimeout=15m;
        ATLAS_API:httpclient.socketTimeout=100m;
        RANGERUI:url=http://localhost:789;
        RANGER:url=http://localhost:rangerport
    </value>
  </property>
</configuration>

/Users/smolnar/test/knoxGateway/conf/auto-discovery-advanced-configuration.properties

gateway.auto.discovery.enforce.required.services=true
gateway.auto.discovery.enabled.ATLAS_API=true
gateway.auto.discovery.enabled.HIVE=false
gateway.auto.discovery.enabled.RANGERUI=true
gateway.auto.discovery.enabled.RANGER=false

During my test rounds, I modified the content of both files, combining them to cover all possible use-cases (e.g. enforcing required services, disabled enforcement, disabled/enabled services, used custom services, etc...). In all cases, I confirmed that CM specific monitors triggered the necessary actions and the expected descriptors were generated.

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this one PR is trying to do too much. Especially around service dependencies.

I thought the idea of the Hadoop configuration XML was to avoid having to have another file to monitor. Why is there a need for auto-discovery-advanced-configuration.properties? Wouldn't the XML file be updated anytime there is a change?


import java.util.Set;

public enum SupportedService {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't quite understand the purpose of this. I left a comment on the Jira about how dependencies between services isn't specific to CM. I don't know why certain services would be listed here. This seems brittle to have to keep up to date.

@smolnar82 smolnar82 force-pushed the KNOX-2186 branch 2 times, most recently from 4574f04 to 29cd2d4 Compare January 16, 2020 20:32
@smolnar82 smolnar82 force-pushed the KNOX-2186 branch 4 times, most recently from 14adfba to f04f731 Compare January 16, 2020 23:45
Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1,36 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is AdvancedServiceDiscoveryConfig, then this is named AdvanceServiceDiscoveryConfigurationMessages.
Not a big deal at all, but they differ ("Advanced" vs "Advance").

@smolnar82 smolnar82 merged commit 69b08af into apache:master Jan 17, 2020
@smolnar82 smolnar82 deleted the KNOX-2186 branch January 17, 2020 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants