Secrets Code Gen #6385

gearama · 2025-01-30T01:28:42Z

Generate KV Secrets from tsp with the latest codegen and typespec
From a Secrets POV all is well we have a thing going on, code is generates, compiling and testing
on the innfra side work that needs to happen to streamlline it further

move client tsp and tsconfig info into the specs repo thus the configs come as part of the downloading of TSPs thus no more file copy. It is still very useful as part of local development and rapid turn around for fixes for example for names.
publish codegen to NPM so then we don't need all the magic git operations to get the generator, it would be obtained and invoke by the tsp-client command

Right now we are at parity with the previous API surface , there are changes to the models but the methods should be fine

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

…nto kv_tsp_secrets3

gearama · 2025-01-30T01:30:01Z

/azp run cpp - keyvault

azure-pipelines · 2025-01-30T01:30:52Z

Azure Pipelines successfully started running 1 pipeline(s).

gearama · 2025-01-30T01:40:16Z

/azp run cpp - keyvault

azure-pipelines · 2025-01-30T01:40:50Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-sdk · 2025-01-30T22:02:01Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-security-keyvault-secrets-cpp

LarryOsterman

My biggest comment is that there are a substantial number of breaking changes in this PR, and there are a surprising number of tests that don't appear to be carried forward into the new codebase.

What is the plan for managing these breaking changes?

samples/integration/vcpkg-all-smoke/src/main.cpp

sdk/keyvault/azure-security-keyvault-secrets/CMakeLists.txt

sdk/keyvault/azure-security-keyvault-secrets/test/ut/CMakeLists.txt

antkmsft

George, do we really want to move the header files from, say, <azure/keyvault/secrets.hpp> to <azure/security/keyvault/secrets.hpp>?
This won't align with the rest of the current keyvault libraries, and some of them are in GA state, so we probably won't do the same move for them? And then there will forever be #include <azure/keyvault and #include <azure/security/keyvault which is not great, so is it really worth it? I'd say, keep the include file location the same. If codegen needs some option to support this, let's implement that option in codegen.

gearama · 2025-03-05T19:50:25Z

i guess we could discuss it with Larry and Rick , to be hones i think we made a mistake originally by not including that path , which is in the namespace and the package name , so the headers should have been in azure/security/keyvault/ .... Let me talk with Larry and Rick and circle back when i have a clearer idea.

antkmsft · 2025-03-05T19:54:50Z

@gearama, I agree, possibly that was a mistake, but now, I think, we should keep it, because the alternative is not giving us much while making some problems.

antkmsft · 2025-03-07T01:49:55Z

sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/dll_import_export.hpp

-/**
- * @file
- * @brief DLL export macro.
- */


we should update codegen to include these comments (+comments in the all-inclusive header and dll_import_export as well)

Do these comments even show up in our published documentation?

Same comment applies to rtti.hpp - IMHO we should not be including these files in published documentation, so it only affects the comments. And in that case, I'd just throw in a link to a .md file which explains it in detail so we don't need to duplicate all this text.

antkmsft · 2025-03-07T01:51:05Z

sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets.hpp

-#include "azure/keyvault/secrets/dll_import_export.hpp"
-#include "azure/keyvault/secrets/keyvault_backup_secret.hpp"
-#include "azure/keyvault/secrets/keyvault_deleted_secret.hpp"
-#include "azure/keyvault/secrets/keyvault_operations.hpp"
-#include "azure/keyvault/secrets/keyvault_options.hpp"
-#include "azure/keyvault/secrets/keyvault_secret.hpp"
-#include "azure/keyvault/secrets/keyvault_secret_paged_response.hpp"
-#include "azure/keyvault/secrets/keyvault_secret_properties.hpp"
-#include "azure/keyvault/secrets/rtti.hpp"
-#include "azure/keyvault/secrets/secret_client.hpp"


Should we update codegen with the features to match the existing header files? It can be done... Diffing will certainly be easier, especially for doxygen comments.
Alternative: keep these headers, but include the new ones from them.

i don't think we should do that , we include the headers we generate, i'm not sure , we provide the root header file, if poeple decide to include things manually that's on them, we changed header file names before , so i'm not sure there's a point to trying to match underlying things as before.

antkmsft · 2025-03-07T01:52:05Z

sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/keyvault_operations.hpp

@@ -22,15 +21,15 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets {
  /**
   * @brief Represents a long running operation to restore a deleted secret.
   */
-  class RecoverDeletedSecretOperation final : public Azure::Core::Operation<SecretProperties> {
+  class RecoverDeletedSecretOperation final : public Azure::Core::Operation<KeyVaultSecret> {


Should we rename KeyVaultSecret to SecretProperties?

there is a secret properties already , plus things changes and props that were in the properties meodel moved into the secret and vice versa.

antkmsft · 2025-03-07T01:53:39Z

...zure-security-keyvault-secrets/samples/sample1-basic-operations/sample1_basic_operations.cpp

@@ -21,13 +21,12 @@ using namespace std::chrono_literals;

 int main()
 {
-  // @begin_snippet: SecretSample1CreateCredential


Why did we remove the snippet coments?

build issues, for some reason or another, it was saying it couldn't find them, and i couldn't figure out why that is . That script is a black hole that does not explain why something fails

...zure-security-keyvault-secrets/samples/sample1-basic-operations/sample1_basic_operations.cpp

antkmsft · 2025-03-07T02:07:20Z

sdk/keyvault/azure-security-keyvault-secrets/src/secret_client.cpp

-        std::make_unique<_internal::KeyVaultChallengeBasedAuthenticationPolicy>(
-            credential, tokenContext));


We should keep using this policy, not need to drop it just because codegen doesn't emit it.
We could add a code injection context for policies initialization (codegen: replace SecretClient policies initialization), or channel that through config (replaceAuthPolicy = {include: 'azure/keyvault/shared/keyvault_challenge_based_auth.hpp', statement = 'std::make_unique<_internal::KeyVaultChallengeBasedAuthenticationPolicy>(credential, tokenContext)'}), although code injection is probably a better solution, because it would allow for constructor overloads with different auth policy depending on parameters.

You can even do it today, with // codegen: insert after includes + // codegen: replace KeyClient::KeyClient, and maybe it is how it should be done, without implementing the codegen: replace SecretClient policies initialization - that is what that feature was done for, keeping this scenario in mind.

fi we want to add codegen support to do that maybe we should, but i'm not sure that is a secure way to auth, what we have now works. some architect or lead should give their opinion. My initial reaction is that we don't need it and it's not secure so we shouldn't have it.

Challenge-Based auth is not secure? You must be confusing it with something else.
What we have now works, but it won't work in gov clouds, for example. CHallenge-Based auth is like BearerTokenAuth v 1.1 - it is backwards compatible. We should not be dropping it. I think you should inject it by using //codegen: replace SecretClient::SecretClient in the .cpp file.

antkmsft · 2025-03-07T02:11:16Z

sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp

-     * @brief Set a secret in a specified key vault.
-     *
-     * @param name The name of the secret.
-     * @param secret The secret definition.
-     *
+     * @brief The SET operation adds a secret to the Azure Key Vault. If the named secret already
+     * exists, Azure Key Vault creates a new version of that secret. This operation requires the
+     * secrets/set permission.
+     * @param secretName The name of the secret. The value you provide may be copied globally for
+     * the purpose of running the service. The value provided should not include personally
+     * identifiable or sensitive information.
+     * @param secret The secret to set.


Are doxygen docs good?
The ones I've seen do look good, and not only good but in most cases better, but can we do something to review all of them in a diff, any ideas?
If we want to correct some of them, we can also add // codegen: replace SecretClient::SetSecret comment injection context.

i don't understand the question

Can we have a meeting like the ApiView one, but for the generated documentation?

antkmsft

I'm no longer "requesting changes" in this PR, but I don't know how will we proceed, like what level of review is expected, and how you want to approach it - do we check in first, and then hold some review for the docs and the ApiView, or do we fix all/most of the things before you merge this PR?

Most of the feedback I've left is not directly to you, but to be discussed within our team to make a decision, like we did this morning about the headers path and the Models namespace.

I guess I could set this to "Approved", and I see the reasons to not set this to Approved, depending on how we are going to interpret it for such big and fundamental PR.

not requesting anything specifically, not approving yet either

LarryOsterman · 2025-03-07T18:53:18Z

Generally I'm OK with the changes. But I'd love to see an APIView of the current changes - the one in apiview currently is from January and it's way out-of-date :(.

…asic-operations/sample1_basic_operations.cpp Co-authored-by: Anton Kolesnyk <[email protected]>

…k-for-cpp into kv_tsp_secrets3

gearama added 11 commits October 16, 2024 11:57

first stab at a client tsp /tspconfig scripts

24ef9cc

space

eda20b4

updates to clientTSP

862e76d

main merge

3c69061

Merge branch 'main' of https://github.com/gearama/azure-sdk-for-cpp i…

6207f56

…nto kv_tsp_secrets3

update for latest specs for KV

9db8d18

gfd

669f603

gfd

82417f1

hghf

2a7ff00

dfgdf

dd4b127

jhg

8fc0b67

github-actions bot added the KeyVault label Jan 30, 2025

oopsie

16b6c86

gearama added 7 commits January 29, 2025 17:50

comments

1be63ea

headers

8ff3a87

clang & param

d178705

coverage

36d9024

more comments not gened

c924fcf

spell

9203fd4

more comments

552e50b

gearama added 5 commits February 4, 2025 15:05

regen for testing

1b6aeb3

gfd

d0d1c87

gdf

fa8e70c

tretre

a449b36

update to use tsp client

07a4af7

gearama requested review from RickWinter and LarryOsterman as code owners March 4, 2025 23:44

gearama changed the title ~~Kv tsp secrets3~~ Secrets Code Gen Mar 4, 2025

gearama added 4 commits March 4, 2025 16:42

update to latest codegen

153ea18

gdfgdf

97b84a2

replace method

745e53d

client and stuffs

0c10eb8

LarryOsterman reviewed Mar 5, 2025

View reviewed changes

antkmsft previously requested changes Mar 5, 2025

View reviewed changes

gearama added 7 commits March 5, 2025 16:19

PR comments

e5c9579

remove security from path

a86e05d

clang

b2fb819

qfe

dae20a8

gdf

d2e31ce

update to models and to the lastest codegen

a8478d0

forgot cmake file

1281f89

gearama requested review from LarryOsterman and antkmsft March 7, 2025 00:31

antkmsft reviewed Mar 7, 2025

View reviewed changes

antkmsft self-requested a review March 7, 2025 02:12

antkmsft reviewed Mar 7, 2025

View reviewed changes

gearama and others added 5 commits March 7, 2025 12:47

Update sdk/keyvault/azure-security-keyvault-secrets/samples/sample1-b…

cae0a14

…asic-operations/sample1_basic_operations.cpp Co-authored-by: Anton Kolesnyk <[email protected]>

test api view

aaf0095

Merge branch 'kv_tsp_secrets3' of https://github.com/gearama/azure-sd…

e5f725b

…k-for-cpp into kv_tsp_secrets3

dfsfsd

02f049e

ccxcvx

02313c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secrets Code Gen #6385

Secrets Code Gen #6385

gearama commented Jan 30, 2025 •

edited

Loading

gearama commented Jan 30, 2025

azure-pipelines bot commented Jan 30, 2025

gearama commented Jan 30, 2025

azure-pipelines bot commented Jan 30, 2025

azure-sdk commented Jan 30, 2025 •

edited

Loading

LarryOsterman left a comment

antkmsft left a comment •

edited

Loading

gearama commented Mar 5, 2025

antkmsft commented Mar 5, 2025

antkmsft Mar 7, 2025

LarryOsterman Mar 7, 2025

LarryOsterman Mar 7, 2025

antkmsft Mar 7, 2025

gearama Mar 7, 2025

antkmsft Mar 7, 2025

gearama Mar 7, 2025

antkmsft Mar 7, 2025

gearama Mar 7, 2025 •

edited

Loading

antkmsft Mar 7, 2025

gearama Mar 7, 2025

antkmsft Mar 7, 2025

antkmsft Mar 7, 2025

gearama Mar 7, 2025

antkmsft Mar 7, 2025

antkmsft left a comment

LarryOsterman commented Mar 7, 2025

		std::make_unique<_internal::KeyVaultChallengeBasedAuthenticationPolicy>(
		credential, tokenContext));

Secrets Code Gen #6385

Are you sure you want to change the base?

Secrets Code Gen #6385

Conversation

gearama commented Jan 30, 2025 • edited Loading

Pull Request Checklist

gearama commented Jan 30, 2025

azure-pipelines bot commented Jan 30, 2025

gearama commented Jan 30, 2025

azure-pipelines bot commented Jan 30, 2025

azure-sdk commented Jan 30, 2025 • edited Loading

LarryOsterman left a comment

Choose a reason for hiding this comment

antkmsft left a comment • edited Loading

Choose a reason for hiding this comment

gearama commented Mar 5, 2025

antkmsft commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gearama Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antkmsft left a comment

Choose a reason for hiding this comment

LarryOsterman commented Mar 7, 2025

gearama commented Jan 30, 2025 •

edited

Loading

azure-sdk commented Jan 30, 2025 •

edited

Loading

antkmsft left a comment •

edited

Loading

gearama Mar 7, 2025 •

edited

Loading