Skip to content

[usage] Request signed upload URL from content-service #11493

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

Merged
merged 7 commits into from
Jul 25, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Jul 20, 2022

Description

#11474 added a new UsageReportService to content-service that allowed S3 signed upload URLs to be retrieved for the purpose of uploading usage reports to cloud storage.

This PR hooks up the usage component with the content-service so that it requests a signed upload URL from content-service each time it runs.

Subsequent PRs will use this signed URL to actually upload usage reports to cloud storage.

Related Issue(s)

Part of #9036
Builds on #11474

How to test

Look at the logs for the usage component in the preview environment for this PR. When usage reconciliation runs it successfully requests a signed upload URL from the content-service:

image

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@andrew-farries
Copy link
Contributor Author

/hold because it's based on #11474

@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jul 20, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-upload-usage-reports.18 because the annotations in the pull request description changed
(with .werft/ from main)

@andrew-farries andrew-farries marked this pull request as draft July 21, 2022 07:46
@andrew-farries andrew-farries force-pushed the af/upload-usage-reports branch 3 times, most recently from 3ef8e01 to 68c28fc Compare July 22, 2022 06:10
@andrew-farries andrew-farries marked this pull request as ready for review July 22, 2022 06:10
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

I'm surprised a network policy was not needed to talk to the content service

@@ -20,6 +21,7 @@ import (
func configmap(ctx *common.RenderContext) ([]runtime.Object, error) {
cfg := server.Config{
ControllerSchedule: time.Hour.String(),
ContentServiceUrl: fmt.Sprintf("%s:%d", content_service.Component, content_service.RPCPort),
Copy link
Member

Choose a reason for hiding this comment

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

net.JoinHostPort instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. aed2c3f

@@ -23,6 +23,7 @@ func TestConfigMap_ContainsSchedule(t *testing.T) {
require.JSONEq(t,
`{
"controllerSchedule": "2m",
"contentServiceUrl": "content-service:8080",
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this is not a URL, just an address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed (b50982f)

Base automatically changed from af/store-usage-reports-with-content-service to main July 22, 2022 11:16
@roboquat roboquat requested a review from a team July 22, 2022 11:16
@roboquat roboquat added size/XXL and removed size/L labels Jul 22, 2022
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Jul 22, 2022
@andrew-farries
Copy link
Contributor Author

/unhold

@andrew-farries andrew-farries removed the request for review from a team July 22, 2022 11:20
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Jul 22, 2022

/werft run with-preview

👍 started the job as gitpod-build-af-upload-usage-reports.25
(with .werft/ from main)

@andrew-farries andrew-farries requested a review from easyCZ July 22, 2022 12:39
Andrew Farries added 7 commits July 23, 2022 19:32
Add a dependency on the `content-service-api` component.

And run `leeway link`.
To handle connecting to the content-service gRPC API, retrieving signed
upload URLs and uploading usage reports.
When the usage component starts up.
Populate the usage component's config file with the URL of the
in-cluster content-service.
Show that the connection to the content service is up and running by
requesting a signed upload URL and logging it.
@andrew-farries andrew-farries force-pushed the af/upload-usage-reports branch from f149203 to 7d88600 Compare July 23, 2022 18:32
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Jul 25, 2022

I'm surprised a network policy was not needed to talk to the content service

Yes, that is surprising. The content-service has a network policy that allows ingress from anywhere:

Spec: networkingv1.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{MatchLabels: labels},
PolicyTypes: []networkingv1.PolicyType{"Ingress"},
Ingress: []networkingv1.NetworkPolicyIngressRule{{}},
},

This should probably be more restrictive.

@roboquat roboquat merged commit 2eff43c into main Jul 25, 2022
@roboquat roboquat deleted the af/upload-usage-reports branch July 25, 2022 08:56
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Jul 26, 2022
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants