Add build_secrets support for local Docker builds

[welteki]

Description

Add support for build_secrets in local Docker builds (faas-cli build and faas-cli publish). Previously, build_secrets defined in stack.yml were only used by the remote builder and local builds silently ignored them.

As a breaking change, the remote builder now also reads file contents from build_secrets paths before sealing and sending to the builder API. Literal secret values in stack.yaml are no longer supported. This ensures consistent behaviour for both local and remote builds — build_secrets values are always file paths.

Motivation and Context

Support for build_secrets in local builds was previously only available in the pro plugin. This functionality has now been moved to faas-cli and will be dropped from the pro plugin. The remote builder handling was also updated to get parity between remote and local builds, where build_secrets values are always treated as file paths.

• I have raised an issue to propose this change (required)

How Has This Been Tested?

• Unit tests added for secret flag generation, path resolution, and file reading
• End-to-end tested with a Dockerfile template using RUN --mount=type=secret,id=api_key for:
  • Local build and publish: faas-cli build and faas-cli publish
  • Remote builder: faas-cli publish --remote-builder
• Verified the secret content was correctly injected into the built image in both cases

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have signed-off my commits with git commit -s
• I have added tests to cover my changes.
• All new and existing tests passed.

[reviewfn]

AI Pull Request Overview

Summary

• Adds support for build_secrets in local Docker builds (faas-cli build and faas-cli publish), previously only used by remote builder.
• Introduces breaking change: remote builder now reads file contents from build_secrets paths, no longer supports literal secret values.
• Ensures consistent behavior between local and remote builds by treating build_secrets as file paths only.
• Adds resolveSecretPaths function to resolve relative secret paths to absolute paths.
• Updates dockerBuild struct with BuildSecrets field and modifies build commands to include --secret flags.
• Enables DOCKER_BUILDKIT=1 when build secrets are present for local builds.
• Adds comprehensive unit tests for secret handling in both local and remote scenarios.
• Moves build_secrets functionality from pro plugin to core faas-cli.

Approval rating (1-10)

8/10 - Solid implementation with good test coverage, but breaking change for remote builder may impact existing users without file-based secrets.

Summary per file

Summary per file

File path	Summary
builder/build.go	Adds secret path resolution, BuildSecrets to struct, --secret flags in build commands.
builder/build_test.go	Adds tests for secret flag generation, path resolution, and file reading.
builder/publish.go	Adds secret path resolution and BuildSecrets to publish flow.
builder/remote_builder.go	Changes remote builder to read secret file contents instead of literals.
builder/remote_builder_test.go	Updates tests to use file-based secrets and adds readBuildSecrets tests.

Overall Assessment

The changes successfully implement build_secrets support for local builds, bringing parity with remote builds. The code is well-structured with proper error handling and extensive test coverage. However, the breaking change to remote builder behavior (requiring file paths instead of literal values) could affect users who previously used literal secrets, potentially causing deployment failures if not properly communicated. The security implications of reading and transmitting secret file contents over the network for remote builds should be carefully reviewed to ensure no unintended data exposure.

Detailed Review

Detailed Review

builder/build.go

• The addition of resolveSecretPaths correctly resolves relative paths to absolute paths using filepath.Abs, consistent with other relative path handling in faas-cli. Good error handling for invalid paths.
• Adding BuildSecrets to dockerBuild struct is appropriate and follows the existing pattern.
• Modification to getDockerBuildCommand and enabling DOCKER_BUILDKIT=1 when secrets are present is correct, as Docker BuildKit is required for secret mounts.
• The --secret flag format id=%s,src=%s matches Docker's expected syntax. However, consider if there are any edge cases where secret IDs might conflict with Docker's reserved names or contain special characters that could break the command construction.
• In BuildImage, calling resolveSecretPaths before remote builder check ensures secrets are resolved regardless of build type, which is good for consistency.

builder/build_test.go

• Test additions are comprehensive, covering both Docker and Buildx commands with and without secrets.
• Test_resolveSecretPaths_RelativePaths properly verifies that relative paths are resolved against the current working directory.
• Tests use temporary directories and files, which is good for isolation. However, the tests don't verify behavior when the current working directory changes during execution, which could be a runtime concern if faas-cli changes directories internally.
• Test_resolveSecretPaths_AbsolutePaths confirms absolute paths are unchanged, correct.
• Edge case testing for empty maps and nil inputs is good.

builder/publish.go

• Similar changes to BuildImage, with resolveSecretPaths and BuildSecrets addition. Consistent implementation.
• getDockerBuildxCommand updates mirror those in getDockerBuildCommand, maintaining parity between build and publish operations.

builder/remote_builder.go

• The shift to readBuildSecrets reading file contents is a significant breaking change. The function correctly reads files and returns contents as strings, but this changes the API contract for remote builds.
• Error handling for file reading is appropriate, with clear error messages including the secret key and path.
• Security concern: Secret contents are now read into memory and transmitted over the network. While necessary for the remote builder API, ensure that these contents are not logged, cached, or exposed in error messages. The code appears to handle this correctly by only using the resolved map for the API call.
• No validation on file size or content type, which could be a risk if large files are accidentally specified as secrets.

builder/remote_builder_test.go

• Updates to existing tests correctly use writeTempSecret to create actual files instead of passing literal strings.
• New TestReadBuildSecrets function properly tests file reading functionality.
• TestReadBuildSecrets_FileNotFound tests error handling for missing files, good.
• However, tests don't cover cases where files exist but are unreadable (permission issues), or contain binary data, which could be edge cases.

General Concerns

• Breaking Change Impact: The remote builder change from literal values to file paths is not backward compatible. While documented, this could break existing deployments. Consider adding a deprecation warning or migration guide.
• Security Review: For remote builds, reading secret files and sending contents over HTTP should undergo security review to ensure no unintended data exposure, especially if the builder API logs or stores these values.
• Performance: No consideration for large secret files in remote builds. If secrets are large binary files, this could impact memory usage and network performance.
• Consistency: The implementation maintains good consistency between local and remote builds by treating secrets as paths, though the execution differs (local uses Docker's --secret src=, remote reads and sends content).
• Error Messages: Error messages are clear and actionable, which is good for debugging.
• Testing: Test coverage is excellent, including edge cases, which reduces risk of regressions.

Risks and Recommendations

• High Risk: Breaking change for remote builder users with literal secrets. Recommend adding a migration path or clear upgrade documentation.
• Medium Risk: Security of secret transmission in remote builds. Ensure the remote builder API treats secrets as sensitive data and doesn't log them.
• Low Risk: Potential issues with very large secret files in remote builds. Consider adding size limits or warnings.
• Suggestion: Add validation to ensure secret files exist and are readable before attempting builds, to fail fast rather than during Docker operations.
• Challenge: The assumption that all users can move to file-based secrets may not hold for all deployment scenarios. Consider if a hybrid approach (supporting both) would be better, though it increases complexity.

Overall, the implementation is technically sound and well-tested. The breaking change is justified for consistency but requires careful communication to users.

AI agent details.

Agent processing time: 32.569s
Environment preparation time: 4.175s
Total time from webhook: 41.935s