build: refactor external rule tests #522

sofisl · 2020-11-23T07:46:11Z

Fixes #520

This test is currently failing because we are attempting to create an external forwarding rule, which I believe gce enforcer is blocking. So, I've tried refactoring the code so that we create an internal forwarding rule and test that.

I tried many different ways of setting up this rule, including following the how-to guides here. I could not get them to work. The only way I was successful was by using the CLI, even after trying to convert the CLI commands into REST API.

I know this is not the ideal solution, but I've got nothing better. Would love to get a second pair of eyes on this if possible.

stephenplusplus

I went through and found our API's comparable methods to the SDK calls.

system-test/compute.js

stephenplusplus · 2020-11-23T15:57:04Z

system-test/compute.js

+    after(async () => {
+      const [firewalls] = await compute.getFirewalls();
+      const firewallsToDelete = firewalls
+        .filter(x => x.name.includes('network-name'))


Suggested change

.filter(x => x.name.includes('network-name'))

.filter(x => x.name.startsWith(TESTS_PREFIX))

stephenplusplus · 2020-11-23T16:01:00Z

system-test/compute.js

+        .filter(x => x.name.includes('network-name'))
+        .map(y => y.name);
+      for (const firewall of firewallsToDelete) {
+        await deleteFirewallRule(firewall);


Would the method firewall.delete() work? (You'd have to remove the .map(y => y.name) part above)

stephenplusplus · 2020-11-23T16:06:38Z

system-test/compute.js

+      execSync(
+        `gcloud compute networks create ${NETWORK_NAME} --subnet-mode=custom`
+      );


Suggested change

execSync(

`gcloud compute networks create ${NETWORK_NAME} --subnet-mode=custom`

);

const [network, operation] = await compute.createNetwork(NETWORK_NAME, {autoCreateSubnetworks: false});

await operation.promise();

^ I'm not sure that's equivalent. Just a guess from the docs: 'When set to true, the VPC network is created in auto mode. When set to false, the VPC network is created in custom mode.'

stephenplusplus · 2020-11-23T16:09:09Z

system-test/compute.js

+      execSync(
+        `gcloud compute networks subnets create ${SUBNETWORK_NAME} --network=${NETWORK_NAME} --range=10.0.1.0/24 --region=us-central1`
+      );


Suggested change

execSync(

`gcloud compute networks subnets create ${SUBNETWORK_NAME} --network=${NETWORK_NAME} --range=10.0.1.0/24 --region=us-central1`

);

const [subnetwork, operation] = await network.createSubnetwork(SUBNETWORK_NAME, {

region: 'us-central1',

range: '10.0.1.0/24'

});

await operation.promise();

stephenplusplus · 2020-11-23T16:18:11Z

system-test/compute.js

+      execSync(
+        `gcloud compute backend-services create ${BACKEND_SERVICE_NAME} --load-balancing-scheme=INTERNAL --region=us-central1`
+      );


Suggested change

execSync(

`gcloud compute backend-services create ${BACKEND_SERVICE_NAME} --load-balancing-scheme=INTERNAL --region=us-central1`

);

await computeRequest({

method: 'POST',

uri: '/regions/us-central1/backendServices',

json: {

name: BACKEND_SERVICE_NAME,

loadBalancingScheme: 'INTERNAL',

},

});

stephenplusplus · 2020-11-23T16:25:58Z

system-test/compute.js

+      execSync(
+        `gcloud compute forwarding-rules create ${RULE_NAME} --load-balancing-scheme=INTERNAL --backend-service=${BACKEND_SERVICE_NAME} --subnet=${SUBNETWORK_NAME}  --network=${NETWORK_NAME} --region=us-central1 --ports=80-82`
+      );


Suggested change

execSync(

`gcloud compute forwarding-rules create ${RULE_NAME} --load-balancing-scheme=INTERNAL --backend-service=${BACKEND_SERVICE_NAME} --subnet=${SUBNETWORK_NAME} --network=${NETWORK_NAME} --region=us-central1 --ports=80-82`

);

const region = compute.region('us-central1');

const [rule, operation] = region.createRule(RULE_NAME, {

loadBalancingScheme: 'INTERNAL',

backendService: BACKEND_SERVICE_NAME,

subnetwork: SUBNETWORK_NAME,

network: NETWORK_NAME,

ports: ['80', '81', '82'],

});

await operation.promise();

stephenplusplus · 2020-11-23T16:26:44Z

system-test/compute.js

+      }
+      await deleteForwardingRules(RULE_NAME);
+      await deleteBackendService(BACKEND_SERVICE_NAME);
+      await deleteSubnetworks(SUBNETWORK_NAME);


Suggested change

await deleteSubnetworks(SUBNETWORK_NAME);

const [operation] = await subnetwork.delete();

await operation.promise();

stephenplusplus · 2020-11-23T16:28:13Z

system-test/compute.js

+      for (const firewall of firewallsToDelete) {
+        await deleteFirewallRule(firewall);
+      }
+      await deleteForwardingRules(RULE_NAME);


Suggested change

await deleteForwardingRules(RULE_NAME);

const [operation] = await rule.delete();

await operation.promise():

Co-authored-by: Stephen <[email protected]>

codecov · 2020-11-24T06:09:36Z

Codecov Report

Merging #522 (566361e) into master (6e7def6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #522   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          22       22           
  Lines       11949    11949           
=======================================
  Hits        11903    11903           
  Misses         46       46

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7def6...566361e. Read the comment docs.

sofisl · 2020-11-24T21:30:37Z

@stephenplusplus, I keep getting this error when changing the methods to using the API, which I was getting before I just decided to use the CLI 🙂

Invalid value for field 'resource.subnetwork': 'gcloud-1158ae29-subnetwork-ab80d4bb'. The URL is malformed.

When I try to add the full URI (projects/projectId etc), I get this error:

Error: Invalid value for field 'resource.name': 'projects/long-door-651/regions/us-central1/subnetworks/gcloud-04490660-subnetwork-3a74b658'. Must be a match of regex '(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)'

Do you know how the name should actually be formed?

stephenplusplus · 2020-11-30T23:39:14Z

@sofisl I ran into similar issues, but I think I was able to get the formatting correct. Here's a diff of what I tried: https://gist.github.com/stephenplusplus/87b66993a0627e114dc296bf6ea132f5

…is/nodejs-compute into skipExternalForwardingRuleTests

bcoe

👍 contingent on dropping the gaxios dependency.

bcoe · 2020-12-21T21:14:55Z

package.json

@@ -57,6 +57,7 @@
    "c8": "^7.0.0",
    "codecov": "^3.0.2",
    "concat-stream": "^2.0.0",
+    "gaxios": "^4.0.1",


I think we can drop this.

gcf-merge-on-green · 2020-12-22T03:24:01Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

sofisl added 2 commits November 18, 2020 12:55

build: skip tests that create an external forwarding rule

7d8427d

fix: testing error

8265dd6

sofisl requested a review from a team as a code owner November 23, 2020 07:46

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 23, 2020

product-auto-label bot added the api: compute Issues related to the googleapis/nodejs-compute API. label Nov 23, 2020

sofisl added 2 commits November 22, 2020 23:47

fix await

fee830d

fix comment

54e7b41

bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 23, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 23, 2020

sofisl mentioned this pull request Nov 23, 2020

build: skip tests for now #523

Merged

sofisl changed the title ~~build: refactor external rule tests~~ [DRAFT] build: refactor external rule tests Nov 23, 2020

stephenplusplus reviewed Nov 23, 2020

View reviewed changes

Update system-test/compute.js

f736017

Co-authored-by: Stephen <[email protected]>

Merge branch 'master' into skipExternalForwardingRuleTests

3234336

sofisl requested a review from stephenplusplus November 30, 2020 18:03

sofisl and others added 5 commits December 4, 2020 22:52

fix: create forwrarding rule

cf634de

fix: system test

fe09e94

Merge branch 'master' into skipExternalForwardingRuleTests

6339f75

fix: broken system test

1d281f6

Merge branch 'skipExternalForwardingRuleTests' of github.com:googleap…

c9bf82c

…is/nodejs-compute into skipExternalForwardingRuleTests

sofisl added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 5, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 5, 2020

sofisl changed the title ~~[DRAFT] build: refactor external rule tests~~ build: refactor external rule tests Dec 12, 2020

sofisl requested a review from bcoe December 12, 2020 09:06

bcoe approved these changes Dec 21, 2020

View reviewed changes

bcoe reviewed Dec 21, 2020

View reviewed changes

fix: remove gaxios

566361e

sofisl added the automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit label Dec 21, 2020

gcf-merge-on-green bot removed the automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit label Dec 22, 2020

sofisl merged commit 06d0297 into master Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: refactor external rule tests #522

build: refactor external rule tests #522

sofisl commented Nov 23, 2020

stephenplusplus left a comment

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

stephenplusplus Nov 23, 2020

codecov bot commented Nov 24, 2020 •

edited

Loading

sofisl commented Nov 24, 2020 •

edited

Loading

stephenplusplus commented Nov 30, 2020

bcoe left a comment

bcoe Dec 21, 2020

gcf-merge-on-green bot commented Dec 22, 2020

	.filter(x => x.name.includes('network-name'))
	.filter(x => x.name.startsWith(TESTS_PREFIX))

-      execSync(
-        `gcloud compute networks subnets create ${SUBNETWORK_NAME} --network=${NETWORK_NAME} --range=10.0.1.0/24 --region=us-central1`
-      );
+      const [subnetwork, operation] = await network.createSubnetwork(SUBNETWORK_NAME, {
+        region: 'us-central1',
+        range: '10.0.1.0/24'
+      });
+      await operation.promise();

-      execSync(
-        `gcloud compute backend-services create ${BACKEND_SERVICE_NAME} --load-balancing-scheme=INTERNAL --region=us-central1`
-      );
+      await computeRequest({
+        method: 'POST',
+        uri: '/regions/us-central1/backendServices',
+        json: {
+          name: BACKEND_SERVICE_NAME,
+          loadBalancingScheme: 'INTERNAL',
+        },
+      });

-      execSync(
-        `gcloud compute forwarding-rules create ${RULE_NAME} --load-balancing-scheme=INTERNAL --backend-service=${BACKEND_SERVICE_NAME} --subnet=${SUBNETWORK_NAME}  --network=${NETWORK_NAME} --region=us-central1 --ports=80-82`
-      );
+      const region = compute.region('us-central1');
+      const [rule, operation] = region.createRule(RULE_NAME, {
+        loadBalancingScheme: 'INTERNAL',
+        backendService: BACKEND_SERVICE_NAME,
+        subnetwork: SUBNETWORK_NAME,
+        network: NETWORK_NAME,
+        ports: ['80', '81', '82'],
+      });
+      await operation.promise();

	await deleteSubnetworks(SUBNETWORK_NAME);
	const [operation] = await subnetwork.delete();
	await operation.promise();

	await deleteForwardingRules(RULE_NAME);
	const [operation] = await rule.delete();
	await operation.promise():

build: refactor external rule tests #522

build: refactor external rule tests #522

Conversation

sofisl commented Nov 23, 2020

stephenplusplus 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

codecov bot commented Nov 24, 2020 • edited Loading

Codecov Report

sofisl commented Nov 24, 2020 • edited Loading

stephenplusplus commented Nov 30, 2020

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcf-merge-on-green bot commented Dec 22, 2020

codecov bot commented Nov 24, 2020 •

edited

Loading

sofisl commented Nov 24, 2020 •

edited

Loading