Integrate Armstrong Validation into the spec PR check suite.

[konrad-jamrozik]

Our partner @ms-zhenhua is working on adding a new check to the spec PR check suite.

Overview of the proposed check

The proposed initial check implementation can be currently found in this PR:

• Pull Request 544843: Add Armstrong Validation Task

Per that PR description, the proposed check is checking for presence of credentials in Terraform (.tf) files by leveraging https://github.com/Azure/armstrong.

General approach to implementing the check

@ms-zhenhua we no longer add any new checks to the openapi-alps repository. All new checks should follow the new check integration model we adopted.

Eventually we will provide a full guide (#28772) but for now below I provide some pointers to help you get started.

Implementing a new check: a quickstart

@ms-zhenhua We have few checks following this model:

• PowerShell-based TypeSpec Requirement GitHub check.
• TypeScript-based TypeSpec Validation GitHub check.
• Swagger PrettierCheck GitHub check migrated from openapi-alps to the new model.

You can find sources of all these checks in https://github.com/Azure/azure-rest-api-specs/tree/main/eng.

You should implement your new check by copy-pasting and adapting one of the existing checks. I.e. you should create a PR that will add your new check artifacts inside the https://github.com/Azure/azure-rest-api-specs/tree/main/eng directory. We will review it.

We recommend keeping it simple and implementing it using PowerShell, like TypeSpec Requirement. Consider using TypeScript only if you think the check is too complex for PowerShell.

Let me use TypeSpec Requirement to explain a bit more how these checks work.

• You can see that TypeSpec Requirement is one of the checks running on specs PRs. Here is a recent example.
• We have TypeSpec Requirement ADO pipeline that powers this check. It is integrated with GitHub by virtue of being based on GitHub repository and using appropriate service connection. See the YAML tab / Get sources sub-tab.
• The pipeline definition is typespec-requirement.yml.
• The pipeline calls into TypeSpec-Requirement.ps1 which has the business logic. If it returns exit 1 then the check will be reported as failed.
• We have configured the relevant branch protection rule to make the TypeSpec Requirement (resource-manager) (which is one of the entries in the job matrix in the pipeline definition) required so when it fails, it will block the PR.
• Because the check is required, it will automatically be included in the Automated merging requirements met check.
• To ensure appropriate message is provided in the Next Steps to Merge comment if this check fails, we have the following line of code: in openapi-alps: createCheckInfo(0, "TypeSpec Requirement (resource-manager)", [], typeSpecRequirementArmTsg),. That is the only integration point with openapi-alps.
• For suppression support, see Implementing support for suppressions section below.

When implementing your check, you should mimic the above setup. We will help with code review, setting up relevant branch protection rule and any other questions you have.

Implementing support for suppressions

@ms-zhenhua re suppressions:

The official guidance on existing suppressions is here:

• https://eng.ms/docs/products/azure-developer-experience/design/specs-pr-guides/pr-suppressions

And here is additional guidance for us devs:

• Documentation: Spec PR tools suppressions mechanisms reference azure-sdk-tools#8133

The Approved-*** is obsolete model which we slowly migrate away from. You can read more about this in the design section in Azure/azure-sdk-tools#8133.

Instead, we will want for you to provide suppression via appropriate entry in the suppresions.yaml file. The aforementioned TypeSpec-Requirement.ps1 implements support for reading suppressions.yaml. You can reuse that code for your own check. You can also read about the design here: #27348.

Notes on the existing PR

@ms-zhenhua we did preliminary review of Pull Request 544843: Add Armstrong Validation Task.

Our initial conclusions are as follows:

• The code to install "go" and the the "azure/armstrong" package should be moved into pipeline yml.
• The core of the check appears to be following logic which we think can be implemented in PowerShell-based check:
  • Find all files changed in PR named "main.tf"
  • Run "armstrong credscan" on matching folders
  • If any errors, flow to GitHub errors and fail check
• The name appears to be wrong. It should be called Terraform Armstrong Validation not Swagger ArmstrongValidation.

[maririos]

I am curious on understanding this process and the implication it will have on our users. There was a conversation on email where it was communicated that the check was going to be optional so not everyone will require it. What is the criteria for the users?

[ms-zhenhua]

I am curious on understanding this process and the implication it will have on our users. There was a conversation on email where it was communicated that the check was going to be optional so not everyone will require it. What is the criteria for the users?

There are 2 types of Armstrong validations for users:

1. When do the users need to use Armstrong to do API Test? Currently, we only require new APIs against the main branch of the public swagger repo needs to do API Test, but the scope may change in future.
2. The submitted API Test configurations must not have any explicit credential values. This check should be required for all the swagger repo.

[ms-zhenhua]

Hi @konrad-jamrozik, thank you for your guidance. I have submitted a PR(#28803) to integrate Armstrong Validation. Please help take a review.
BTW, is it possible to create a pipeline to test this feature on my dev branch as what openapi-alps supports? I need to test what's the final result displayed in GitHub.

[maririos]

Thank you @ms-zhenhua

When do the users need to use Armstrong to do API Test? Currently, we only require new APIs against the main branch of the public swagger repo needs to do API Test, but the scope may change in future.

How will the user know that? Besides a failure in the CI, how areyou thinking on letting the user know that this is a required step now?

[ms-zhenhua]

Thank you @ms-zhenhua

When do the users need to use Armstrong to do API Test? Currently, we only require new APIs against the main branch of the public swagger repo needs to do API Test, but the scope may change in future.

How will the user know that? Besides a failure in the CI, how are you thinking on letting the user know that this is a required step now?

RP API Review Team is drafting the API Test workflow and will schedule a meeting with RP teams. After reaching an agreement, API Test will be included as a part of RP API review and the RP team can submit the API Test configurations for their new APIs. Before that, I need to make Armstrong Validation as a required check so that it can help identify the security risks when RP teams start to submit their Terraform configurations.

[mikeharder]

BTW, is it possible to create a pipeline to test this feature on my dev branch as what openapi-alps supports? I need to test what's the final result displayed in GitHub.

Yes, you'd want to do the following:

1. Move the changes in your PR from a branch in your fork "ms-zhenhua", to a branch in the main repo "azure-rest-api-specs" (named something like "ms-zhenhua/armstrong-validation"). Typically we recommend using a fork, but for new checks, it's easier to test from a branch in the main repo.
2. Create a new PR from the branch, and close Integrate Armstrong Validation into the spec PR check. #28803.
3. Azure SDK creates a pipeline in our "playground" project that points to your branch, and runs your pipeline only on PRs to your branch (not to main).
4. You can test the pipeline by creating draft PRs to your branch (rather than to main).
5. When you think the check is ready for prod, merge your PR to azure-rest-api-specs/main, and we will move the DevOps pipeline to prod (and make it a required check).

[mikeharder]

@ms-zhenhua: I just left comments on 28803, feel free to reply in that PR, or if you open a new PR I can move my comments. Once you have opened the new PR we can create the test pipeline.

[ms-zhenhua]

@ms-zhenhua: I just left comments on 28803, feel free to reply in that PR, or if you open a new PR I can move my comments. Once you have opened the new PR we can create the test pipeline.

@mikeharder, thank you for the review. I created a new PR(#28829) to replace with the original one and resolved the comments. Could you please guide me how to do the step 3 of Azure SDK creates a pipeline in our "playground" project that points to your branch, and runs your pipeline only on PRs to your branch (not to main) so that I can do some test?

Besides that, I have another problem about the first point of this comment to confirm with you. At first, I decide to implement the logic with the following steps:

1. let the RP API reviewer or pipeline task to add a label Armstrong-Validation to mark whether a PR needs to do API Test and blocks this PR.
2. The RP team complete the API Test and submit the test result.(The result contains 2 parts: one are the test configurations which will be submitted with the PR, the other part are test results which will be submitted in the PR comments)
3. The RP API reviewer reviews the test result and add the "Approved-Armstrong-Validation" label to unblock the PR.

Since the label method is obsolete, is there other mechanism which can support such manual controlled validation for new check?

[maririos]

FYI @ladonnaq and @josefree