Fix: Service responses are not compressed (#6360) #6412

dsotirho-ucsc · 2024-07-16T00:30:23Z

Connected issues: #6360

Checklist

Author

PR is a draft
Target branch is develop
Name of PR branch matches issues/<GitHub handle of author>/<issue#>-<slug>
On ZenHub, PR is connected to all issues it (partially) resolves
PR description links to connected issues
PR title matches¹ that of a connected issue _{or comment in PR explains why they're different}
PR title references all connected issues
For each connected issue, there is at least one commit whose title references that issue

¹ when the issue title describes a problem, the corresponding PR
title is Fix: followed by the issue title

Author (partiality)

Added p tag to titles of partial commits
This PR is labeled partial _{or completely resolves all connected issues}
This PR partially resolves each of the connected issues _{or does not have the partial label}

Author (chains)

This PR is blocked by previous PR in the chain _{or is not chained to another PR}
The blocking PR is labeled base _{or this PR is not chained to another PR}
This PR is labeled chained _{or is not chained to another PR}

Author (reindex, API changes)

Added r tag to commit title _{or the changes introduced by this PR will not require reindexing of any deployment}
This PR is labeled reindex:dev _{or the changes introduced by it will not require reindexing of dev}
This PR is labeled reindex:anvildev _{or the changes introduced by it will not require reindexing of anvildev}
This PR is labeled reindex:anvilprod _{or the changes introduced by it will not require reindexing of anvilprod}
This PR is labeled reindex:prod _{or the changes introduced by it will not require reindexing of prod}
This PR is labeled reindex:partial and its description documents the specific reindexing procedure for dev, anvildev, anvilprod and prod _{or requires a full reindex or carries none of the labels reindex:dev, reindex:anvildev, reindex:anvilprod and reindex:prod}
This PR and its connected issues are labeled API _{or this PR does not modify a REST API}
Added a (A) tag to commit title for backwards (in)compatible changes _{or this PR does not modify a REST API}
Updated REST API version number in app.py _{or this PR does not modify a REST API}

Author (upgrading deployments)

Ran make image_manifests.json and committed the resulting changes _{or this PR does not modify azul_docker_images, or any other variables referenced in the definition of that variable}
Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading deployments}
Added u tag to commit title _{or this PR does not require upgrading deployments}
This PR is labeled upgrade _{or does not require upgrading deployments}
This PR is labeled deploy:shared _{or does not modify image_manifests.json, and does not require deploying the shared component for any other reason}
This PR is labeled deploy:gitlab _{or does not require deploying the gitlab component}
This PR is labeled deploy:runner _{or does not require deploying the runner image}

Author (hotfixes)

Added F tag to main commit title _{or this PR does not include permanent fix for a temporary hotfix}
Reverted the temporary hotfixes for any connected issues _{or the none of the stable branches (anvilprod and prod) have temporary hotfixes for any of the issues connected to this PR}

Author (before every review)

Rebased PR branch on develop, squashed old fixups
Ran make requirements_update _{or this PR does not modify requirements*.txt, common.mk, Makefile and Dockerfile}
Added R tag to commit title _{or this PR does not modify requirements*.txt}
This PR is labeled reqs _{or does not modify requirements*.txt}
make integration_test passes in personal deployment _{or this PR does not modify functionality that could affect the IT outcome}

Peer reviewer (after approval)

PR is not a draft
Ticket is in Review requested column
PR is awaiting requested review from system administrator
PR is assigned to only the system administrator

System administrator (after approval)

Actually approved the PR
Labeled connected issues as demo or no demo
Commented on connected issues about demo expectations _{or all connected issues are labeled no demo}
Decided if PR can be labeled no sandbox
A comment to this PR details the completed security design review
PR title is appropriate as title of merge commit
N reviews label is accurate
Moved ticket to Approved column
PR is assigned to only the operator

Operator (before pushing merge the commit)

System administrator

Background migrations for dev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
Background migrations for anvildev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
PR is assigned to only the operator

Operator (before pushing merge the commit)

Operator (chain shortening)

Changed the target branch of the blocked PR to develop _{or this PR is not labeled base}
Removed the chained label from the blocked PR _{or this PR is not labeled base}
Removed the blocking relationship from the blocked PR _{or this PR is not labeled base}
Removed the base label from this PR _{or this PR is not labeled base}

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Propagated the deploy:shared, deploy:gitlab, deploy:runner, API, reindex:partial, reindex:anvilprod and reindex:prod labels to the next promotion PRs _{or this PR carries none of these labels}
Propagated any specific instructions related to the deploy:shared, deploy:gitlab, deploy:runner, API, reindex:partial, reindex:anvilprod and reindex:prod labels, from the description of this PR to that of the next promotion PRs _{or this PR carries none of these labels}
PR is assigned to no one

Shorthand for review comments

L line is too long
W line wrapping is wrong
Q bad quotes
F other formatting problem

coveralls · 2024-07-16T00:53:15Z

coverage: 85.413% (-0.03%) from 85.438%
when pulling 606e717 on issues/dsotirho-ucsc/6360-compressed-responses
into e1bb690 on develop.

codecov · 2024-07-16T00:54:02Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.39%. Comparing base (e1bb690) to head (606e717).
Report is 2 commits behind head on develop.

Files	Patch %	Lines
src/azul/terraform.py	0.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6412      +/-   ##
===========================================
- Coverage    85.42%   85.39%   -0.03%     
===========================================
  Files          156      156              
  Lines        20674    20680       +6     
===========================================
  Hits         17660    17660              
- Misses        3014     3020       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dsotirho-ucsc · 2024-07-30T17:09:39Z

6412_IT_2024-07-30.txt

$ curl -v 'https://service.daniel.dev.singlecell.gi.ucsc.edu/index/bundles?catalog=dcp3' -H 'Accept: application/json' -H 'Accept-Encoding: gzip,deflate'
* Host service.daniel.dev.singlecell.gi.ucsc.edu:443 was resolved.
* IPv6: (none)
* IPv4: 18.239.199.125, 18.239.199.107, 18.239.199.100, 18.239.199.86
*   Trying 18.239.199.125:443...
* Connected to service.daniel.dev.singlecell.gi.ucsc.edu (18.239.199.125) port 443
* ALPN: curl offers h2,http/1.1
* (304) (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/cert.pem
*  CApath: none
* (304) (IN), TLS handshake, Server hello (2):
* (304) (IN), TLS handshake, Unknown (8):
* (304) (IN), TLS handshake, Certificate (11):
* (304) (IN), TLS handshake, CERT verify (15):
* (304) (IN), TLS handshake, Finished (20):
* (304) (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / AEAD-AES128-GCM-SHA256 / [blank] / UNDEF
* ALPN: server accepted h2
* Server certificate:
*  subject: CN=service.daniel.dev.singlecell.gi.ucsc.edu
*  start date: Jun 27 00:00:00 2024 GMT
*  expire date: Jul 26 23:59:59 2025 GMT
*  subjectAltName: host "service.daniel.dev.singlecell.gi.ucsc.edu" matched cert's "service.daniel.dev.singlecell.gi.ucsc.edu"
*  issuer: C=US; O=Amazon; CN=Amazon RSA 2048 M03
*  SSL certificate verify ok.
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://service.daniel.dev.singlecell.gi.ucsc.edu/index/bundles?catalog=dcp3
* [HTTP/2] [1] [:method: GET]
* [HTTP/2] [1] [:scheme: https]
* [HTTP/2] [1] [:authority: service.daniel.dev.singlecell.gi.ucsc.edu]
* [HTTP/2] [1] [:path: /index/bundles?catalog=dcp3]
* [HTTP/2] [1] [user-agent: curl/8.6.0]
* [HTTP/2] [1] [accept: application/json]
* [HTTP/2] [1] [accept-encoding: gzip,deflate]
> GET /index/bundles?catalog=dcp3 HTTP/2
> Host: service.daniel.dev.singlecell.gi.ucsc.edu
> User-Agent: curl/8.6.0
> Accept: application/json
> Accept-Encoding: gzip,deflate
>
< HTTP/2 200
< content-type: application/json
< content-length: 27738
< date: Tue, 30 Jul 2024 17:07:52 GMT
< x-amzn-trace-id: Root=1-66a91de8-4f83a89e23e4684d231a50ac;Parent=079cfce87090b573;Sampled=0;lineage=18704a40:0
< x-amzn-requestid: 575cabde-ba97-4819-955c-226748833daf
< access-control-allow-origin: *
< content-encoding: gzip
< strict-transport-security: max-age=31536000; includeSubDomains
< access-control-allow-headers: Authorization,Content-Type,X-Amz-Date,X-Amz-Security-Token,X-Api-Key
< x-frame-options: DENY
< x-amz-apigw-id: bvGcXEqEIAMEMxg=
< cache-control: no-store
< x-content-type-options: nosniff
< x-cache: Miss from cloudfront
< via: 1.1 55e9464473b96a5fed56600591202122.cloudfront.net (CloudFront)
< x-amz-cf-pop: SFO53-P3
< x-amz-cf-id: E7bSLuulpl0XpMcBfnw85i3NTKajjdpDrjanCUkuhDQDlXhvfQylrA==
<
Warning: Binary output can mess up your terminal. Use "--output -" to tell
Warning: curl to output it to your terminal anyway, or consider "--output
Warning: <FILE>" to save to a file.
* Failure writing output to destination
* Connection #0 to host service.daniel.dev.singlecell.gi.ucsc.edu left intact

achave11-ucsc

Consider the following suggestion, otherwise looks good!

achave11-ucsc · 2024-07-31T04:33:53Z

src/azul/terraform.py

+        # Using these AWS API Gateway extensions to the OpenAPI specification
+        # avoids maintaining a complicated `replace_triggered_by` and `triggers`
+        # dependency between Terraform resources in order to ensure that the API
+        # gateway is deployed when these properties are modified.


Suggested change

# Using these AWS API Gateway extensions to the OpenAPI specification

# avoids maintaining a complicated `replace_triggered_by` and `triggers`

# dependency between Terraform resources in order to ensure that the API

# gateway is deployed when these properties are modified.

# Using these AWS API Gateway extensions to the OpenAPI specification

# eliminates the need to maintain a complicated `replace_triggered_by` and

# `triggers` dependency between Terraform resources, thereby ensuring the

# API Gateway resource is deployed when these properties are modified.

or as you may see appropriate.

Since there is only one of these API Gateway extensions being added in this PR, I changed the verbiage to be singular. Later, when other extensions are added in we can change the comment to be plural (e.g. "this property" -> "these properties").

dsotirho-ucsc · 2024-07-31T18:24:42Z

6412_IT_2024-07-31.txt

achave11-ucsc

Approved ✅

hannes-ucsc · 2024-08-02T00:00:11Z

lambdas/indexer/.chalice/config.json.template.py

@@ -25,7 +25,6 @@
    "manage_iam_role": False,
    "iam_role_arn": "${aws_iam_role.%s.arn}" % app_name,
    "environment_variables": config.lambda_env,
-    "minimum_compression_size": config.minimum_compression_size,


I'm having a hard time parsing this, probably due to the elaborate prompt setup you have going there. We usually use $ to indicate a shell prompt in terminal transcripts. Could you simplify your evidence according to that convention. The */.chalice/terraform/chalice.tf.json files are intermediates, what counts ultimately are the tf.json files in terraform/. Please use those. Lastly, please use jq to extract the resource that would contain the conflicting property.

hannes-ucsc · 2024-08-02T16:13:12Z

lambdas/indexer/.chalice/config.json.template.py

@@ -25,7 +25,6 @@
    "manage_iam_role": False,
    "iam_role_arn": "${aws_iam_role.%s.arn}" % app_name,
    "environment_variables": config.lambda_env,
-    "minimum_compression_size": config.minimum_compression_size,


Scratch that. In fact, since we're post-processing the Chalice-generated TF config, please just add an assertion that the corresponding TF resource property is absent. Then we won't need any evidence.

dsotirho-ucsc · 2024-08-02T18:25:53Z

6412_IT_2024-08-02.txt

hannes-ucsc · 2024-08-03T03:27:48Z

src/azul/terraform.py

+        # dependency between Terraform resources.
+        #
+        rest_api = resources['aws_api_gateway_rest_api'][app_name]
+        assert 'minimum_compression_size' not in rest_api, rest_api.keys()


Suggested change

assert 'minimum_compression_size' not in rest_api, rest_api.keys()

assert 'minimum_compression_size' not in rest_api, rest_api

When it's unexpectedly set, you'd want to see what it's set to.

hannes-ucsc · 2024-08-03T03:32:40Z

src/azul/terraform.py

+        # specification lets us avoid maintaining a complicated trigger
+        # dependency between Terraform resources.


This will not make sense to anyone not intimately familiar with the issue. I think you shouldn't explain what the solution was (the complicated dependencies) but rather what the problem was (the property reset, and when it occurred).

dsotirho-ucsc · 2024-08-07T22:56:24Z

6412_IT_2024-08-07.txt

dsotirho-ucsc · 2024-08-12T17:20:45Z

6412_IT_2024-08-12.txt

hannes-ucsc · 2024-08-13T00:48:54Z

Security design review

github-actions bot added the orange [process] Done by the Azul team label Jul 16, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch 11 times, most recently from 56e0429 to 86f06dd Compare July 19, 2024 22:46

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from 86f06dd to 8594d4a Compare July 25, 2024 16:58

dsotirho-ucsc assigned hannes-ucsc and unassigned hannes-ucsc Jul 26, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from 8594d4a to d5cea4b Compare July 30, 2024 16:01

dsotirho-ucsc requested a review from achave11-ucsc July 30, 2024 17:10

dsotirho-ucsc assigned achave11-ucsc Jul 30, 2024

achave11-ucsc reviewed Jul 31, 2024

View reviewed changes

achave11-ucsc removed their assignment Jul 31, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from 820ce2b to e09ec05 Compare July 31, 2024 17:36

dsotirho-ucsc requested a review from achave11-ucsc July 31, 2024 18:24

dsotirho-ucsc assigned achave11-ucsc Jul 31, 2024

achave11-ucsc previously approved these changes Jul 31, 2024

View reviewed changes

achave11-ucsc marked this pull request as ready for review July 31, 2024 19:51

achave11-ucsc requested a review from hannes-ucsc as a code owner July 31, 2024 19:51

dsotirho-ucsc requested a review from hannes-ucsc August 1, 2024 23:00

dsotirho-ucsc assigned hannes-ucsc Aug 1, 2024

hannes-ucsc requested changes Aug 2, 2024

View reviewed changes

hannes-ucsc removed their assignment Aug 2, 2024

hannes-ucsc requested changes Aug 2, 2024

View reviewed changes

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from c5ea756 to a2ae7b7 Compare August 2, 2024 18:01

dsotirho-ucsc requested a review from hannes-ucsc August 2, 2024 21:39

dsotirho-ucsc assigned hannes-ucsc Aug 2, 2024

hannes-ucsc requested changes Aug 3, 2024

View reviewed changes

hannes-ucsc removed their assignment Aug 3, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from a2ae7b7 to 25220b3 Compare August 7, 2024 18:58

dsotirho-ucsc requested a review from nadove-ucsc as a code owner August 7, 2024 18:58

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from 25220b3 to c5d660d Compare August 7, 2024 22:13

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from c5d660d to 9004360 Compare August 12, 2024 16:18

dsotirho-ucsc requested a review from hannes-ucsc August 12, 2024 17:25

dsotirho-ucsc assigned hannes-ucsc Aug 12, 2024

hannes-ucsc approved these changes Aug 13, 2024

View reviewed changes

hannes-ucsc removed the request for review from nadove-ucsc August 13, 2024 00:48

hannes-ucsc assigned achave11-ucsc and unassigned hannes-ucsc Aug 13, 2024

Fix: Service responses are not compressed (#6360)

606e717

achave11-ucsc force-pushed the issues/dsotirho-ucsc/6360-compressed-responses branch from 9004360 to 606e717 Compare August 14, 2024 23:59

achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Aug 14, 2024

achave11-ucsc merged commit d4f970b into develop Aug 15, 2024
11 checks passed

achave11-ucsc deleted the issues/dsotirho-ucsc/6360-compressed-responses branch August 15, 2024 15:50

achave11-ucsc removed their assignment Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Service responses are not compressed (#6360) #6412

Fix: Service responses are not compressed (#6360) #6412

dsotirho-ucsc commented Jul 16, 2024 •

edited by achave11-ucsc

Loading

coveralls commented Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 16, 2024 •

edited

Loading

dsotirho-ucsc commented Jul 30, 2024

achave11-ucsc left a comment

achave11-ucsc Jul 31, 2024

dsotirho-ucsc Jul 31, 2024

dsotirho-ucsc commented Jul 31, 2024

achave11-ucsc left a comment

hannes-ucsc Aug 2, 2024

hannes-ucsc Aug 2, 2024

dsotirho-ucsc commented Aug 2, 2024

hannes-ucsc Aug 3, 2024

hannes-ucsc Aug 3, 2024

dsotirho-ucsc commented Aug 7, 2024

dsotirho-ucsc commented Aug 12, 2024

hannes-ucsc commented Aug 13, 2024

	assert 'minimum_compression_size' not in rest_api, rest_api.keys()
	assert 'minimum_compression_size' not in rest_api, rest_api

		# specification lets us avoid maintaining a complicated trigger
		# dependency between Terraform resources.

Fix: Service responses are not compressed (#6360) #6412

Fix: Service responses are not compressed (#6360) #6412

Conversation

dsotirho-ucsc commented Jul 16, 2024 • edited by achave11-ucsc Loading

Checklist

Author

Author (partiality)

Author (chains)

Author (reindex, API changes)

Author (upgrading deployments)

Author (hotfixes)

Author (before every review)

Peer reviewer (after approval)

System administrator (after approval)

Operator (before pushing merge the commit)

System administrator

Operator (before pushing merge the commit)

Operator (chain shortening)

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Shorthand for review comments

coveralls commented Jul 16, 2024 • edited Loading

codecov bot commented Jul 16, 2024 • edited Loading

Codecov Report

dsotirho-ucsc commented Jul 30, 2024

achave11-ucsc left a comment

Choose a reason for hiding this comment

achave11-ucsc Jul 31, 2024

Choose a reason for hiding this comment

dsotirho-ucsc Jul 31, 2024

Choose a reason for hiding this comment

dsotirho-ucsc commented Jul 31, 2024

achave11-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc Aug 2, 2024

Choose a reason for hiding this comment

hannes-ucsc Aug 2, 2024

Choose a reason for hiding this comment

dsotirho-ucsc commented Aug 2, 2024

hannes-ucsc Aug 3, 2024

Choose a reason for hiding this comment

hannes-ucsc Aug 3, 2024

Choose a reason for hiding this comment

dsotirho-ucsc commented Aug 7, 2024

dsotirho-ucsc commented Aug 12, 2024

hannes-ucsc commented Aug 13, 2024

Security design review

dsotirho-ucsc commented Jul 16, 2024 •

edited by achave11-ucsc

Loading

coveralls commented Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 16, 2024 •

edited

Loading