Skip to content

Commit

Permalink
fix: store update (#1052)
Browse files Browse the repository at this point in the history
## Description

Fixes store since etcd is unescaping the store values.
e2e defenseunicorns/pepr-excellent-examples#53
## Related Issue

Fixes #1047 
<!-- or -->
Relates to #

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://docs.pepr.dev/main/contribute/#submitting-a-pull-request)
followed

---------

Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
  • Loading branch information
cmwylie19 authored Aug 15, 2024
1 parent ef7a141 commit 74f5872
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 30 deletions.
9 changes: 7 additions & 2 deletions journey/pepr-deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ function testMutate() {
const cm5 = await waitForConfigMap("pepr-demo", "example-5");

expect(cm5.metadata?.annotations?.["static-test.pepr.dev/hello-pepr"]).toBe("succeeded");
expect(cm5.data?.["chuck-says"]).toBeTruthy();
});

it("should mutate secret-1", async () => {
Expand Down Expand Up @@ -292,7 +291,13 @@ function testStore() {

// Should have been migrated and removed
const nullKey2 = await noWaitPeprStoreKey("pepr-static-test-store", `hello-pepr-example-1-data`);
expect(nullKey1).toBeUndefined();
expect(nullKey2).toBeUndefined();

// Should have a key from the joke url and getItem should have worked
const key3 = await waitForPeprStoreKey("pepr-static-test-store", `hello-pepr-v2-https://icanhazdadjoke.com/`);
expect(key3).toContain(" ");
const cm = await waitForConfigMap("pepr-demo", "example-5");
expect(cm.data?.["chuck-says"]).toBeTruthy();
});

it("should write the correct data to the PeprStore from a Watch Action", async () => {
Expand Down
2 changes: 0 additions & 2 deletions src/lib/controller/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ export class PeprControllerStore {
fillCache(name, "add", [key.slice(offset)], data[key]);
}
}

// await K8s(PeprStore, { namespace, name: this.#name }).Patch(payload);
}
await flushCache();
this.#setupWatch();
Expand Down
35 changes: 24 additions & 11 deletions src/lib/storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-FileCopyrightText: 2023-Present The Pepr Authors

import { beforeEach, describe, expect, it, jest } from "@jest/globals";
import { DataStore, Storage, v2StoreKey, stripV2Prefix } from "./storage";
import { DataStore, Storage, v2StoreKey, v2UnescapedStoreKey, stripV2Prefix } from "./storage";
import fc from "fast-check";

describe("stripV2Prefix", () => {
Expand All @@ -27,6 +27,18 @@ describe("v2StoreKey", () => {
}
});
});

describe("v2UnescapedStoreKey", () => {
it("should prefix the key with v2", () => {
const keys = ["https://google.com", "sso-client-http://bin", "key3", "key4", "key5"];
const results = ["v2-https://google.com", "v2-sso-client-http://bin", "v2-key3", "v2-key4", "v2-key5"];

for (let i = 0; i < keys.length; i++) {
const result = v2UnescapedStoreKey(keys[i]);
expect(result).toEqual(results[i]);
}
});
});
describe("Storage with fuzzing and property-based tests", () => {
let storage: Storage;

Expand All @@ -39,7 +51,7 @@ describe("Storage with fuzzing and property-based tests", () => {
fc.assert(
fc.property(fc.string(), fc.string(), (key, value) => {
storage.setItem(key, value);
const mockData: DataStore = { [v2StoreKey(key)]: value };
const mockData: DataStore = { [v2UnescapedStoreKey(key)]: value };
storage.receive(mockData);
if (value === "") {
expect(storage.getItem(key)).toBeNull();
Expand Down Expand Up @@ -75,7 +87,7 @@ describe("Storage with fuzzing and property-based tests", () => {
fc.assert(
fc.property(fc.string(), fc.string(), (key, value) => {
storage.setItem(key, value);
const mockData: DataStore = { [v2StoreKey(key)]: value };
const mockData: DataStore = { [v2UnescapedStoreKey(key)]: value };
storage.receive(mockData);
if (value === "") {
expect(storage.getItem(key)).toBeNull();
Expand All @@ -100,19 +112,20 @@ describe("Storage", () => {
const key = "key1";
storage.setItem(key, "value1");

expect(mockSender).toHaveBeenCalledWith("add", [v2StoreKey(key)], "value1");
expect(mockSender).toHaveBeenCalledWith("add", [v2UnescapedStoreKey(key)], "value1");
});

it("should set an item and wait", () => {
const mockSender = jest.fn();
storage.registerSender(mockSender);
jest.useFakeTimers();
const key = "key1";
const keys = ["key1", "https://google.com", "sso-client-http://bin"];

// asserting on sender invocation rather than Promise so no need to wait
void storage.setItemAndWait(key, "value1");
keys.map(key => {
void storage.setItemAndWait(key, "value1");
expect(mockSender).toHaveBeenCalledWith("add", [v2StoreKey(key)], "value1");
});

expect(mockSender).toHaveBeenCalledWith("add", [v2StoreKey(key)], "value1");
jest.useRealTimers();
});

Expand Down Expand Up @@ -142,18 +155,18 @@ describe("Storage", () => {
const mockSender = jest.fn();
storage.registerSender(mockSender);

storage.receive({ key1: "value1", key2: "value2" });
storage.receive({ key1: "value1", key2: "value2", "sso-client-http://bin": "value3" });
storage.clear();

expect(mockSender).toHaveBeenCalledWith("remove", ["key1", "key2"], undefined);
expect(mockSender).toHaveBeenCalledWith("remove", ["key1", "key2", "sso-client-http:~1~1bin"], undefined);
});

it("should get an item", () => {
const keys = ["key1", "!", "!", "pepr", "https://google.com", "sftp://here:22", "!"];
const results = ["value1", null, "!", "was-here", "3f7dd007-568f-4f4a-bbac-2e6bfff93860", "your-machine", " "];

keys.map((key, i) => {
const mockData: DataStore = { [v2StoreKey(key)]: results[i]! };
const mockData: DataStore = { [v2UnescapedStoreKey(key)]: results[i]! };

storage.receive(mockData);
const value = storage.getItem(keys[i]);
Expand Down
16 changes: 12 additions & 4 deletions src/lib/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ export function v2StoreKey(key: string) {
return `${STORE_VERSION_PREFIX}-${pointer.escape(key)}`;
}

export function v2UnescapedStoreKey(key: string) {
return `${STORE_VERSION_PREFIX}-${key}`;
}

export function stripV2Prefix(key: string) {
return key.replace(/^v2-/, "");
}
Expand Down Expand Up @@ -95,15 +99,18 @@ export class Storage implements PeprStore {
};

getItem = (key: string) => {
const result = this.#store[v2StoreKey(key)] || null;
const result = this.#store[v2UnescapedStoreKey(key)] || null;
if (result !== null && typeof result !== "function" && typeof result !== "object") {
return result;
}
return null;
};

clear = () => {
this.#dispatchUpdate("remove", Object.keys(this.#store));
this.#dispatchUpdate(
"remove",
Object.keys(this.#store).map(key => pointer.escape(key)),
);
};

removeItem = (key: string) => {
Expand All @@ -124,9 +131,10 @@ export class Storage implements PeprStore {
*/
setItemAndWait = (key: string, value: string) => {
this.#dispatchUpdate("add", [v2StoreKey(key)], value);

return new Promise<void>((resolve, reject) => {
const unsubscribe = this.subscribe(data => {
if (data[`${v2StoreKey(key)}`] === value) {
if (data[`${v2UnescapedStoreKey(key)}`] === value) {
unsubscribe();
resolve();
}
Expand All @@ -151,7 +159,7 @@ export class Storage implements PeprStore {
this.#dispatchUpdate("remove", [v2StoreKey(key)]);
return new Promise<void>((resolve, reject) => {
const unsubscribe = this.subscribe(data => {
if (!Object.hasOwn(data, `${v2StoreKey(key)}`)) {
if (!Object.hasOwn(data, `${v2UnescapedStoreKey(key)}`)) {
unsubscribe();
resolve();
}
Expand Down
37 changes: 26 additions & 11 deletions src/templates/capabilities/hello-pepr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,24 +268,39 @@ interface TheChuckNorrisJoke {
}

When(a.ConfigMap)
.IsCreated()
.IsCreatedOrUpdated()
.WithLabel("chuck-norris")
.Mutate(async change => {
.Mutate(cm => cm.SetLabel("got-jokes", "true"))
.Watch(async cm => {
const jokeURL = "https://icanhazdadjoke.com/";
// Try/catch is not needed as a response object will always be returned
const response = await fetch<TheChuckNorrisJoke>(
"https://icanhazdadjoke.com/",
{
headers: {
Accept: "application/json",
},
const response = await fetch<TheChuckNorrisJoke>(jokeURL, {
headers: {
Accept: "application/json",
},
);
});

// Instead, check the `response.ok` field
if (response.ok) {
const { joke } = response.data;
// Add Joke to the Store
await Store.setItemAndWait(jokeURL, joke);
// Add the Chuck Norris joke to the configmap
change.Raw.data["chuck-says"] = response.data.joke;
return;
try {
await K8s(kind.ConfigMap).Apply({
metadata: {
name: cm.metadata.name,
namespace: cm.metadata.namespace,
},
data: {
"chuck-says": Store.getItem(jokeURL),
},
});
} catch (error) {
Log.error(error, "Failed to apply ConfigMap using server-side apply.", {
cm,
});
}
}

// You can also assert on different HTTP response codes
Expand Down

0 comments on commit 74f5872

Please sign in to comment.