-
Notifications
You must be signed in to change notification settings - Fork 4
add cloudflare cache purge #638
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
base: develop
Are you sure you want to change the base?
Changes from all commits
9e5778c
69ef42f
54acc74
75dec89
423ad77
db43cee
f130695
804441b
d7e5db6
9d92ddc
2fa320b
b8881a1
7e884ed
bc7951f
945878a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -830,19 +830,27 @@ func (api *DatasetAPI) putState(w http.ResponseWriter, r *http.Request) { | |
| Type: models.Static.String(), | ||
| } | ||
|
|
||
| updatedVersion, err := api.smDatasetAPI.AmendVersion(r.Context(), vars, versionUpdate) | ||
| _, err = api.smDatasetAPI.AmendVersion(r.Context(), vars, versionUpdate) | ||
| if err != nil { | ||
| handleVersionAPIErr(ctx, err, w, logData) | ||
| return | ||
| } | ||
|
|
||
| if stateUpdate.State == models.PublishedState && updatedVersion.Distributions != nil && len(*updatedVersion.Distributions) > 0 { | ||
| err = api.publishDistributionFiles(ctx, updatedVersion, logData) | ||
| if stateUpdate.State == models.PublishedState && currentVersion.Distributions != nil && len(*currentVersion.Distributions) > 0 { | ||
| err = api.publishDistributionFiles(ctx, currentVersion, logData) | ||
| if err != nil { | ||
| log.Error(ctx, "putState endpoint: failed to publish distribution files", err, logData) | ||
| handleVersionAPIErr(ctx, err, w, logData) | ||
| return | ||
| } | ||
|
|
||
| if api.cloudflareClient != nil { | ||
| if err := api.cloudflareClient.PurgeCacheByPrefix(ctx, datasetID, edition); err != nil { | ||
| log.Error(ctx, "failed to purge cloudflare cache", err, logData) | ||
| } else { | ||
| log.Info(ctx, "successfully triggered cloudflare cache purge", logData) | ||
| } | ||
| } | ||
|
Comment on lines
+847
to
+853
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the client is nil here then there is no trace and cache will not be cleared which is a big problem for support. Ideally the client should be confirmed as not nil at setup so we don't have the nil check here. |
||
| } | ||
|
|
||
| setJSONContentType(w) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| package cloudflare | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "time" | ||
|
|
||
| "github.com/ONSdigital/log.go/v2/log" | ||
| "github.com/cloudflare/cloudflare-go" | ||
| ) | ||
|
|
||
| // Clienter defines the interface for Cloudflare cache operations | ||
| type Clienter interface { | ||
| PurgeCacheByPrefix(ctx context.Context, datasetID, editionID string) error | ||
| } | ||
|
|
||
| // Client wraps the Cloudflare API client | ||
| type Client struct { | ||
| api *cloudflare.API | ||
| zoneID string | ||
| baseURL string | ||
| httpClient *http.Client | ||
| apiToken string | ||
| } | ||
|
|
||
| // New creates a new Cloudflare client | ||
| func New(apiToken, zoneID string, useSDK bool, baseURL ...string) (*Client, error) { | ||
| if apiToken == "" { | ||
| return nil, fmt.Errorf("cloudflare API token is required") | ||
| } | ||
| if zoneID == "" { | ||
| return nil, fmt.Errorf("cloudflare zone ID is required") | ||
| } | ||
|
|
||
| // if SDK disabled for local testing with the cloudflare stub, use the HTTP client | ||
| if !useSDK && len(baseURL) > 0 && baseURL[0] != "" { | ||
| return &Client{ | ||
| api: nil, | ||
| zoneID: zoneID, | ||
| baseURL: baseURL[0], | ||
| apiToken: apiToken, | ||
| httpClient: &http.Client{Timeout: 10 * time.Second}, | ||
| }, nil | ||
| } | ||
|
|
||
| // use real Cloudflare SDK | ||
| api, err := cloudflare.NewWithAPIToken(apiToken) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create cloudflare client: %w", err) | ||
| } | ||
|
|
||
| return &Client{ | ||
| api: api, | ||
| zoneID: zoneID, | ||
| }, nil | ||
| } | ||
|
|
||
| // PurgeCacheByPrefix purges the Cloudflare cache for dataset-related URLs | ||
| func (c *Client) PurgeCacheByPrefix(ctx context.Context, datasetID, editionID string) error { | ||
|
Comment on lines
+62
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this function shouldn't be in the SDK as it is specific to the dataset-api. The SDK should just accept a list of prefixes which it then purges (so any service can use the function). |
||
| prefixes := buildPrefixes(datasetID, editionID) | ||
|
|
||
| logData := log.Data{ | ||
| "dataset_id": datasetID, | ||
| "edition": editionID, | ||
| "prefixes": prefixes, | ||
| } | ||
| log.Info(ctx, "purging cloudflare cache", logData) | ||
|
|
||
| // local mock | ||
| if c.baseURL != "" { | ||
| return c.purgeCacheViaHTTP(ctx, prefixes) | ||
| } | ||
|
|
||
| // for sdk | ||
| return c.purgeCacheViaSDK(ctx, prefixes) | ||
| } | ||
|
|
||
| // purgeCacheViaSDK uses the cloudflare sdk | ||
| func (c *Client) purgeCacheViaSDK(ctx context.Context, prefixes []string) error { | ||
| params := cloudflare.PurgeCacheRequest{ | ||
| Prefixes: prefixes, | ||
| } | ||
|
|
||
| _, err := c.api.PurgeCache(ctx, c.zoneID, params) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to purge cache: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // purgeCacheViaHTTP makes direct HTTP call for local cloudflare stub | ||
| func (c *Client) purgeCacheViaHTTP(ctx context.Context, prefixes []string) error { | ||
| url := fmt.Sprintf("%s/zones/%s/purge_cache", c.baseURL, c.zoneID) | ||
|
|
||
| payload := map[string]interface{}{ | ||
| "prefixes": prefixes, | ||
| } | ||
|
|
||
| body, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal request: %w", err) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewBuffer(body)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.apiToken)) | ||
|
|
||
| resp, err := c.httpClient.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to make request: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| bodyBytes, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("purge failed with status %d: %s", resp.StatusCode, string(bodyBytes)) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func buildPrefixes(datasetID, editionID string) []string { | ||
| return []string{ | ||
| fmt.Sprintf("www.ons.gov.uk/datasets/%s", datasetID), | ||
| fmt.Sprintf("www.ons.gov.uk/datasets/%s/editions", datasetID), | ||
| fmt.Sprintf("www.ons.gov.uk/datasets/%s/editions/%s/versions", datasetID, editionID), | ||
| fmt.Sprintf("api.beta.ons.gov.uk/v1/datasets/%s", datasetID), | ||
| fmt.Sprintf("api.beta.ons.gov.uk/v1/datasets/%s/editions", datasetID), | ||
| fmt.Sprintf("api.beta.ons.gov.uk/v1/datasets/%s/editions/%s/versions", datasetID, editionID), | ||
| } | ||
|
Comment on lines
+131
to
+139
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the previous comment, this function should be in the dataset-api and not the SDK package. I would also avoid hard-coded domain names and instead build off config values |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package cloudflare_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/ONSdigital/dp-dataset-api/cloudflare" | ||
| . "github.com/smartystreets/goconvey/convey" | ||
| ) | ||
|
|
||
| func TestNew(t *testing.T) { | ||
| Convey("Given valid API token and zone ID", t, func() { | ||
| apiToken := "test-token" | ||
| zoneID := "test-zone-id" | ||
| useSDK := false | ||
|
|
||
| Convey("When creating a new Cloudflare client", func() { | ||
| client, err := cloudflare.New(apiToken, zoneID, useSDK) | ||
|
|
||
| Convey("Then no error is returned", func() { | ||
| So(err, ShouldBeNil) | ||
| }) | ||
|
|
||
| Convey("And the client is not nil", func() { | ||
| So(client, ShouldNotBeNil) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| Convey("Given an empty API token", t, func() { | ||
| apiToken := "" | ||
| zoneID := "test-zone-id" | ||
| useSDK := false | ||
|
|
||
| Convey("When creating a new Cloudflare client", func() { | ||
| client, err := cloudflare.New(apiToken, zoneID, useSDK) | ||
|
|
||
| Convey("Then an error is returned", func() { | ||
| So(err, ShouldNotBeNil) | ||
| So(err.Error(), ShouldContainSubstring, "API token is required") | ||
| }) | ||
|
|
||
| Convey("And the client is nil", func() { | ||
| So(client, ShouldBeNil) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| Convey("Given an empty zone ID", t, func() { | ||
| apiToken := "test-token" | ||
| zoneID := "" | ||
| useSDK := false | ||
|
|
||
| Convey("When creating a new Cloudflare client", func() { | ||
| client, err := cloudflare.New(apiToken, zoneID, useSDK) | ||
|
|
||
| Convey("Then an error is returned", func() { | ||
| So(err, ShouldNotBeNil) | ||
| So(err.Error(), ShouldContainSubstring, "zone ID is required") | ||
| }) | ||
|
|
||
| Convey("And the client is nil", func() { | ||
| So(client, ShouldBeNil) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| Convey("Given both empty API token and zone ID", t, func() { | ||
| apiToken := "" | ||
| zoneID := "" | ||
| useSDK := false | ||
|
|
||
| Convey("When creating a new Cloudflare client", func() { | ||
| client, err := cloudflare.New(apiToken, zoneID, useSDK) | ||
|
|
||
| Convey("Then an error is returned", func() { | ||
| So(err, ShouldNotBeNil) | ||
| }) | ||
|
|
||
| Convey("And the client is nil", func() { | ||
| So(client, ShouldBeNil) | ||
| }) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the change to
_instead ofupdatedVersionis due to the linter which looks ok.However
currentVersionis being used on lines 839 and 840 where it was previouslyupdatedVersion. I think it should remain asupdatedVersionunless there's an issue I'm not seeing here?