CDAP-15588: Prefer internal ip when vpc peering exists #11457

rohitsinha54 · 2019-06-27T22:30:05Z

…t network prefer internal ip

anwar6953 · 2019-06-27T22:33:51Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+        String peeredNetwork = matcher.group(4);
+        if (peeredProjectID.equals(systemProjectId) && peeredNetwork.equals(systemNetwork)) {
+          if (peering.getState().equals("ACTIVE")) {
+            // peering to tenant project found prefer internal unless prefer external IP is set


remove comment about "tenant project"

also remove it from the error message and the PR title?

anew

Overall looks good, but internal language such as "tenant project" should not be here. I see it twice.

albertshau · 2019-06-28T16:46:43Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

@@ -122,6 +133,36 @@ public static DataprocClient fromConf(DataprocConf conf) throws IOException, Gen
    String subnet = conf.getSubnet();
    Network networkInfo = getNetworkInfo(projectId, network, compute);

+    // if a peered network is specified then the instance is tied to a network
+    if (!Strings.isNullOrEmpty(conf.getPeeredNetwork())) {


I thought we talked about just checking if the configured network is peered with the CDAP network and using the internal IP if so. Are these two new properties (private instance and peered network)actually needed?

albertshau · 2019-06-28T16:47:59Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

  // something like 2018-04-16T12:09:03.943-07:00
  private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd'T'hh:mm:ss.SSSX");

+  private static final Pattern GCP_NETWORK_FULL_PATH =
+    Pattern.compile("(https://www.googleapis.com/compute/v1/projects/)(.*)?(/global/networks/)(.*)?");


is 'global' here a location? Is it correct to hardcode it?

Since its only for comparing with the system network, I think its ok to have hardcode in it as we always create the system network with the global location. Also in future we may run the cdap services with customer supplied service accounts in which case there is no guarantee that the service account will have access on the system network. Ideally we should pass the system configs to the provisioner so we dont have to have logic of figuring out system network or system project in the Dataproc provisioner itself right?

albertshau · 2019-06-28T16:50:08Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+                                  boolean raiseException) {
+    List<NetworkPeering> peerings = networkInfo.getPeerings();
+    for (NetworkPeering peering : peerings) {
+      Matcher matcher = GCP_NETWORK_FULL_PATH.matcher(peering.getNetwork());


It seems safer to get the system network URI and check if it is equal to the peering network rather than parsing this string. Can this be gotten from the Network object corresponding to the system network?

rohitsinha54 · 2019-07-02T18:59:25Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+      throw new IllegalStateException(String.format("VPC Peering to the system network %s in system project %s found " +
+                                                      "in INACTIVE state. Please fix peering setup to be in ACTIVE " +
+                                                      "state.", systemNetwork, systemProjectId));
+    } else {


In case of private instance which is tied to a network and can only use private ip if the customer has not setup the peering on their end then the peeringState will return NONE in that case internalIP will be preferred unless someone set externalIP to true but the run will fail since there is no peering from customer side.

I believe we will have to ensure that the peering does exist when instance is private.

Please open a bug for this .

rohitsinha54 · 2019-07-04T00:43:44Z

...ataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocProvisioner.java

@@ -59,6 +60,8 @@

  // Keys for looking up system properties
  private static final String LABELS_PROPERTY = "labels";
+  // Name of the network on customer project where the Dataproc cluster is to be created
+  private static final String NETWORK = "network";


This is unused.

albertshau · 2019-07-08T17:43:37Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+    PeeringState state = peeringState(systemNetwork, systemProjectId, networkInfo);
+    if (state == PeeringState.ACTIVE) {
+      if (conf.isPreferExternalIP()) {
+        LOG.warn("Prefer external IP setting will be ignored and internal IP will used to communicate with the " +


We should not be making an assumption that the user has misconfigured this property. If a user is explicitly telling the provisioner to use the external IP, it should always use the external IP, regardless of how the network is setup. We can log an INFO message that the network is peered and suggest that the internal IP be used, but we should not override something they have explicitly set.

albertshau · 2019-07-08T17:49:37Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

@@ -149,6 +178,27 @@ public static DataprocClient fromConf(DataprocConf conf) throws IOException, Gen
    return new DataprocClient(new DataprocConf(conf, network, subnet), client, compute, useInternalIP);
  }

+  private static PeeringState peeringState(String systemNetwork, String systemProjectId, Network networkInfo) {


the system network is nullable. It is null when CDAP is not running on GCP. When null, there is no peering between CDAP and the configured network.

added @Nullable annotation and corresponding check.

albertshau · 2019-07-08T17:51:02Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+
+      String peeredProjectID = matcher.group(2);
+      String peeredNetwork = matcher.group(4);
+      if (peeredProjectID.equals(systemProjectId) && peeredNetwork.equals(systemNetwork)) {


As mentioned previously, we should not be using pattern matching for this. The Network object for the system network can be fetched and used to check equality. The global location should not be hardcoded either, as it should be known from the Network object.

removed the pattern matching code.

albertshau · 2019-07-08T17:52:31Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

@@ -263,6 +313,11 @@ public OperationSnapshot createCluster(String name, String imageVersion, Map<Str
        clusterConfig.addTags(targetTag);
      }

+      // if internal ip is prefered then create dataproc cluster without external ip for better security
+      if (!conf.isPreferExternalIP()) {


this is not correct, it will create internal IP clusters in many instances where CDAP requires the external IP. This should be using the useInternalIP flag.

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

albertshau · 2019-07-08T17:54:24Z

...ataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocProvisioner.java

@@ -18,6 +18,7 @@

 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
+import com.google.common.base.Strings;


accidental addition?

albertshau · 2019-07-08T17:56:48Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+      }
+      useInternalIP = true;
+    } else if (state == PeeringState.INACTIVE) {
+      throw new IllegalStateException(String.format("VPC Peering to the system network %s in system project %s found " +


should include the dataproc network being used.

VPC Peering from network '%s' in project '%s' to network '%s' in project '%s' is in the INACTIVE state. Please fix the peering setup to be in the ACTIVE state.

albertshau · 2019-07-08T17:59:25Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+                                                      "in INACTIVE state. Please fix peering setup to be in ACTIVE " +
+                                                      "state.", systemNetwork, systemProjectId));
+    } else {
+      useInternalIP = !conf.isPreferExternalIP();


this is not correct. This setting is used to tell the provisioner to use the external IP even when it detects that the internal IP can be used. When this is false and the provisioner detects that the internal IP cant be used (when the system network does not exist or is different than the dataproc network and it is not peered), useInternalIP should be false.

done. so conf.isPreferExternalIP() takes precedence.

sagarkapare · 2019-07-08T21:47:12Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+    // Get self link for the network.
+    String systemNetworkSelfLink = systemNetworkInfo.getSelfLink();
+
+    LOG.info(String.format("Self link for the system network is %s", systemNetworkSelfLink));


Added log line for debugging purpose. Will remove before merging.

albertshau · 2019-07-08T23:27:30Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

@@ -122,6 +129,32 @@ public static DataprocClient fromConf(DataprocConf conf) throws IOException, Gen
    String subnet = conf.getSubnet();
    Network networkInfo = getNetworkInfo(projectId, network, compute);

+    boolean useInternalIP = !conf.isPreferExternalIP();


this isn't correct. use internal IP should only be true if:

prefer external IP is false
AND
(system network is the same as the dataproc network) OR (system network is peered with dataproc network)

updated code as discussed.

albertshau · 2019-07-08T23:29:14Z

comment about unused import still exists too

albertshau · 2019-07-08T23:48:01Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+                                                      "(Prefer External IP = false) Dataproc clusters.",
+                                                    systemProjectId, projectId));
+    }
+


boolean useInternalIP = !conf.isPreferExternalIP() && (sytemNetwork.equals(network) || PeeringState.ACTIVE == state)

sagarkapare · 2019-07-09T00:09:54Z

@albertshau please take a look again. thanks!

albertshau

lgtm pending removal of debugging statement.

sagarkapare · 2019-07-09T00:43:50Z

Build is running here - https://builds.cask.co/browse/CDAP-DUT6996-9

rohitsinha54 · 2019-07-09T08:02:23Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+      return PeeringState.NONE;
+    }
+
+    Network systemNetworkInfo = getNetworkInfo(systemProjectId, systemNetwork, compute);


This requires permission on the network which is acceptable but will not be present on existing setups and will require backfill.

@sagarkapare : Thinking more about this permission issue change it has a much wider impact. This requires the management SA to have network permission on the tp network. In Beta it does not which means that for existing customer if we roll out this change the dataproc provisioning will start failing for all instances till the permissions are backfilled.

albertshau · 2019-07-09T16:50:43Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

-      // Peering is setup between the system network and customer network. However it is not in ACTIVE state.
-      // User has also intended to use the internal IP. Dataproc cluster cannot be launched on the internal IP unless
-      // the peering is fixed.
+    } else if (!conf.isPreferExternalIP() && state != PeeringState.ACTIVE) {


this was correct before and incorrect now. "Prefer External IP" just means always use the external ip if it is true. If it is false, let the provisioner decide. A false value does not mean always use the internal ip.

chtyim · 2019-07-10T08:38:07Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+    }
+  }
+
+  private static PeeringState peeringState(@Nullable String systemNetwork, String systemProjectId,


Should be called getPeeringState.

chtyim · 2019-07-10T08:41:40Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+    LOG.info(String.format("Self link for the system network is %s", systemNetworkSelfLink));
+    List<NetworkPeering> peerings = networkInfo.getPeerings();
+    // if the customer does not has a peering established at all the peering list is null
+    if (peerings != null) {


Invert the check and return early. It makes the code cleaner and easier to reason all the states.

if (peerings == null) { return PeeringState.NONE; } for (...) { }

chtyim · 2019-07-10T08:44:20Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+        if (!systemNetworkSelfLink.equals(peering.getNetwork())) {
+          continue;
+        }
+        if (peering.getState().equals("ACTIVE")) {


Use the ? : operator will make the logic easier to read.

return peering.getState().equals("ACTIVE") ? PeeringState.ACTIVE : PeeringState.INACTIVE;

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

chtyim · 2019-07-10T08:47:46Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+      // However user has selected to preferred external IP the instance. This is not a private instance and is
+      // capable of communicating with Dataproc cluster with external ip so just add warning message indicating that
+      // internal IP can be used.
+      LOG.warn(String.format("VPC Peering from network '%s' in project '%s' to network '%s' " +


This is more like an INFO rather than warning.

chtyim · 2019-07-10T08:50:02Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+      // (CDAP is running in the same customer project as Dataproc is going to be launched or
+      // Network peering is done between customer network and system network and is in ACTIVE mode).
+      useInternalIP = !conf.isPreferExternalIP()
+        && ((network.equals(systemNetwork) && projectId.equals(systemProjectId)) || state == PeeringState.ACTIVE);


Is the systemNetwork the GCP network that CDAP itself is using? If that's the case, why it has to be the same as the Dataproc network?

Yes this check is there for EAP and other setup where it runs in customer project.

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

albertshau

lgtm

rohitsinha54 · 2019-07-10T23:12:10Z

Waiting on build: https://builds.cask.co/browse/CDAP-DUT6996-13

rohitsinha54 · 2019-07-11T00:08:06Z

Build passed merging.

chtyim · 2019-07-11T02:46:25Z

...ext-dataproc/src/main/java/io/cdap/cdap/runtime/spi/provisioner/dataproc/DataprocClient.java

+
+    if (privateInstance) {
+      // private instance must have a peering established
+      verifyPeering(conf, network, systemNetwork, projectId, systemProjectId, state);


Please remove the peering check. What we need is connectivity in some form, and is not necessarily achieved via network peering

rohitsinha54 requested a review from albertshau June 27, 2019 22:30

rohitsinha54 assigned albertshau Jun 27, 2019

anwar6953 reviewed Jun 27, 2019

View reviewed changes

rohitsinha54 force-pushed the feature/CDAP-15588-private branch from b468983 to 958f1c2 Compare June 27, 2019 22:50

anew suggested changes Jun 27, 2019

View reviewed changes

rohitsinha54 force-pushed the feature/CDAP-15588-private branch from 958f1c2 to 3c6eaab Compare June 27, 2019 22:55

rohitsinha54 closed this Jun 27, 2019

rohitsinha54 reopened this Jun 28, 2019

rohitsinha54 force-pushed the feature/CDAP-15588-private branch 2 times, most recently from d527eaf to 528d33a Compare June 28, 2019 05:07

rohitsinha54 changed the title ~~CDAP-15588: Private instance - if customer network is peered to tenan…~~ CDAP-15588: Prefer internal ip when vpc peering exists Jun 28, 2019

anew approved these changes Jun 28, 2019

View reviewed changes

albertshau reviewed Jun 28, 2019

View reviewed changes

rohitsinha54 commented Jul 2, 2019

View reviewed changes

rohitsinha54 commented Jul 4, 2019

View reviewed changes

rohitsinha54 force-pushed the feature/CDAP-15588-private branch from e1c740d to febb386 Compare July 6, 2019 21:35

albertshau reviewed Jul 8, 2019

View reviewed changes

sagarkapare reviewed Jul 8, 2019

View reviewed changes

albertshau reviewed Jul 8, 2019

View reviewed changes

albertshau approved these changes Jul 9, 2019

View reviewed changes

CDAP-15588: Add support for using internal ip with vpc peering

6459fc7

sagarkapare force-pushed the feature/CDAP-15588-private branch from 072c9d5 to 6459fc7 Compare July 9, 2019 00:36

If customer does not have peering handle null and throw helpful error

b814eb0

rohitsinha54 commented Jul 9, 2019

View reviewed changes

Fixed for private dataproc clsuter

3fb7027

albertshau reviewed Jul 9, 2019

View reviewed changes

Add better error handling for peering required

a2d0951

chtyim requested changes Jul 10, 2019

View reviewed changes

Address comments

3f48a36

albertshau reviewed Jul 10, 2019

View reviewed changes

rohitsinha54 added 2 commits July 10, 2019 14:43

Address comments

6020890

Remove fetching system network info

b94dfd8

albertshau approved these changes Jul 10, 2019

View reviewed changes

rohitsinha54 requested a review from chtyim July 10, 2019 23:12

remove error out in lack active peering

5e9fb76

rohitsinha54 force-pushed the feature/CDAP-15588-private branch from 0c6df95 to 5e9fb76 Compare July 11, 2019 03:03

chtyim approved these changes Jul 11, 2019

View reviewed changes

rohitsinha54 merged commit 8cc4949 into release/6.0 Jul 11, 2019

chtyim deleted the feature/CDAP-15588-private branch August 19, 2019 15:50

CDAP-15588: Prefer internal ip when vpc peering exists #11457

CDAP-15588: Prefer internal ip when vpc peering exists #11457

Conversation

rohitsinha54 commented Jun 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anew left a 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

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

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

sagarkapare Jul 8, 2019 • edited

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

albertshau commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagarkapare commented Jul 9, 2019

albertshau left a comment

Choose a reason for hiding this comment

sagarkapare commented Jul 9, 2019

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

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

albertshau left a comment

Choose a reason for hiding this comment

rohitsinha54 commented Jul 10, 2019

rohitsinha54 commented Jul 11, 2019

Choose a reason for hiding this comment

rohitsinha54 commented Jun 27, 2019 •

edited

sagarkapare Jul 8, 2019 •

edited