Skip to content
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

chore: adr for store enhancement #933

Merged
merged 15 commits into from
Jul 9, 2024
Merged
89 changes: 89 additions & 0 deletions adr/0008-store-key-sanitization.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# 8: Store Key Sanitization

Date: 2024-07-02

## Status

Under Review

## Context

Issues were discovered when accepting arbitrary Keycloak client IDs as Store keys without sanitization. For instance, IDs like `https://google.com/` failed during `Store.setItem` due to the `/` character being interpreted as a path separator, resulting in a `422` error.


## Decision

Sanitize the keys by replacing `/` with [a character that does not break the `patch` operation](https://datatracker.ietf.org/doc/html/rfc6902/#section-4) before setting, getting, or deleting a key in/from the store. We will also increase the tests to find other edge cases before releasing the change and deciding upon the exact character or pattern. We are prioritizing the cluster operator persona in this case who wants to quickly check all keys and values in the store by looking at the PeprStore CR.
cmwylie19 marked this conversation as resolved.
Show resolved Hide resolved

#### Sanitize using String Replacement
* Sanitize the key by replacing `/` with a character that does not break the `patch` operation before getting, setting, or deleting a key.
* Increasing the tests to find other edge cases before releasing the change.
cmwylie19 marked this conversation as resolved.
Show resolved Hide resolved

#### Why not use string replacement?

* There is a small risk of that there will be some edge case that we have not discovered yet of a key that cannot be placed in the store

## Alternatives

#### Sanitize using Base64 Encoding
* Base64 encode the key after receiving it from the user and before setting it into the Store.
* Update the Store key prefix with v2: `/data/${capabilityName}-v2-${key}`
* Migrate before calling `this.#onReady()` in the `#receive` function of `src/lib/controller/store.ts`, checking if each key that matches the old prefix, if so, migrating to the new prefix with base64 encoding.
* Add a new pepr command for viewing store items. like `npx pepr view-store`, maybe a `kubectl` plugin or alias too.
* Enhance existing unit testing to cover the new base64 encoding and migration logic. Add a battery of new fuzz and property-based tests.

Example of the Store Resource before and after migration:

Before:
```yaml
apiVersion: pepr.dev/v1
data:
__pepr_do_not_delete__: k-thx-bye
hello-pepr-watch-data: This data was stored by a Watch Action.
kind: PeprStore
metadata:
name: pepr-static-test-store
namespace: pepr-system
```

After:
```yaml
apiVersion: pepr.dev/v1
data:
__pepr_do_not_delete__: k-thx-bye
hello-pepr-v2-d2F0Y2gtZGF0YQo=: This data was stored by a Watch Action.
kind: PeprStore
metadata:
name: pepr-static-test-store
namespace: pepr-system
```
#### Why not use base64 encoding?

* Although base64 encoding gaurantees that the key will be unique and not contain any special characters, it will introduce a lot of overhead:
* * Harder for users to read PeprStore CR because key names will be encoded
* * Require a migration path for existing store items
* * Require a npx pepr command for viewing all store items at once?
* * Require a new store path prefix.
* * Potentially would want a k9s command for cluster users to be able to view PeprStore CR
* * A lot must be done for a relatively simple problem that is based on the issue and much is lost in terms of interacting with the PeprStore CR.


## Pros and Cons of the Decision

### Pros
- No migration path required because keys that cannot get placed in the store are not in the store
- No overhead of base64 encoding/decoding
- No new store path prefix
- No new pepr command for viewing store items because users can see the PeprStore CR as always
- Add more unit and e2e tests around the store
- No need for a k9s command for k9s users to be able to view PeprStore CR

### Cons
- Could be edge cases that we have not discovered yet of a key that you cannot place in the store

## Consequences

1. Implement the sanitization feature so that the store can accept arbitrary keycloak client ids
2. Add fuzz and property-based tests to find edge cases
3. Add more e2e tests around the store

Loading