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

Add ConfigurationProtectedSettings to extensions resource #3752

Merged

Conversation

matthchr
Copy link
Member

If applicable:

  • this PR contains documentation
  • this PR contains tests
  • this PR contains YAML Samples

@matthchr
Copy link
Member Author

/ok-to-test sha=7bbdfd2

@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (27e6a82) 53.54% compared to head (7bbdfd2) 53.53%.
Report is 1 commits behind head on main.

Files Patch % Lines
...configuration/v1api20230501/extension_types_gen.go 92.00% 1 Missing and 1 partial ⚠️
...2/internal/controllers/controller_resources_gen.go 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3752      +/-   ##
==========================================
- Coverage   53.54%   53.53%   -0.02%     
==========================================
  Files        1419     1419              
  Lines      482072   482115      +43     
==========================================
- Hits       258134   258093      -41     
- Misses     184917   184996      +79     
- Partials    39021    39026       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -53,6 +54,7 @@ github.com/Azure/azure-service-operator/v2/api/kubernetesconfiguration/v1api2023
│ │ └── "UserAssigned"
│ ├── AutoUpgradeMinorVersion: *bool
│ ├── Conditions: conditions.Condition[]
│ ├── ConfigurationProtectedSettings: map[string]string
Copy link
Member

Choose a reason for hiding this comment

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

Should this exist on the Status? Won't it reveal secrets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment in add_secrets.go, previous PR explains this:

// We only remove pure secret references here. For the case of secret maps, different services seem to treat them
// differently. Some services (such as Microsoft.KubernetesConfiguration/extensions) will return the keys of the map
// but not the values. Other services (such as APIM) will return certain keys and values that it knows are non-secret, but
// redact the ones that are secret. Since it's hard to know statically what will be returned for any given service, we
// default to having the map[string]string on the Status type and letting the service return what it wants.

Basically the problem here is that unlike pure string secrets, not every service has the same behavior. They all redact things but they all do it differently. It seems like there's much less of a standard here for questions like: "Are the keys secret too" or "is everything in this map secret or only some stuff" - so rather than try to guess at the services intent we just let them return what they want and display it.

The reasoning behind this (and feel free to disagree w/ this if you think it's wrong) is to let the service show us what it wants to show us. If the service is returning something on GET which is a secret that's a service bug and would impact more than just ASO (as SDKs, etc also always have these fields, we only remove them in ASO as a convenience because we know they'll always be empty in the case of single-string secrets).

@matthchr matthchr added this pull request to the merge queue Feb 1, 2024
Merged via the queue into Azure:main with commit 70c2840 Feb 1, 2024
8 checks passed
@matthchr matthchr deleted the feature/configuration-protected-settings branch February 1, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants