Secrets design proposal

[matthchr]

This is related to #1471.

[codecov-commenter]

Codecov Report

Merging #1989 (d30f866) into main (03b5be6) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1989      +/-   ##
==========================================
+ Coverage   57.15%   57.18%   +0.03%     
==========================================
  Files         422      422              
  Lines       85734    85514     -220     
==========================================
- Hits        49002    48904      -98     
+ Misses      30452    30371      -81     
+ Partials     6280     6239      -41     

Impacted Files	Coverage Δ
...101/network_security_groups__spec_arm_types_gen.go	33.33% <0.00%> (-33.34%)	⬇️
...0501storage/flexible_servers_database_types_gen.go	2.50% <0.00%> (-7.85%)	⬇️
...pi20201201/virtual_machines__spec_arm_types_gen.go	33.33% <0.00%> (-6.67%)	⬇️
...210101previewstorage/namespaces_topic_types_gen.go	7.89% <0.00%> (-2.46%)	⬇️
...ternal/functions/one_of_json_unmarshal_function.go	77.35% <0.00%> (-1.89%)	⬇️
...t.eventgrid/v1alpha1api20200601/topic_types_gen.go	63.71% <0.00%> (-1.74%)	⬇️
...201101storage/virtual_network_gateway_types_gen.go	3.44% <0.00%> (-1.68%)	⬇️
...orage/sql_database_throughput_setting_types_gen.go	8.00% <0.00%> (-1.38%)	⬇️
...torage/flexible_servers_configuration_types_gen.go	10.34% <0.00%> (-1.29%)	⬇️
...lpha1api20201101storage/load_balancer_types_gen.go	9.52% <0.00%> (-0.83%)	⬇️
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03b5be6...d30f866. Read the comment docs.

[theunrepentantgeek]

Partial review, with comments made as I thought of things. It's a good read, lots of decisions to make though!

[theunrepentantgeek]

Automatically triggering purge on delete seems like a problem to me - intentionally defeating the feature that KeyVault added to prevent very common user disruption issues.

[matthchr]

The user has deleted the resource the secret corresponds to though - there's no resource left so the secret is meaningless.

Imagine the scenario where the user has a storage account and puts theirs keys into KeyVault. Then they delete their storage account. Does it make sense to leave those keys in KeyVault (and not purge them?). That just means that if you wanted to recreate the storage account you'd need to use a different secret name as the old one is taken for 90 days until it is purged.

The keys there have no value though because even if you recreate the exact same named storage account, it'll have different keys.

Still, the proposal here is to by default leave them there (and thus make customers use a different name when recreating), but they can optionally purge them, which is useful especially for cases like testing where you'll probably be creating and deleting similarly named resources and secrets regularly.

[sakthi-vetrivel]

Why do want to give users an option of not using managed identity? Are we just limited by resources supporting managed identity or is there customer value in being able to choose?

[matthchr]

There are "good reasons" to not use managed identity. For example we use service principal for our ASO test environment, because managed identity requires you to run in Azure, which is annoying when you could otherwise kick the tires/test outside of Azure.

There are also some resources that don't support managed identity, or more often don't support it for all scenarios.

but I think it's mostly that the service itself doesn't force you to use managed identity. If we for example forced all users of Azure storage to use managed identity, it would preclude users who just want to lift+shift their stuff into ASO and have it adopt their existing resources, but weren't using managed identity. Just like in Azure core, the users are free to choose what makes sense for them.

With that said, this preference for managed identity here means we shouldn't default to pulling secrets and we should probably (as best we can) document with managed identity first.

[sakthi-vetrivel]

Good point. Do we then create a KeyVault operator to essentially do the role of the secret store CSI driver, or is there anything else we can add?

[matthchr]

I don't know what the right story is here honestly. It kinda feels like, if you want keyvault for security reasons, then you by definition do not want your secrets in Kubernetes and so no Kubernetes based solution for storing (or creating) the secrets can work for you.

If you want KeyVault because Azure "makes" you for some case or another, then you might not care and an ASO KeyVault Key resource that takes a k8s Secret as input might make sense for that case. We'd need to be sure that case was common enough that we care about it though, and I am not 100% sure it is.

Note: This doesn't stop us from putting the secrets we get from Azure into KeyVault (which this proposal suggests support for). That's fine because we can bypass Kubernetes secrets entirely and then the user could use the KeyVault CSI driver to inject those same secrets that ASO created in KeyVault (via ListKeys on storage for example) into their pod. This means we would compose well with CSI driver.

This is also very similar to what I would imagine that the app connector (meeting we had yesterday) would do in Kubernetes (if it supported Kubernetes) - which is why I was saying we'd really need to understand what the use case for it is, because if it's just "we make secrets in KeyVault and then the user can inject them with CSI driver" ASO can (basically) already do that in a k8s native way without any need for a special Azure resource or anything.

[babbageclunk]

Pushing partial review in preparation for discussion.

[babbageclunk]

Maybe this should be a list with names rather than a map, for better compliance with the k8s guidelines? I guess we still have a lot of maps that we inherit from ARM, but it's probably worth not making it worse.

[matthchr]

My intent at least was that it is an object, not an open ended map.

[matthchr]

/ok-to-test sha=751a824

[matthchr]

/ok-to-test sha=130f792