Skip to content

Conversation

gdams
Copy link
Member

@gdams gdams commented Sep 24, 2025

Add deprecation warnings for opensslcrypto, cngcrypto, and darwincrypto GOEXPERIMENT values in CI environments (GitHub Actions and Azure DevOps). These specific backend experiments are deprecated in favor of systemcrypto, which automatically selects the appropriate backend for the target platform.

The warnings use CI-specific formats:

  • GitHub Actions: ::warning:: annotation
  • Azure DevOps: ##vso[task.logissue type=warning] command

Warnings are only emitted when:

  1. Building in a detected CI environment
  2. The deprecated backend was explicitly specified by the user (not auto-selected by systemcrypto)

This helps users migrate away from the specific backend experiments without breaking existing tooling that might parse stdout/stderr, and avoids warning when systemcrypto automatically selects a backend.

A few test outputs to confirm functionality:

# GitHub actions env, using systemcrypto experiment
GITHUB_ACTIONS=true GOEXPERIMENT=systemcrypto ./go/bin/go run test_crypto.go 
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# GitHub actions env, using darwincrypto experiment (expert a warning)
GITHUB_ACTIONS=true GOEXPERIMENT=darwincrypto ./go/bin/go run test_crypto.go
::warning::GOEXPERIMENT=darwincrypto is deprecated. Use GOEXPERIMENT=systemcrypto instead, or remove GOEXPERIMENT entirely as systemcrypto is now the default.
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# GitHub actions env, providing no GOEXPERIMENT (default systemcrypto)
GITHUB_ACTIONS=true ./go/bin/go run test_crypto.go     
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# GitHub actions env, using nosystemcrypto experiment
GITHUB_ACTIONS=true GOEXPERIMENT=nosystemcrypto ./go/bin/go run test_crypto.go
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824


# Azure Devops env, using systemcrypto experiment
TF_BUILD=true GOEXPERIMENT=systemcrypto ./go/bin/go run test_crypto.go
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# Azure Devops env, using darwincrypto experiment (expert a warning)
TF_BUILD=true GOEXPERIMENT=darwincrypto ./go/bin/go run test_crypto.go
##vso[task.logissue type=warning]GOEXPERIMENT=darwincrypto is deprecated. Use GOEXPERIMENT=systemcrypto instead, or remove GOEXPERIMENT entirely as systemcrypto is now the default.
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# Azure Devops env, providing no GOEXPERIMENT (default systemcrypto)
TF_BUILD=true ./go/bin/go run test_crypto.go     
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

# Azure Devops env, using nosystemcrypto experiment
TF_BUILD=true GOEXPERIMENT=nosystemcrypto ./go/bin/go run test_crypto.go
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824


# Non Azure Devops / GitHub actions env using  using darwincrypto experiment (expect no warning)
GOEXPERIMENT=darwincrypto ./go/bin/go run test_crypto.go
SHA256: 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824

… backends

Add deprecation warnings for opensslcrypto, cngcrypto, and darwincrypto
GOEXPERIMENT values in CI environments (GitHub Actions and Azure DevOps).
These specific backend experiments are deprecated in favor of systemcrypto,
which automatically selects the appropriate backend for the target platform.

The warnings use CI-specific formats:
- GitHub Actions: ::warning:: annotation
- Azure DevOps: ##vso[task.logissue type=warning] command

Warnings are only emitted when:
1. Running in a detected CI environment
2. The deprecated backend was explicitly specified by the user
   (not auto-selected by systemcrypto)

This helps users migrate away from the specific backend experiments without
breaking existing tooling that might parse stdout/stderr, and avoids
warning when systemcrypto automatically selects a backend.
@gdams gdams requested a review from a team as a code owner September 24, 2025 11:47
@gdams gdams changed the title crypto/internal/backend: add deprecation warnings for specific crypto… crypto/internal/backend: add deprecation warnings for specific crypto backends Sep 24, 2025
@gdams
Copy link
Member Author

gdams commented Sep 24, 2025

I managed to test this by switching one of our test jobs to use cngcrypto temporarily:

Screenshot 2025-09-24 at 13 02 08

@gdams gdams force-pushed the dev/gadams/deprecate branch from 0998bf2 to 17a0655 Compare September 24, 2025 12:30
@gdams gdams marked this pull request as draft September 24, 2025 12:54
@gdams gdams force-pushed the dev/gadams/deprecate branch from d6535f5 to 8c2e476 Compare September 24, 2025 14:54
@gdams gdams marked this pull request as ready for review September 24, 2025 14:55
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

This is better than emitting some unformatted string to stdout, but only because it's more likely to be seen, as far as I can tell. It can still cause breakage:

  • It's still something being printed to a std output stream, just in a way that some environments can automatically parse.
  • A warning is often considered an error in principle, even if that logic is not directly implemented in AzDO/Actions.
  • Arguably it's worse to detect environment because then it's less reproducible. If the output is causing an error in a non-obvious way (e.g. if stderr is parsed by something else, never reported directly and therefore never seen by AzDO/Actions), this could be quite a challenge to debug.
    • Or: it might simply show up as an error, which is already bad.

But even without that, I don't think we have justification for removing or even discouraging the old experiment values. What's the problem if people keep using them? Who benefits from this warning?

+ backend)
+ }
+ // For other environments, we don't emit warnings to avoid breaking
+ // users who might be parsing stdout/stderr
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this logic holds: I would expect child processes to inherit the variables, so e.g. even if isAzureDevOps() is true, there might be a wrapper process doing parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants