Create ADR for properties that are sometimes secret and sometimes not#5303
Create ADR for properties that are sometimes secret and sometimes not#5303theunrepentantgeek merged 5 commits intomainfrom
Conversation
github-project-automation
Bot
moved this to In Progress
in Azure Service Operator Roadmap
There was a problem hiding this comment.
Pull request overview
Adds a new Architecture Decision Record (ADR) describing how ASO should handle properties that are sometimes sensitive and sometimes not (optional secrets), and updates the design index to list the new ADR and reorganize entries.
Changes:
- Add a new ADR describing configuration/generation options and the selected approach for optional secrets.
- Update the design index to include the new ADR and move some prior items into the “Completed Changes” section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/hugo/content/design/ADR-2026-04-Secrets/_index.html | New ADR documenting the problem space, options, and decision for “optional secrets”. |
| docs/hugo/content/design/_index.md | Updates the ADR listing to include the new “Optional Secrets” ADR and reorganize prior entries. |
matthchr
left a comment
matthchr
left a comment
There was a problem hiding this comment.
Approved, though I have notes about specifics (see comments)
|
|
||
| ## Decision | ||
|
|
||
| Configuration Option 4: Change to new Enum, with backward compatibility. |
There was a problem hiding this comment.
I'm not sold that we really need the back-compat complexity. It seems like it might be easier to just make a one-shot change? The number of outstanding PRs that would be impacted is not huge, and probably it's mostly us. I'd honestly rather do a bit more work now to ensure that we have a limited/more understandable number of options in azure-arm.yaml at the end of the day.
There was a problem hiding this comment.
I'm only thinking of having back-compat for the very short term - removing it before v2.20 is released, and mostly to avoid us having to rework PRs that are otherwise completed. It also provides a window for external PRs to pop up without problems.
There was a problem hiding this comment.
Fine with that too as long as the end result is the simpler variant (not both in perpetuity)
|
|
||
| ## Configuration Option 4: Change to new Enum, with backward compatibility | ||
|
|
||
| Replace the existing `$isSecret` boolean with a new enum property, say `$secret` with values `always`, `never`, and |
There was a problem hiding this comment.
Should we just match importConfigMapMode?
(From our azure-arm.yaml):
# $importConfigMapMode: <optional|required>
# Specifies that the property can be imported from a config map.
# Optional: The property may be specified as string or imported from a config map.
# To achieve this in a non-breaking way, a new property is added to the object living alongside
# the existing property. The new property is called <propName>FromConfig.
# Required: The property must be specified from a config map, it cannot be given as a raw string.
Maybe with an added never if you want the 3 states explicitly (I think w/ importConfigMapMode we treat the absence of the field as an implied 3rd state == never. I don't love that but am fine with it if we wanted to do it for this secret field too)
Mostly I think the names and values should match as much as possible for consistency.
There was a problem hiding this comment.
We have a bunch of examples of $isSecret: false - used to turn off our heuristics, so we will definitely need never as a possible value. ConfigMaps don't have heuristics popping up, so they don't need the third value.
There was a problem hiding this comment.
I don't love the name $importConfigMapMode but going with $importSecretMode for consistency would be good. Changed.
There was a problem hiding this comment.
TBH I'd be fine changing them both to something else too. Just feels like they should be consistent between them is all.
github-project-automation
Bot
moved this from In Progress
to Recently Completed
in Azure Service Operator Roadmap
matthchr
moved this from Recently Completed
to Ready for Release
in Azure Service Operator Roadmap
What this PR does
Adds a design document discussing the options for handling properties that might sometimes be sensitive, but mostly are not.
How does this PR make you feel?
Checklist