feat(gce): Add GCP internal http(s) load balancer. #4725

takaaki7 · 2020-07-07T15:45:50Z

Added GCP Internal HTTP(S) load balancer support. spinnaker/spinnaker#5042
PR for deck: spinnaker/deck#8397

Many parts are copy and pasted from HTTP Load Balancer implementation.
Differences are:

It is regional
It has a network and subnet
It doesn't have legacy http(s) health checks
etc

spinnakerbot · 2020-07-07T15:50:22Z

We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:

clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/ops/loadbalancer/DeleteGoogleInternalHttpLoadBalancerAtomicOperation.groovy
clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/ops/loadbalancer/UpsertGoogleInternalHttpLoadBalancerAtomicOperation.groovy
clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/model/loadbalancing/GoogleInternalHttpLoadBalancer.groovy
clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/provider/agent/GoogleInternalHttpLoadBalancerCachingAgent.groovy

See our server-side commit conventions here.

...er-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/model/GoogleSubnet.groovy

maggieneterval · 2020-07-08T13:55:59Z

@takaaki7 Let's hold off on merging the corresponding Deck PR until this PR is merged. Per @spinnakerbot's comment, could you please ensure that any new files are added in Java rather than Groovy before we review this? Would be great if @plumpy could take a look as well since he is most familiar with the Clouddriver GCE implementation. 🙏 🍡

takaaki7 · 2020-07-08T14:51:35Z

Thanks for comment! I'll fix it.

takaaki7 · 2020-07-09T13:07:21Z

I converted groovy to Java. @plumpy Could you review this?

Actually I just want deployment feature of internal http lb for my project. No need Upsert,Delete feature because I can manage loadbalancers by terraform.

And I noticed, this PR might be split into two part. (deploy and Upser, Delete)

If PR for only deployment feature is acceptable (and if merging this large PR is hard), I'll try to split.

… global lbs

takaaki7 · 2020-07-12T09:21:44Z

...n/groovy/com/netflix/spinnaker/clouddriver/google/provider/view/GoogleClusterProvider.groovy

@@ -220,7 +221,7 @@ class GoogleClusterProvider implements ClusterProvider<GoogleCluster.View> {
      serverGroupData.each { CacheData serverGroupCacheData ->
        GoogleServerGroup serverGroup = serverGroupFromCacheData(serverGroupCacheData, clusterView.accountName, instances, securityGroups, loadBalancers)
        clusterView.serverGroups << serverGroup.view
-        clusterView.loadBalancers.addAll(serverGroup.loadBalancers*.view)
+        clusterView.loadBalancers.addAll(serverGroup.loadBalancers)


Referencing nonexistent property GoogleLoadBalancerView.view.
Using java, this line causes exception.

takaaki7 · 2020-07-13T08:19:16Z

How is the progress?

plumpy · 2020-07-13T19:47:05Z

Sorry for the delay, but the PR build is failing because of a Groovy compilation error, so I was waiting for that to be resolved before I looked at it... if you don't know how to fix it, I can try to figure it out, I guess...

It's going to take me a little bit to review this since it's quite a lot of code and I don't know much about the internal HTTP load balancers, so I'll need to research and play around with those, but I'm hoping to get to it this week.

takaaki7 · 2020-07-14T10:29:04Z

I was waiting for that to be resolved before I looked at it...

Oh, sorry for that.

I looked at it but I couldn't figure it out...
I can run ./gradlew -PenableCrossCompilerPlugin=true build on local machine without error.

And the line causing the error seems no problem for me..
import com.netflix.spinnaker.clouddriver.security.ProviderVersion;

I’d like to ask you for your help.

maggieneterval · 2020-07-14T13:06:26Z

@takaaki7 ProviderVersion was recently deprecated and removed. I'll add a comment inline where you need to remove this.

...iver/google/deploy/ops/loadbalancer/UpsertGoogleInternalHttpLoadBalancerAtomicOperation.java

takaaki7 · 2020-07-21T13:48:57Z

Is it going to take a little longer? I know this PR is large, so I can consider different approach if prefered. But I want to hear your opinion at first.

plumpy

Sorry this is taking so long. This code is just so messy that it's hard to understand what's going on. (The existing GCE code, not your changes.) I'm trying to focus mostly on the parts that will affect users who aren't using internal HTTP load balancers, to make sure this doesn't cause any new bugs for them.

I really appreciate you sending this PR and I'm sorry I'm taking so long to get it reviewed. I'll make sure we get it wrapped up and submitted next week.

...google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/model/callbacks/Utils.groovy

plumpy · 2020-07-30T18:50:20Z

...ovy/com/netflix/spinnaker/clouddriver/google/deploy/handlers/BasicGoogleDeployHandler.groovy

@@ -488,7 +492,7 @@ class BasicGoogleDeployHandler implements DeployHandler<BasicGoogleDeployDescrip
        .setTargetPools(targetPools)
        .setAutoHealingPolicies(autoHealingPolicy)

-    if (hasBackendServices && (description?.loadBalancingPolicy || description?.source?.serverGroupName))  {
+    if (!internalLoadBalancers && (description?.loadBalancingPolicy || description?.source?.serverGroupName))  {


I'm curious about this change... it feels significant and risky, but it's hard to figure out what's happening in the middle of a 500-line method, with a variable defined 300 lines away 😨 Can you explain what this change does?

As it's written, lbs that isn't internal (= external http/tcp/ssl , and internal HTTP) requires namedPorts handling.
Because hasBackendServices is false when internal HTTP loadbalancers exists, now it's not appropriate here. (hasBackendServices should be renamed to hasGlobalBackendServices)

This condition can be written as following way to reduce the change impact, but I think it's not clean.
if ((hasBackendServices || internalHttpLoadBalancers) && (description?.loadBalancingPolicy || description?.source?.serverGroupName))

Thanks for the explanation! Isn't it possible that we have internalLoadBalancers and tcpLoadsBalancers (i.e. two types of load balancers for this server group)? In which case we need to configure the named ports, but as it's written here we won't?

Wow that's right.

fix

...er-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/model/GoogleSubnet.groovy

plumpy

Okay, I think this is all I have, nothing very major.

Thanks again for doing all this work, just clean up these smallish things and then I'll happily merge it. I really appreciate you doing this work (especially since you didn't even need all of it). 🍡

plumpy · 2020-08-06T03:39:16Z

...netflix/spinnaker/clouddriver/google/model/loadbalancing/GoogleInternalHttpLoadBalancer.java

+@ToString(includeSuper = true)
+@groovy.transform.EqualsAndHashCode(callSuper = true)


You can replace these with lombok:

Suggested change

@ToString(includeSuper = true)

@groovy.transform.EqualsAndHashCode(callSuper = true)

@Data

@EqualsAndHashCode(callSuper = true)

@ToString(callSuper = true)

Then:

mark type and loadBalancingScheme as final

delete all the getters and setters (except getView)

Thank you for detailed instruction! It's very helpful.

plumpy · 2020-08-06T03:39:55Z

...netflix/spinnaker/clouddriver/google/model/loadbalancing/GoogleInternalHttpLoadBalancer.java

+    return new InternalHttpLbView();
+  }
+
+  @EqualsAndHashCode(callSuper = false)


Suggested change

@EqualsAndHashCode(callSuper = false)

@Value

@EqualsAndHashCode(callSuper = true)

@ToString(callSuper = true)

Then delete the serverGroups field and all the methods.

plumpy · 2020-08-06T03:41:04Z

...tflix/spinnaker/clouddriver/google/provider/agent/AbstractGoogleServerGroupCachingAgent.java

-import static com.netflix.spinnaker.clouddriver.google.deploy.GCEUtil.GLOBAL_LOAD_BALANCER_NAMES;
-import static com.netflix.spinnaker.clouddriver.google.deploy.GCEUtil.LOAD_BALANCING_POLICY;
-import static com.netflix.spinnaker.clouddriver.google.deploy.GCEUtil.REGIONAL_LOAD_BALANCER_NAMES;
+import static com.netflix.spinnaker.clouddriver.google.deploy.GCEUtil.*;


don't use wildcard imports

plumpy · 2020-08-06T03:50:29Z

...iver/google/deploy/ops/loadbalancer/DeleteGoogleInternalHttpLoadBalancerAtomicOperation.java

+        healthCheckUrls.addAll(backendService.getHealthChecks());
+      }
+
+      DefaultGroovyMethods.unique(healthCheckUrls);


Probably you can just make healthCheckUrls into a Set?

plumpy · 2020-08-06T03:51:52Z

.../spinnaker/clouddriver/google/provider/agent/GoogleInternalHttpLoadBalancerCachingAgent.java

+    if (healthCheck.getTcpHealthCheck() != null) {
+      port = healthCheck.getTcpHealthCheck().getPort();
+      hcType = GoogleHealthCheck.HealthCheckType.TCP;
+    } else if (DefaultGroovyMethods.asBoolean(healthCheck.getSslHealthCheck())) {


All these DefaultGroovyMethods.asBoolean can just be != null, right?

takaaki7 · 2020-08-07T04:52:59Z

I've addressed your comments.

plumpy · 2020-08-07T15:29:08Z

Thanks again, I appreciate your contribution! And sorry again for the long delay...

takaaki7 · 2020-08-08T06:29:57Z

Thank you for merging! When will this feature be released?

maggieneterval · 2020-08-10T16:28:46Z

The next release is Spinnaker 1.22, for which the branches will be cut next week. You can subscribe to the Spinnaker release calendar or #spinnaker-releases Slack channel to see upcoming releases, and can also check the target-release label that spinnakerbot adds to PRs to determine for which release they're slated. Thanks!

takaaki7 · 2020-08-11T03:08:10Z

I see, thank you!

fix(provider/google): add internal http(s) load balancer. (spinnaker#…

50521ce

…5042)

takaaki7 changed the title ~~Add GCP internal http(s) load balancer.~~ feat(gce): Add GCP internal http(s) load balancer. Jul 7, 2020

takaaki7 commented Jul 7, 2020

View reviewed changes

...er-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/model/GoogleSubnet.groovy Show resolved Hide resolved

takaaki7 mentioned this pull request Jul 8, 2020

feat(gce): Add GCP internal http(s) load balancer. spinnaker/deck#8397

Merged

fix(provider/gce): Fix Internal HTTP LB implementations to use java.

fb27d94

plumpy self-requested a review July 9, 2020 12:56

takaaki7 added 2 commits July 12, 2020 18:16

fix(provider/gce): Fix internal http lb cannot be deployed with other…

aa0d8ee

… global lbs

fix(provider/gce): Fix referencing not existing property

3e5afcc

takaaki7 commented Jul 12, 2020

View reviewed changes

maggieneterval reviewed Jul 14, 2020

View reviewed changes

...iver/google/deploy/ops/loadbalancer/UpsertGoogleInternalHttpLoadBalancerAtomicOperation.java Outdated Show resolved Hide resolved

chore(gce): no use ProviderVersion

d7a6778

takaaki7 force-pushed the internal-lb-http branch from da0ecf1 to d7a6778 Compare July 14, 2020 17:30

plumpy reviewed Jul 30, 2020

View reviewed changes

takaaki7 added 3 commits August 5, 2020 00:49

Merge branch 'master' into internal-lb-http

0f9cb56

chore(gce): null check in common place

3935935

chore(gce): set subnet purpose default value n/a

08c0be4

takaaki7 force-pushed the internal-lb-http branch from 0ac9e15 to 08c0be4 Compare August 4, 2020 16:37

plumpy reviewed Aug 6, 2020

View reviewed changes

chore(gce): refactor internal http lb

d3129da

plumpy added the ready to merge Approved and ready for a merge label Aug 7, 2020

plumpy approved these changes Aug 7, 2020

View reviewed changes

mergify bot added the auto merged Merged automatically by a bot label Aug 7, 2020

Merge branch 'master' into internal-lb-http

89fc56e

mergify bot merged commit a34b951 into spinnaker:master Aug 7, 2020

spinnakerbot added the target-release/1.22 label Aug 7, 2020

ezimanyi mentioned this pull request Aug 11, 2020

fix(google): Fix assertion error on unknown target proxy #4789

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gce): Add GCP internal http(s) load balancer. #4725

feat(gce): Add GCP internal http(s) load balancer. #4725

takaaki7 commented Jul 7, 2020 •

edited

Loading

spinnakerbot commented Jul 7, 2020

maggieneterval commented Jul 8, 2020

takaaki7 commented Jul 8, 2020

takaaki7 commented Jul 9, 2020 •

edited

Loading

takaaki7 Jul 12, 2020

takaaki7 commented Jul 13, 2020

plumpy commented Jul 13, 2020

takaaki7 commented Jul 14, 2020

maggieneterval commented Jul 14, 2020

takaaki7 commented Jul 21, 2020

plumpy left a comment

plumpy Jul 30, 2020

takaaki7 Aug 2, 2020

plumpy Aug 6, 2020

takaaki7 Aug 7, 2020 •

edited

Loading

plumpy left a comment

plumpy Aug 6, 2020

takaaki7 Aug 7, 2020

plumpy Aug 6, 2020

plumpy Aug 6, 2020

plumpy Aug 6, 2020

plumpy Aug 6, 2020

takaaki7 commented Aug 7, 2020 •

edited

Loading

plumpy commented Aug 7, 2020

takaaki7 commented Aug 8, 2020

maggieneterval commented Aug 10, 2020

takaaki7 commented Aug 11, 2020

		@ToString(includeSuper = true)
		@groovy.transform.EqualsAndHashCode(callSuper = true)

feat(gce): Add GCP internal http(s) load balancer. #4725

feat(gce): Add GCP internal http(s) load balancer. #4725

Conversation

takaaki7 commented Jul 7, 2020 • edited Loading

spinnakerbot commented Jul 7, 2020

maggieneterval commented Jul 8, 2020

takaaki7 commented Jul 8, 2020

takaaki7 commented Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

takaaki7 commented Jul 13, 2020

plumpy commented Jul 13, 2020

takaaki7 commented Jul 14, 2020

maggieneterval commented Jul 14, 2020

takaaki7 commented Jul 21, 2020

plumpy 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

takaaki7 Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

plumpy 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

takaaki7 commented Aug 7, 2020 • edited Loading

plumpy commented Aug 7, 2020

takaaki7 commented Aug 8, 2020

maggieneterval commented Aug 10, 2020

takaaki7 commented Aug 11, 2020

takaaki7 commented Jul 7, 2020 •

edited

Loading

takaaki7 commented Jul 9, 2020 •

edited

Loading

takaaki7 Aug 7, 2020 •

edited

Loading

takaaki7 commented Aug 7, 2020 •

edited

Loading