Address PR Feedback

- Fix mistakes in tests - Add warning for target-sign-config-overrides-root-sign-config Signed-off-by: Peter Engelbert <[email protected]>
Azure · Jul 22, 2024 · 8ac280b · 8ac280b
1 parent 9f6bf82
commit 8ac280b
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 8 deletions.
diff --git a/frontend/request.go b/frontend/request.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/Azure/dalec"
 	"github.com/goccy/go-yaml"
@@ -186,18 +187,23 @@ func MaybeSign(ctx context.Context, client gwclient.Client, st llb.State, spec *
 		return st, nil
 	}
 
+	cfg, warning := spec.GetSigner(targetKey)
 	switch cfgPath := getUserSignConfigPath(client); cfgPath {
 	case "": // no custom path provided, check the spec
-		cfg := spec.GetSigner(targetKey)
 		if cfg == nil {
 			// i.e. there's no signing config. not in the build context, not in the spec.
 			return st, nil
 		}
 
+		if warning != nil {
+			t, _, _ := strings.Cut(targetKey, "/")
+			Warnf(ctx, client, st, "%s: target %q", warning.Error(), t)
+		}
+
 		return forwardToSigner(ctx, client, cfg, st)
 	default:
 		configCtxName := getSignContextNameWithDefault(client)
-		if specCfg := spec.GetSigner(targetKey); specCfg != nil {
+		if specCfg := cfg; specCfg != nil {
 			Warnf(ctx, client, st, "Spec signing config overwritten by config at path %q in build-context %q", cfgPath, configCtxName)
 		}
 

diff --git a/helpers.go b/helpers.go
@@ -3,6 +3,7 @@ package dalec
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"path"
 	"path/filepath"
@@ -14,7 +15,13 @@ import (
 	"github.com/moby/buildkit/identity"
 )
 
-var disableDiffMerge atomic.Bool
+type Warning error
+
+var (
+	disableDiffMerge atomic.Bool
+
+	WarningTargetOverrideRootSigningConfig = errors.New("Root signing spec overridden by target signing spec")
+)
 
 // DisableDiffMerge allows disabling the use of [llb.Diff] and [llb.Merge] in favor of [llb.Copy].
 // This is needed when the buildkit version does not support [llb.Diff] and [llb.Merge].
@@ -368,18 +375,23 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption {
 	})
 }
 
-func (s *Spec) GetSigner(targetKey string) *PackageSigner {
+func (s *Spec) GetSigner(targetKey string) (*PackageSigner, Warning) {
 	if s.Targets != nil {
+		var warning Warning
+		if hasValidSigner(s.PackageConfig) {
+			warning = WarningTargetOverrideRootSigningConfig
+		}
+
 		if t, ok := s.Targets[targetKey]; ok && hasValidSigner(t.PackageConfig) {
-			return t.PackageConfig.Signer
+			return t.PackageConfig.Signer, warning
 		}
 	}
 
 	if hasValidSigner(s.PackageConfig) {
-		return s.PackageConfig.Signer
+		return s.PackageConfig.Signer, nil
 	}
 
-	return nil
+	return nil, nil
 }
 
 func hasValidSigner(pc *PackageConfig) bool {

diff --git a/test/signing_test.go b/test/signing_test.go
@@ -89,6 +89,19 @@ func linuxSigningTests(ctx context.Context, testConfig testLinuxConfig) func(*te
 			t.Parallel()
 			spec := newSigningSpec()
 
+			var found bool
+			handleStatus := func(status *client.SolveStatus) {
+				if found {
+					return
+				}
+				for _, w := range status.Warnings {
+					if strings.Contains(string(w.Short), "Root signing spec overridden") {
+						found = true
+						return
+					}
+				}
+			}
+
 			first, _, _ := strings.Cut(testConfig.SignTarget, "/")
 			spec.Targets = map[string]dalec.Target{
 				first: {
@@ -103,7 +116,9 @@ func linuxSigningTests(ctx context.Context, testConfig testLinuxConfig) func(*te
 			}
 
 			spec.PackageConfig.Signer.Image = "notexist"
-			runTest(t, distroSigningTest(t, spec, testConfig.SignTarget))
+			runTest(t, distroSigningTest(t, spec, testConfig.SignTarget), testenv.WithSolveStatusFn(handleStatus))
+
+			assert.Assert(t, found, "Spec signing override warning message not emitted")
 		})
 
 		t.Run("with args", func(t *testing.T) {
@@ -171,6 +186,8 @@ signer:
 				),
 				testenv.WithSolveStatusFn(handleStatus),
 			)
+
+			assert.Assert(t, found, "Signing overwritten warning message not emitted")
 		})
 
 		t.Run("with path build arg and build context", func(t *testing.T) {