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

Move tests to use azure pipeline credentials #5754

Merged
merged 49 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
b5bb953
test1
gearama Jul 1, 2024
e4c3ea2
hgdfchg
gearama Jul 1, 2024
2c9810f
remove the remnants of azure client secret
gearama Jul 1, 2024
023df7b
Merge branch 'main' of https://github.com/gearama/azure-sdk-for-cpp i…
gearama Jul 1, 2024
2bd4ad2
test KV with federated auth
gearama Jul 2, 2024
e916110
UseFederatedAuth
gearama Jul 2, 2024
c429beb
fdsa
gearama Jul 2, 2024
718edf4
kv template with managed
gearama Jul 2, 2024
6c1d77f
try try again
gearama Jul 2, 2024
7bff790
retry permissions
gearama Jul 2, 2024
47f4dbc
add net acls
gearama Jul 2, 2024
896bcaf
blunt force replace the resource json
gearama Jul 2, 2024
a096bc2
put back stuff
gearama Jul 3, 2024
64aab50
trey again with new method
gearama Jul 3, 2024
7536282
attempt
gearama Jul 3, 2024
70cc098
missed something
gearama Jul 3, 2024
bd16918
flip if else
gearama Jul 3, 2024
8a2b5e7
Merge branch 'Azure:main' into testCreds
gearama Jul 3, 2024
8d07d30
Temporarily use empty sub config file path for preview cloud
danieljurek Jul 3, 2024
14ea9cb
remove client secret
gearama Jul 3, 2024
32a831f
Merge branch 'testCreds' of https://github.com/gearama/azure-sdk-for-…
gearama Jul 3, 2024
40036c4
try to fix the identity tests
gearama Jul 3, 2024
76745bb
live skip failing tests and return in samples
gearama Jul 3, 2024
757efd6
samples for identity fix
gearama Jul 3, 2024
90504d5
disable failing samples in identity
gearama Jul 3, 2024
95d26f4
fix winhttp failing test
gearama Jul 3, 2024
1d6fa4c
comment out code
gearama Jul 4, 2024
a90e896
remove managed identity
gearama Jul 5, 2024
fb17a28
restore version from main
gearama Jul 5, 2024
89d9bcf
revert readme changes
gearama Jul 5, 2024
03e5338
PR comments
gearama Jul 8, 2024
f1f7b82
test 2
gearama Jul 8, 2024
5e686e2
clang
gearama Jul 8, 2024
a06f20c
attempt default creds with pipeline chanined
gearama Jul 8, 2024
737e344
clangs
gearama Jul 8, 2024
5aa9d9a
identity test and clangs
gearama Jul 8, 2024
a499b63
oops
gearama Jul 8, 2024
142ed8d
live
gearama Jul 9, 2024
462c5bc
cleanup
gearama Jul 9, 2024
c4f6681
reter
gearama Jul 9, 2024
2509a54
test
gearama Jul 9, 2024
2b31613
revert the DAC change
gearama Jul 9, 2024
a78674d
missed one
gearama Jul 9, 2024
2e2d86c
taking the samples to a farm upstate
gearama Jul 9, 2024
56dc0e8
PR comments
gearama Jul 10, 2024
80ed23c
Merge branch 'main' of https://github.com/gearama/azure-sdk-for-cpp i…
gearama Jul 10, 2024
13d3170
Merge remote-tracking branch 'upstream/main' into HEAD
antkmsft Jul 11, 2024
5d502b8
Merge branch 'main' into testCreds
antkmsft Jul 12, 2024
f717e6f
Fix bad merge
antkmsft Jul 12, 2024
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
52 changes: 26 additions & 26 deletions eng/pipelines/templates/jobs/live.tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -244,32 +244,6 @@ jobs:
# Will run samples described on a file name [service]-samples.txt within the build directory.
# For example keyvault-samples.txt.
# The file is written by CMake during configuration when building samples.
- bash: |
IFS=$'\n'
if [[ -f "./${{ parameters.ServiceDirectory }}-samples.txt" ]]; then
for sample in `cat ./${{ parameters.ServiceDirectory }}-samples.txt`
do
export AZURE_CLIENT_ID=$(${{parameters.ServiceDirectory}}_CLIENT_ID)
export AZURE_TENANT_ID=$(${{parameters.ServiceDirectory}}_TENANT_ID)
Comment on lines -247 to -253
Copy link
Member

Choose a reason for hiding this comment

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

What's motivating this change to move this as an else?

Copy link
Member Author

Choose a reason for hiding this comment

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

the if statement is IF use AzrurePipelineCredentials define these ENVs , ti was upside down

Copy link
Member

Choose a reason for hiding this comment

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

@weshaggard since Daniel is OOF, can you please help review the changes in this live.tests.yml file. I think it could benefit from an extra set of eyes to make sure it is as expected :)

Copy link
Member Author

Choose a reason for hiding this comment

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

it was very easy to check , before my change the build the live tests were failing, after it was working
and if you read the line right above it you see the if statement

Copy link
Member

Choose a reason for hiding this comment

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

It looks correct assuming you want to continue to run the samples via default credentials instead of with pipeline credentials.

Copy link
Member Author

Choose a reason for hiding this comment

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

Daniel was aware of the change, i talked to him before making it

Copy link
Member Author

Choose a reason for hiding this comment

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

[7/3 10:01 AM] George Arama
from what i see in the live.tests.yml the

  • ${{ if parameters.UseFederatedAuth }}:
    .....
    -${{ else }}:
    ..
    $env:AZURESUBSCRIPTION_CLIENT_ID = $account.Id

$env:AZURESUBSCRIPTION_TENANT_ID = $account.Tenants

...

the variables get defined when not using federated auth on the False branch

[7/3 10:24 AM] George Arama
in the live tests yaml the if/else branches for the run samples are flipped , i'm testing my fix for it

[7/3 11:13 AM] George Arama
looks like ti's working now as expected

export AZURE_CLIENT_SECRET=$(${{parameters.ServiceDirectory}}_CLIENT_SECRET)
echo "**********Running sample: ${sample}"
bash -c "$sample"
status=$?
if [[ $status -eq 0 ]]; then
echo "*********Sample completed*********"
else
echo "*Sample returned a failed code: $status"
exit 1
fi
done
fi
workingDirectory: build
displayName: "Run Samples for : ${{ parameters.ServiceDirectory }}"
condition: and(succeeded(), eq(variables['RunSamples'], '1'))
env:
${{ insert }}: ${{ parameters.EnvVars }}

- ${{ else }}:
- task: AzurePowerShell@5
displayName: "Run Samples for : ${{ parameters.ServiceDirectory }}"
condition: and(succeeded(), eq(variables['RunSamples'], '1'))
Expand Down Expand Up @@ -299,6 +273,32 @@ jobs:
SYSTEM_ACCESSTOKEN: $(System.AccessToken)
${{ insert }}: ${{ parameters.EnvVars }}

- ${{ else }}:
- bash: |
IFS=$'\n'
if [[ -f "./${{ parameters.ServiceDirectory }}-samples.txt" ]]; then
for sample in `cat ./${{ parameters.ServiceDirectory }}-samples.txt`
do
export AZURE_CLIENT_ID=$(${{parameters.ServiceDirectory}}_CLIENT_ID)
export AZURE_TENANT_ID=$(${{parameters.ServiceDirectory}}_TENANT_ID)
export AZURE_CLIENT_SECRET=$(${{parameters.ServiceDirectory}}_CLIENT_SECRET)
echo "**********Running sample: ${sample}"
bash -c "$sample"
status=$?
if [[ $status -eq 0 ]]; then
echo "*********Sample completed*********"
else
echo "*Sample returned a failed code: $status"
exit 1
fi
done
fi
workingDirectory: build
displayName: "Run Samples for : ${{ parameters.ServiceDirectory }}"
condition: and(succeeded(), eq(variables['RunSamples'], '1'))
env:
${{ insert }}: ${{ parameters.EnvVars }}

# Make coverage targets (specified in coverage_targets.txt) and assemble
# coverage report
- bash: |
Expand Down
2 changes: 2 additions & 0 deletions eng/pipelines/templates/stages/archetype-sdk-client.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ parameters:
Preview:
SubscriptionConfiguration: $(sub-config-azure-cloud-test-resources-preview)
ServiceConnection: azure-sdk-tests
# Temporary fix until an eng/common config for Preview can be merged
SubscriptionConfigurationFilePaths: []
gearama marked this conversation as resolved.
Show resolved Hide resolved
Canary:
SubscriptionConfiguration: $(sub-config-azure-cloud-test-resources)
ServiceConnection: azure-sdk-tests
Expand Down
1 change: 1 addition & 0 deletions sdk/attestation/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ extends:
LiveTestCtestRegex: azure-security-attestation.*
LineCoverageTarget: 70
BranchCoverageTarget: 34
UseFederatedAuth: true
gearama marked this conversation as resolved.
Show resolved Hide resolved
Artifacts:
- Name: azure-security-attestation
Path: azure-security-attestation
Expand Down
16 changes: 14 additions & 2 deletions sdk/core/azure-core-test/inc/azure/core/test/test_base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <azure/core/internal/client_options.hpp>
#include <azure/core/internal/diagnostics/log.hpp>
#include <azure/core/internal/environment.hpp>
#include <azure/identity/azure_pipelines_credential.hpp>
#include <azure/identity/chained_token_credential.hpp>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>

Expand Down Expand Up @@ -246,7 +248,17 @@ namespace Azure { namespace Core { namespace Test {
}
if (clientSecret.empty())
{
m_testCredential = std::make_shared<Azure::Identity::DefaultAzureCredential>();
m_testCredential = std::make_shared<Azure::Identity::ChainedTokenCredential>(
Azure::Identity::ChainedTokenCredential::Sources{
std ::make_shared<Azure::Identity::AzurePipelinesCredential>(
Azure::Core::_internal::Environment::GetVariable(
"AZURESUBSCRIPTION_TENANT_ID"),
Azure::Core::_internal::Environment::GetVariable(
"AZURESUBSCRIPTION_CLIENT_ID"),
Azure::Core::_internal::Environment::GetVariable(
"AZURESUBSCRIPTION_SERVICE_CONNECTION_ID"),
Azure::Core::_internal::Environment::GetVariable("SYSTEM_ACCESSTOKEN")),
std::make_shared<Azure::Identity::DefaultAzureCredential>()});
}
else
{
Expand Down Expand Up @@ -302,7 +314,7 @@ namespace Azure { namespace Core { namespace Test {
*
* @return The value of the environment variable retrieved.
*
* @note If AZURE_TENANT_ID, AZURE_CLIENT_ID, or AZURE_CLIENT_SECRET are not available in the
* @note If AZURE_TENANT_ID or AZURE_CLIENT_ID are not available in the
* environment, the AZURE_SERVICE_DIRECTORY environment variable is used to set those values
* with the values emitted by the New-TestResources.ps1 script.
*
Expand Down
1 change: 1 addition & 0 deletions sdk/core/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ extends:
LiveTestTimeoutInMinutes: 90 # default is 60 min. We need a little longer on worst case for Win+jsonTests
LineCoverageTarget: 88
BranchCoverageTarget: 50
UseFederatedAuth: true
# PreTestSteps:
# - pwsh: |
# docker build -t squid-local $(Build.SourcesDirectory)/sdk/core/azure-core/test/ut/proxy_tests/localproxy
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/perf/inc/azure/perf/base_test.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ namespace Azure { namespace Perf {
*
* @return The value of the environment variable retrieved.
*
* @note If AZURE_TENANT_ID, AZURE_CLIENT_ID, or AZURE_CLIENT_SECRET are not available in the
* @note If AZURE_TENANT_ID or AZURE_CLIENT_ID are not available in the
* environment, the AZURE_SERVICE_DIRECTORY environment variable is used to set those values
* with the values emitted by the New-TestResources.ps1 script.
*
Expand Down
12 changes: 11 additions & 1 deletion sdk/core/perf/src/base_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#endif
#include <azure/core/http/policies/policy.hpp>
#include <azure/core/internal/http/pipeline.hpp>
#include <azure/identity/azure_pipelines_credential.hpp>
#include <azure/identity/chained_token_credential.hpp>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>

Expand Down Expand Up @@ -285,7 +287,15 @@ namespace Azure { namespace Perf {
}
if (clientSecret.empty())
{
m_testCredential = std::make_shared<Azure::Identity::DefaultAzureCredential>();
m_testCredential = std::make_shared<Azure::Identity::ChainedTokenCredential>(
Azure::Identity::ChainedTokenCredential::Sources{
std ::make_shared<Azure::Identity::AzurePipelinesCredential>(
Azure::Core::_internal::Environment::GetVariable("AZURESUBSCRIPTION_TENANT_ID"),
Azure::Core::_internal::Environment::GetVariable("AZURESUBSCRIPTION_CLIENT_ID"),
Azure::Core::_internal::Environment::GetVariable(
"AZURESUBSCRIPTION_SERVICE_CONNECTION_ID"),
Azure::Core::_internal::Environment::GetVariable("SYSTEM_ACCESSTOKEN")),
std::make_shared<Azure::Identity::DefaultAzureCredential>()});
}
else
{
Expand Down
1 change: 1 addition & 0 deletions sdk/eventhubs/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ extends:
LiveTestTimeoutInMinutes: 120
LineCoverageTarget: 27
BranchCoverageTarget: 13
UseFederatedAuth: true
gearama marked this conversation as resolved.
Show resolved Hide resolved
Artifacts:
- Name: azure-messaging-eventhubs
Path: azure-messaging-eventhubs
Expand Down
38 changes: 19 additions & 19 deletions sdk/identity/azure-identity/samples/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,22 @@ target_link_libraries(workload_identity_credential_sample PRIVATE azure-identity
target_include_directories(workload_identity_credential_sample PRIVATE .)
create_per_service_target_build_for_sample(identity workload_identity_credential_sample)

add_executable(client_secret_credential_sample client_secret_credential.cpp)
target_link_libraries(client_secret_credential_sample PRIVATE azure-identity service get-env-helper)
target_include_directories(client_secret_credential_sample PRIVATE .)
create_per_service_target_build_for_sample(identity client_secret_credential_sample)

add_executable(default_azure_credential_sample default_azure_credential.cpp)
target_link_libraries(default_azure_credential_sample PRIVATE azure-identity service)
target_include_directories(default_azure_credential_sample PRIVATE .)
create_per_service_target_build_for_sample(identity default_azure_credential_sample)

add_executable(environment_credential_sample environment_credential.cpp)
target_link_libraries(environment_credential_sample PRIVATE azure-identity service)
target_include_directories(environment_credential_sample PRIVATE .)
create_per_service_target_build_for_sample(identity environment_credential_sample)

add_executable(managed_identity_credential_sample managed_identity_credential.cpp)
target_link_libraries(managed_identity_credential_sample PRIVATE azure-identity service)
target_include_directories(managed_identity_credential_sample PRIVATE .)
create_per_service_target_build_for_sample(identity managed_identity_credential_sample)
#add_executable(client_secret_credential_sample client_secret_credential.cpp)
gearama marked this conversation as resolved.
Show resolved Hide resolved
gearama marked this conversation as resolved.
Show resolved Hide resolved
#target_link_libraries(client_secret_credential_sample PRIVATE azure-identity service get-env-helper)
#target_include_directories(client_secret_credential_sample PRIVATE .)
#create_per_service_target_build_for_sample(identity client_secret_credential_sample)

#add_executable(default_azure_credential_sample default_azure_credential.cpp)
#target_link_libraries(default_azure_credential_sample PRIVATE azure-identity service)
#target_include_directories(default_azure_credential_sample PRIVATE .)
#create_per_service_target_build_for_sample(identity default_azure_credential_sample)
Comment on lines +39 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Please explain why we are commenting all the samples out.

Copy link
Member Author

Choose a reason for hiding this comment

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

identity samples that don't run due to creds issues in live

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand the issue.

It's reasonable for us to disable building/running the samples if we know the problem and have a plan to update it (even in a separate, follow-up PR).

Can you share the error you were seeing or the link to the failing pipeline that required us to comment these out?

Copy link
Member

Choose a reason for hiding this comment

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


#add_executable(environment_credential_sample environment_credential.cpp)
#target_link_libraries(environment_credential_sample PRIVATE azure-identity service)
#target_include_directories(environment_credential_sample PRIVATE .)
#create_per_service_target_build_for_sample(identity environment_credential_sample)

#add_executable(managed_identity_credential_sample managed_identity_credential.cpp)
#target_link_libraries(managed_identity_credential_sample PRIVATE azure-identity service)
#target_include_directories(managed_identity_credential_sample PRIVATE .)
#create_per_service_target_build_for_sample(identity managed_identity_credential_sample)
Original file line number Diff line number Diff line change
Expand Up @@ -675,4 +675,4 @@ TEST(AzurePipelinesCredential, DISABLED_InvalidSystemAccessToken_LIVEONLY_)
{
EXPECT_TRUE(std::string(ex.what()).find("302 (Found)") != std::string::npos) << ex.what();
}
}
}*/
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ if (BUILD_PERFORMANCE_TESTS)
add_subdirectory(test/perf)
endif()

if(BUILD_SAMPLES)
if(BUILD_SAMPLES_DISABLED)
add_subdirectory(samples)
endif()

Expand Down
2 changes: 1 addition & 1 deletion sdk/keyvault/azure-security-keyvault-keys/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ if (BUILD_PERFORMANCE_TESTS)
add_subdirectory(test/perf)
endif()

if(BUILD_SAMPLES)
if(BUILD_SAMPLES_DISABLED)
add_subdirectory(samples)
endif()

Expand Down
3 changes: 1 addition & 2 deletions sdk/keyvault/azure-security-keyvault-keys/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,10 @@ Use the [Azure CLI][azure_cli] snippet below to create/get client secret credent
```
"<your-service-principal-object-id>"
```
- Use the returned credentials above to set **AZURE_CLIENT_ID** (appId), **AZURE_CLIENT_SECRET** (password), and **AZURE_TENANT_ID** (tenant) environment variables. The following example shows a way to do this in Powershell:
- Use the returned credentials above to set **AZURE_CLIENT_ID** (appId) and **AZURE_TENANT_ID** (tenant) environment variables. The following example shows a way to do this in Powershell:
gearama marked this conversation as resolved.
Show resolved Hide resolved

```PowerShell
$Env:AZURE_CLIENT_ID="generated-app-ID"
$Env:AZURE_CLIENT_SECRET="random-password"
$Env:AZURE_TENANT_ID="tenant-ID"
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ if (BUILD_PERFORMANCE_TESTS)
add_subdirectory(test/perf)
endif()

if(BUILD_SAMPLES)
if(BUILD_SAMPLES_DISABLED)
gearama marked this conversation as resolved.
Show resolved Hide resolved
add_subdirectory(samples)
endif()

Expand Down
1 change: 1 addition & 0 deletions sdk/keyvault/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ extends:
LiveTestTimeoutInMinutes: 120
LineCoverageTarget: 81
BranchCoverageTarget: 42
UseFederatedAuth: true
gearama marked this conversation as resolved.
Show resolved Hide resolved
Artifacts:
- Name: azure-security-keyvault-keys
Path: azure-security-keyvault-keys
Expand Down
1 change: 0 additions & 1 deletion sdk/storage/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,3 @@ additional questions or comments.
[coc]: https://opensource.microsoft.com/codeofconduct/
[coc_faq]: https://opensource.microsoft.com/codeofconduct/faq/
[coc_contact]: mailto:[email protected]

1 change: 1 addition & 0 deletions sdk/storage/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ extends:
LiveTestCtestRegex: azure-storage
Clouds: Preview
SupportedClouds: Preview
UseFederatedAuth: false
gearama marked this conversation as resolved.
Show resolved Hide resolved
Artifacts:
- Name: azure-storage-common
Path: azure-storage-common
Expand Down
1 change: 1 addition & 0 deletions sdk/tables/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ extends:
CtestRegex: azure-data
LineCoverageTarget: 77
BranchCoverageTarget: 42
UseFederatedAuth: true
gearama marked this conversation as resolved.
Show resolved Hide resolved
LiveTestCtestRegex: azure-data
Clouds: Preview
SupportedClouds: Preview
Expand Down
Loading