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

chore: adr for store enhancement #933

merged 15 commits into from
Jul 9, 2024

Conversation

cmwylie19
Copy link
Collaborator

@cmwylie19 cmwylie19 commented Jul 2, 2024

Description

Extends existing Store functionality to v2 to enable storing keys like URLs, and provides a seamless hand-off migration path for users while introducing additional tooling to view the store resource.

  • How to enable the store to set special characters as keys
  • Address how to view the Store CR key/values in a user friendly way
  • Hands off end user migration path is decided on
  • Stakeholder feedback
  • UDS Core processes UDSExcemptions on startup so we want to leave that process unbothered (We think this should probably be fine but need to test. Other solutions pre-starup would involve initContainer). If this were a problem i think it could be solved by moving startExemptionWatch to the Store.onReady callback. IMO this checkbox is part of stakeholder feedback.

Also, i don't think this Store change will affect the startExemptionWatch function bc they are not using the actual Pepr Store in that instance but populating a map[string]interface{} type of store object.

Related Issue

Fixes #924

Relates to #918

Type of change

  • 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

Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
@cmwylie19 cmwylie19 changed the title chore: first pass at ADR for Store enhancement chore: ADR for Store enhancement Jul 2, 2024
@cmwylie19 cmwylie19 changed the title chore: ADR for Store enhancement chore: adr for Store enhancement Jul 2, 2024
@cmwylie19 cmwylie19 changed the title chore: adr for Store enhancement chore: adr for store enhancement Jul 2, 2024
Copy link
Collaborator

@btlghrants btlghrants left a comment

Choose a reason for hiding this comment

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

Overall, not a bad start -- and I've added some suggested refinements -- but... it reads more like a plan of action than an ADR, honestly -- it describes an issue and then just hops straight into a list of TODOs supporting a specific, apparently pre-decided implementation.

Where's the reasoning / alternatives? Will a future reader (or a soon-to-be reviewer) understand why we decided to solve the issue in the specific way we did?

For example:

  • Why did we decided to code this up instead of just dumping something into the docs saying "Here's the rules!"?
  • Why Base64 encode? Why not really sanitize the input by stripping out "bad" characters?
  • Why did you decide to put a "v2" in every key instead of having a v2 "container" injected in to keep the new / old keys separate (or whatever else)?
  • Why is it worth it to us to add a new Pepr command to the CLI when we could just tell user's it's base64'd and leave it up to them to decide how to read it?

...and so on. Happy to chat through any of that if you wanna, whenever.

adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
cmwylie19 and others added 5 commits July 2, 2024 15:29
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
@cmwylie19
Copy link
Collaborator Author

Overall, not a bad start -- and I've added some suggested refinements -- but... it reads more like a plan of action than an ADR, honestly -- it describes an issue and then just hops straight into a list of TODOs supporting a specific, apparently pre-decided implementation.

Where's the reasoning / alternatives? Will a future reader (or a soon-to-be reviewer) understand why we decided to solve the issue in the specific way we did?

For example:

  • Why did we decided to code this up instead of just dumping something into the docs saying "Here's the rules!"?
  • Why Base64 encode? Why not really sanitize the input by stripping out "bad" characters?
  • Why did you decide to put a "v2" in every key instead of having a v2 "container" injected in to keep the new / old keys separate (or whatever else)?
  • Why is it worth it to us to add a new Pepr command to the CLI when we could just tell user's it's base64'd and leave it up to them to decide how to read it?

...and so on. Happy to chat through any of that if you wanna, whenever.

Sure, will add these fields. It does feel a little more like a KEP than an ADR

Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Copy link
Collaborator

@btlghrants btlghrants left a comment

Choose a reason for hiding this comment

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

Still working it through, but these should be reasonable comments for the short-term.

adr/0008-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/0008-store-key-sanitization.md Outdated Show resolved Hide resolved
cmwylie19 and others added 3 commits July 8, 2024 13:07
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com>
@btlghrants btlghrants merged commit de7f0ce into main Jul 9, 2024
13 checks passed
@btlghrants btlghrants deleted the 924 branch July 9, 2024 17:20
mjnagel pushed a commit to defenseunicorns/uds-core that referenced this pull request Jul 17, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change | Age | Adoption | Passing |
Confidence |
|---|---|---|---|---|---|---|---|
|
[defenseunicorns/uds-common](https://togithub.com/defenseunicorns/uds-common)
| | minor | `v0.7.1` -> `v0.8.0` |
[![age](https://developer.mend.io/api/mc/badges/age/github-tags/defenseunicorns%2fuds-common/v0.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/github-tags/defenseunicorns%2fuds-common/v0.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/github-tags/defenseunicorns%2fuds-common/v0.7.1/v0.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/github-tags/defenseunicorns%2fuds-common/v0.7.1/v0.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
| [pepr](https://togithub.com/defenseunicorns/pepr) | dependencies |
patch | [`0.32.6` ->
`0.32.7`](https://renovatebot.com/diffs/npm/pepr/0.32.6/0.32.7) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/pepr/0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/pepr/0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/pepr/0.32.6/0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/pepr/0.32.6/0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
|
[registry1.dso.mil/ironbank/opensource/defenseunicorns/pepr/controller](https://togithub.com/defenseunicorns/pepr)
([source](https://repo1.dso.mil/dsop/opensource/defenseunicorns/pepr/controller))
| | patch | `v0.32.6` -> `v0.32.7` |
[![age](https://developer.mend.io/api/mc/badges/age/docker/registry1.dso.mil%2fironbank%2fopensource%2fdefenseunicorns%2fpepr%2fcontroller/v0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/docker/registry1.dso.mil%2fironbank%2fopensource%2fdefenseunicorns%2fpepr%2fcontroller/v0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/docker/registry1.dso.mil%2fironbank%2fopensource%2fdefenseunicorns%2fpepr%2fcontroller/v0.32.6/v0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/docker/registry1.dso.mil%2fironbank%2fopensource%2fdefenseunicorns%2fpepr%2fcontroller/v0.32.6/v0.32.7?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
| [ts-jest](https://kulshekhar.github.io/ts-jest)
([source](https://togithub.com/kulshekhar/ts-jest)) | devDependencies |
patch | [`29.2.0` ->
`29.2.2`](https://renovatebot.com/diffs/npm/ts-jest/29.2.0/29.2.2) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/ts-jest/29.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/ts-jest/29.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/ts-jest/29.2.0/29.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/ts-jest/29.2.0/29.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>defenseunicorns/uds-common
(defenseunicorns/uds-common)</summary>

###
[`v0.8.0`](https://togithub.com/defenseunicorns/uds-common/releases/tag/v0.8.0)

[Compare
Source](https://togithub.com/defenseunicorns/uds-common/compare/v0.7.1...v0.8.0)

##### Features

- **compliance:** add support for extra options on compliance validate
([#&#8203;170](https://togithub.com/defenseunicorns/uds-common/issues/170))
([d191505](https://togithub.com/defenseunicorns/uds-common/commit/d19150566784e51f7c8d31b7d37b6915cdacc410))

##### Bug Fixes

- chainguard creds/renovate match
([#&#8203;173](https://togithub.com/defenseunicorns/uds-common/issues/173))
([49401cc](https://togithub.com/defenseunicorns/uds-common/commit/49401cc5c8000a661c6e1bc9e10e42fa6f6e2389))

##### Miscellaneous

- add cgr.dev renovate rule
([#&#8203;171](https://togithub.com/defenseunicorns/uds-common/issues/171))
([68497f9](https://togithub.com/defenseunicorns/uds-common/commit/68497f95ffdccf5802da81f2f0c9a8f7f8fe912c))
- **deps:** update uds common support dependencies
([#&#8203;164](https://togithub.com/defenseunicorns/uds-common/issues/164))
([6c50f47](https://togithub.com/defenseunicorns/uds-common/commit/6c50f47ecd9c75483ab70953d5c31682362377c2))
- **deps:** update uds common support dependencies
([#&#8203;169](https://togithub.com/defenseunicorns/uds-common/issues/169))
([b6a4232](https://togithub.com/defenseunicorns/uds-common/commit/b6a4232cb030f3ea7e66041306b5cfcd9a488a98))
- update CODEOWNERS with more specific permissions
([#&#8203;175](https://togithub.com/defenseunicorns/uds-common/issues/175))
([f2b7220](https://togithub.com/defenseunicorns/uds-common/commit/f2b722051014d64d350bd34ea087e6ffb3daf428))

</details>

<details>
<summary>defenseunicorns/pepr (pepr)</summary>

###
[`v0.32.7`](https://togithub.com/defenseunicorns/pepr/releases/tag/v0.32.7)

[Compare
Source](https://togithub.com/defenseunicorns/pepr/compare/v0.32.6...v0.32.7)

Preparing for signed releases next release. These are mostly just
patches.

#### What's Changed

- chore: adr for store enhancement by
[@&#8203;cmwylie19](https://togithub.com/cmwylie19) in
[defenseunicorns/pepr#933
- chore: bump actions/download-artifact from 4.1.7 to 4.1.8 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#937
- chore: bump docker/setup-buildx-action from 3.3.0 to 3.4.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#934
- chore: bump chainguard/node-lts from `437a945` to `6d9e76d` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#935
- chore: bump actions/upload-artifact from 4.3.3 to 4.3.4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#936
- chore: bump ts-jest from 29.1.5 to 29.2.0 in the
development-dependencies group by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#941
- chore: bump chainguard/node-lts from `6d9e76d` to `afddf0f` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#939
- chore: bump [@&#8203;types/ramda](https://togithub.com/types/ramda)
from 0.30.0 to 0.30.1 in the production-dependencies group by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#940
- chore: bump chainguard/node-lts from `afddf0f` to `691fdeb` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#943
- chore: bump actions/setup-node from 4.0.2 to 4.0.3 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#942
- chore: bump chainguard/node-lts from `691fdeb` to `ea8ec8f` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#949
- chore: bump anchore/scan-action from 3.6.4 to 4.0.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#947
- chore: bump ts-jest from 29.2.0 to 29.2.2 in the
development-dependencies group by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#948
- chore: bump kubernetes-fluent-client from 2.6.3 to 2.6.4 in the
production-dependencies group by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[defenseunicorns/pepr#950

**Full Changelog**:
defenseunicorns/pepr@v0.32.6...v0.32.7

</details>

<details>
<summary>kulshekhar/ts-jest (ts-jest)</summary>

###
[`v29.2.2`](https://togithub.com/kulshekhar/ts-jest/blob/HEAD/CHANGELOG.md#2922-2024-07-10)

[Compare
Source](https://togithub.com/kulshekhar/ts-jest/compare/v29.2.1...v29.2.2)

###
[`v29.2.1`](https://togithub.com/kulshekhar/ts-jest/blob/HEAD/CHANGELOG.md#2921-2024-07-10)

[Compare
Source](https://togithub.com/kulshekhar/ts-jest/compare/v29.2.0...v29.2.1)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/defenseunicorns/uds-core).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjUuMSIsInVwZGF0ZWRJblZlciI6IjM3LjQzMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@cmwylie19 cmwylie19 self-assigned this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Store Enhancement Planning ADR
2 participants