Skip to content

Create ADR for properties that are sometimes secret and sometimes not #5303

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.

Sign up for GitHub

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

Merged
Bevan Arps (theunrepentantgeek) merged 5 commits into from
Apr 14, 2026
Merged

Create ADR for properties that are sometimes secret and sometimes not #5303

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e2f184
Add new ADR
theunrepentantgeek Apr 9, 2026
6c83863
Update index
theunrepentantgeek Apr 9, 2026
9d38d74
Fix typo
theunrepentantgeek Apr 10, 2026
1b0c383
Update based on copilot review
theunrepentantgeek Apr 10, 2026
0ab86a3
Change name of new option
theunrepentantgeek Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 175 additions & 0 deletions docs/hugo/content/design/ADR-2026-04-Secrets/_index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
---
title: '2026-04: Optional Secrets'
toc_hide: true
Comment thread
theunrepentantgeek marked this conversation as resolved.
---

## Context

We have explicit configuration to control whether a property is considered a secret or not - we can specify `$isSecret`
as either `true` or `false`.

Specifying `$isSecret: true` has been required because the Azure REST API specs don't always call out which properties
are secrets; our config allows us to address omissions.

We've also used some heuristics to detect properties which might be secrets, requiring an explicit decision to be made.
Supporting `$isSecret: false` allows us to configure false positives in these cases.

To date, this has covered all of our needs - but we now have an interesting case (identified in
[#5095](https://github.com/Azure/azure-service-operator/issues/5095)) where a property is _sometimes_ a secret but not
Comment thread
matthchr marked this conversation as resolved.
always.

For some users, a webhook service URI is a plain text field with nothing sensitive.

But for others, the URI necessarily contains credentials (a password, api key, or similar) and they want to treat it as
a secret, pulling it from a secret at runtime.

## Requirements

To support this, we need more flexibility than a simple true/false answer to the question "is this property a secret?" -
we need the ability to handle a property which is an optional secret, i.e. it can be treated as a secret if the user
chooses to do so, but it doesn't have to be.

For properties already marked as secrets, we want to continue treating them as secrets, preserving the existing
generated CRD structure exactly as it is. No breaking changes.

We want to keep configuration, code generation, and CRD use as simple as possible, but are willing to accept some
complexity if it leads to a cleaner overall result, expecially if it's simpler for end users.

## Options

The problem falls into two natural parts:

* How do we represent this in our configuration?
* How do we transform the property in our code generator?

Comment thread
theunrepentantgeek marked this conversation as resolved.
## Configuration Option 1: Additional Property

Add new custom configuration for this situation, something mutually exclusive with `$isSecret`, say `$isOptionalSecret:
true/false`.

### Pros

* Minimal Change

### Cons

* Adds complexity to the configuration, and to the code which processes it.
* Doesn't really capture the idea that this is a single property with three states (secret, not secret, or optional
secret) - instead it has two properties which are mutually exclusive, which is a bit awkward.

## Configuration Option 2: Trivalued Boolean

Keep the existing true/false property but allow it to take a third value, say `optional`, to indicate that the property
is an optional secret.

### Pros

* Simpler configuration - just one property to indicate the secret status of a property, with three possible values.
* More directly captures the idea that this is a single property with three states.

### Cons

* More significant change to the configuration and code, as we need to support a new value for an existing property.
* Significantly less intuitive for users who are used to thinking of `$isSecret` as a boolean - they now need to
understand that it can take a non-boolean value.

## Configuration Option 3: Change to new Enum

Replace the existing `$isSecret` boolean with a new enum property, say `$secret` with values `always`, `never`, and
`optional`.

### Pros

* Cleanest solution - we have a new property that clearly indicates the three possible states without overloading the
meaning of an existing property.
* Avoids confusion for users who are used to thinking of `$isSecret` as a boolean, as we have a new property with a
clear name that indicates its purpose.
* Well defined in scope, potentially achievable with Copilot.

### Cons

* Most significant change to the configuration and code, as we need to replace an existing property with a new one and
update all references to it.
* Requires users to update their configuration to use the new property, which is a breaking change to any work currently
in progress.

## 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

@matthchr Matthew Christopher (matthchr) Apr 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@theunrepentantgeek Bevan Arps (theunrepentantgeek) Apr 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@theunrepentantgeek Bevan Arps (theunrepentantgeek) Apr 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't love the name $importConfigMapMode but going with $importSecretMode for consistency would be good. Changed.

@matthchr Matthew Christopher (matthchr) Apr 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TBH I'd be fine changing them both to something else too. Just feels like they should be consistent between them is all.

`optional`. Handle `$isSecret` as a legacy property for backward compatibility, mapping `true` to `always` and `false`
to `never`.

### Pros

* Provides a clear path forward with the new enum property while maintaining backward compatibility for existing
configurations.
* Allows users to transition to the new configuration at their own pace without breaking existing work.
* Clean solution that clearly indicates the three possible states without overloading the meaning of an existing
property
* Avoids confusion for users who are used to thinking of `$isSecret` as a boolean, as we have a new property with a
clear name that indicates its purpose.

### Cons

* Still requires a significant change to the configuration and code to introduce the new enum property and handle the
legacy `$isSecret` property.
* Adds some complexity to the code to support both the new and legacy properties, though this is mitigated by the clear
mapping between them.
* Users will need to update their configuration to use the new property eventually, which is a breaking change to any
work currently in progress, though they can continue using the old property in the meantime.

## Generation Option 1: Parallel Properties

As we've done for properties that can be optionally populated from ConfigMaps, we could have two properties in the
generated code - one for the plain text value and one for the secret reference. They'd be mutually exclusive, and the
user would populate one or the other.

For consistency with the way we've handled properties that can be optionally populated from ConfigMaps, we could use the
suffix `Secret` for the secret reference property

### Pros

* Consistent with existing patterns for optional ConfigMap properties.

### Cons

* Adds complexity to the generated code, as we now have two properties for each optional secret, and we need to ensure
they are mutually exclusive.

## Generation Option 2: Single Property with Union Type

We could have a single property in the generated code that can accept either a plain text value or a secret reference,
using a union type to allow for both possibilities.

### Pros

* Simplifies the generated code by having a single property for each optional secret.
* Reduces the risk of user error by eliminating the need to ensure mutual exclusivity between two properties.

### Cons

* Less consistent with existing patterns for optional ConfigMap properties.
* Contrary to Kubernetes conventions, which explicitly recommend avoiding union/polymorphic properties.


## Decision

Configuration Option 4: Change to new Enum, with backward compatibility.

@matthchr Matthew Christopher (matthchr) Apr 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@theunrepentantgeek Bevan Arps (theunrepentantgeek) Apr 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@matthchr Matthew Christopher (matthchr) Apr 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine with that too as long as the end result is the simpler variant (not both in perpetuity)


Generation Option 1: Parallel Properties.

## Status

Proposed.

## Consequences

TBC.

## Experience Report
Comment thread
theunrepentantgeek marked this conversation as resolved.

TBC.

## References

None.
28 changes: 16 additions & 12 deletions docs/hugo/content/design/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,27 @@ folder. ADRs are listed reverse chronologically by year. [See below](#format) fo

These ADRs reflect work that is being discussed, has been proposed, or is currently in progress.

### 2025

| Proposed | Title | Description | Status |
| :------: | --------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------- |
| 2025 | [**The Long Tail of Property Conversion**]({{< relref "ADR-2025-10-28-Property-Conversion-Long-Tail" >}}) | A design to address the increasingly rare edge cases that fall outside the capabilities of our current automatic conversion generator for inter-version transformation of resources. | Proposed |
| 2025 | [**Version Priority**]({{< relref "ADR-2025-05-Version-Priority" >}}) | A design to address the problems with version priority in Azure Service Operator, ensuring that the most recent resource version is selected by Kubernetes. | Implementation underway |
| 2025 | [**Improving ARM References**]({{< relref "ADR-2025-01-ARM-References" >}}) | Options for remedying the problems we face when we fail to identify a property as an ARM reference prior to releasing ASO. | Implementation underway |
| 2024 | [**Upstream Deletion**]({{< relref "ADR-2024-02-Upstream-Deletion" >}}) | When might deletion of an upstream resource occur and how will ASO handle it. | Open |
| 2023 | [**Resource Deprecation**]({{< relref "ADR-2023-04-Deprecation" >}}) | Understand the policy for handling Azure resource deprecation in Azure Service Operator, including the process for flagging deprecated resources, communicating changes, and removing unsupported resources. | Open |
| 2022 | [**Change Detection**]({{< relref "ADR-2022-11-Change-Detection" >}}) | The challenges of change detection in Azure Service Operator, discussing the limitations of relying on Azure Resource Manager for goal state comparison, the impact of ARM throttling, and a proposal for ASO to perform its own goal state comparison to mitigate these issues. | Revision in progress |
| Earlier | [**CrossPlane**]({{< relref "crossplane" >}}) | Discusses the intricacies of code generation for Crossplane, including static "special" properties, cross resource references, and the challenges associated with generating these references. | Open |
| Earlier | [**Improving the Reconciler interface**]({{< relref "reconcile-interface" >}}) | Explore a proposal for improving the Reconciler interface in controller-runtime, addressing issues of code duplication and potential bugs due to differences in checks for Create vs Delete operations. | Open |
| Proposed | Title | Description | Status |
| :------: | ------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------- |
| 2026 | [**Optional Secrets**]({{< relref "ADR-2026-04-Secrets" >}}) | A design to address the handling of optional secrets in Azure Service Operator, including configuration and generation options. | Proposed |
| 2025 | [**Version Priority**]({{< relref "ADR-2025-05-Version-Priority" >}}) | A design to address the problems with version priority in Azure Service Operator, ensuring that the most recent resource version is selected by Kubernetes. | Implementation underway |
| 2024 | [**Upstream Deletion**]({{< relref "ADR-2024-02-Upstream-Deletion" >}}) | When might deletion of an upstream resource occur and how will ASO handle it. | Open |
| 2023 | [**Resource Deprecation**]({{< relref "ADR-2023-04-Deprecation" >}}) | Understand the policy for handling Azure resource deprecation in Azure Service Operator, including the process for flagging deprecated resources, communicating changes, and removing unsupported resources. | Open |
| 2022 | [**Change Detection**]({{< relref "ADR-2022-11-Change-Detection" >}}) | The challenges of change detection in Azure Service Operator, discussing the limitations of relying on Azure Resource Manager for goal state comparison, the impact of ARM throttling, and a proposal for ASO to perform its own goal state comparison to mitigate these issues. | Revision in progress |
| Earlier | [**CrossPlane**]({{< relref "crossplane" >}}) | Discusses the intricacies of code generation for Crossplane, including static "special" properties, cross resource references, and the challenges associated with generating these references. | Open |
| Earlier | [**Improving the Reconciler interface**]({{< relref "reconcile-interface" >}}) | Explore a proposal for improving the Reconciler interface in controller-runtime, addressing issues of code duplication and potential bugs due to differences in checks for Create vs Delete operations. | Open |

## Completed Changes

These ADRs reflect work that has been completed, grouped by year of proposal.

### 2025

| Title | Description |
| --------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| [**The Long Tail of Property Conversion**]({{< relref "ADR-2025-10-28-Property-Conversion-Long-Tail" >}}) | A design to address the increasingly rare edge cases that fall outside the capabilities of our current automatic conversion generator for inter-version transformation of resources. |
| [**Improving ARM References**]({{< relref "ADR-2025-01-ARM-References" >}}) | Options for remedying the problems we face when we fail to identify a property as an ARM reference prior to releasing ASO. |

### 2024

| Title | Description |
Expand Down
Loading