diff --git a/app/controlplane/pkg/biz/workflowcontract.go b/app/controlplane/pkg/biz/workflowcontract.go index f502cbef2..f4e1747a5 100644 --- a/app/controlplane/pkg/biz/workflowcontract.go +++ b/app/controlplane/pkg/biz/workflowcontract.go @@ -36,6 +36,7 @@ import ( "github.com/go-kratos/kratos/v2/log" "github.com/google/uuid" "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" ) var workflowContractTracer = otelx.Tracer("chainloop-controlplane", "biz/workflowcontract") @@ -497,8 +498,10 @@ func (uc *WorkflowContractUseCase) RevisionWouldChange(ctx context.Context, orgI } // Same rule used by the data layer on Update: a new revision is created - // only when the stored raw body differs from the incoming one. - return !bytes.Equal(latest.Version.Schema.Raw, incoming.Raw), nil + // only when the stored contract differs from the incoming one. The + // comparison is semantic (formatting-insensitive) so dry-run apply and real + // apply agree regardless of which client serialized the bytes. + return !ContractRawEqual(latest.Version.Schema.Raw, incoming.Raw), nil } // ValidateContractPolicies checks that policies and policy groups referenced by the contract @@ -877,6 +880,47 @@ func UnmarshalAndValidateRawContract(raw []byte, format unmarshal.RawFormat) (*C return nil, NewErrValidation(fmt.Errorf("contract validation failed:\n v2 Contract format error: %w\n v1 CraftingSchema format error: %w", v2Err, v1Err)) } +// ContractRawEqual reports whether two raw contract bodies represent the same +// contract once parsed, ignoring serialization/formatting differences such as +// indentation, key ordering, or YAML vs JSON. This is the canonical +// change-detection comparison: re-applying the same contract through any client +// path (verbatim `wf contract apply` or the batch `apply` splitter, which +// re-marshals YAML) must compare equal, while a genuine content edit must not. +// +// Identical bytes are trivially equal. If either body cannot be parsed and +// validated, it falls back to a raw byte comparison so a malformed body never +// silently compares equal to a different one. +func ContractRawEqual(a, b []byte) bool { + // Fast path: identical bytes are trivially equal. + if bytes.Equal(a, b) { + return true + } + + ca, err := identifyUnMarshalAndValidateRawContract(a) + if err != nil { + return false + } + + cb, err := identifyUnMarshalAndValidateRawContract(b) + if err != nil { + return false + } + + return ca.semanticEqual(cb) +} + +// semanticEqual compares two parsed contracts by their canonical proto form, +// so formatting-only differences do not register as changes. When both sides +// use the v2 schema the v2 messages are compared; otherwise the v1 +// representation (always populated by UnmarshalAndValidateRawContract) is used. +func (c *Contract) semanticEqual(other *Contract) bool { + if c.isV2Schema() && other.isV2Schema() { + return proto.Equal(c.Schemav2, other.Schemav2) + } + + return proto.Equal(c.Schema, other.Schema) +} + // Will try to figure out the format of the raw contract and validate it func identifyUnMarshalAndValidateRawContract(raw []byte) (*Contract, error) { format, err := unmarshal.IdentifyFormat(raw) diff --git a/app/controlplane/pkg/biz/workflowcontract_integration_test.go b/app/controlplane/pkg/biz/workflowcontract_integration_test.go index d9bbd1e2c..586628320 100644 --- a/app/controlplane/pkg/biz/workflowcontract_integration_test.go +++ b/app/controlplane/pkg/biz/workflowcontract_integration_test.go @@ -16,6 +16,7 @@ package biz_test import ( + "bytes" "context" "fmt" "os" @@ -28,6 +29,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + yaml "gopkg.in/yaml.v3" ) func (s *workflowContractIntegrationTestSuite) TestUpdate() { @@ -160,6 +162,27 @@ func (s *workflowContractIntegrationTestSuite) TestRevisionWouldChange() { s.False(changed) }) + s.Run("semantically identical schema with different serialization would not change the revision", func() { + // Re-serialize the stored contract through a yaml.v3 Node with a different + // indentation, mirroring what the batch apply path (SplitYAMLDocuments) + // does: the bytes differ (re-indented/reflowed) but the contract is + // semantically unchanged. This is the exact dry-run drift bug. + var node yaml.Node + require.NoError(s.T(), yaml.Unmarshal(currentRaw, &node)) + var buf bytes.Buffer + enc := yaml.NewEncoder(&buf) + enc.SetIndent(2) + require.NoError(s.T(), enc.Encode(&node)) + require.NoError(s.T(), enc.Close()) + reSerialized := buf.Bytes() + // Sanity check: the reproduction is only meaningful if the bytes differ. + s.NotEqual(currentRaw, reSerialized) + + changed, err := s.WorkflowContract.RevisionWouldChange(ctx, s.org.ID, s.contractOrg1.ID.String(), reSerialized) + require.NoError(s.T(), err) + s.False(changed) + }) + s.Run("different schema would change the revision", func() { changed, err := s.WorkflowContract.RevisionWouldChange(ctx, s.org.ID, s.contractOrg1.ID.String(), updatedRaw.Raw) require.NoError(s.T(), err) diff --git a/app/controlplane/pkg/biz/workflowcontract_test.go b/app/controlplane/pkg/biz/workflowcontract_test.go index 97b27e8a4..4fa4ea145 100644 --- a/app/controlplane/pkg/biz/workflowcontract_test.go +++ b/app/controlplane/pkg/biz/workflowcontract_test.go @@ -1,5 +1,5 @@ // -// Copyright 2024 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -93,3 +93,124 @@ func TestIdentifyAndValidateRawContract(t *testing.T) { }) } } + +func TestContractRawEqual(t *testing.T) { + // v2 contract with 2-space indented sequences, as a user would hand-write it + v2TwoSpace := []byte(`apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract +spec: + materials: + - name: my-image + type: CONTAINER_IMAGE + optional: false + - name: source-code + type: ARTIFACT + optional: true + envAllowList: + - NODE_ENV +`) + + // Same semantic contract, but re-indented the way yaml.v3 reflows sequences + // (4-space indentation under the key). This mimics what the batch apply path + // produces after round-tripping through a yaml.v3 Node. + v2Reindented := []byte(`apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract +spec: + materials: + - name: my-image + type: CONTAINER_IMAGE + optional: false + - name: source-code + type: ARTIFACT + optional: true + envAllowList: + - NODE_ENV +`) + + // Same semantic contract expressed as JSON. + v2JSON := []byte(`{ + "apiVersion": "chainloop.dev/v1", + "kind": "Contract", + "metadata": {"name": "my-contract"}, + "spec": { + "materials": [ + {"name": "my-image", "type": "CONTAINER_IMAGE", "optional": false}, + {"name": "source-code", "type": "ARTIFACT", "optional": true} + ], + "envAllowList": ["NODE_ENV"] + } +}`) + + // A genuine content change: a material type differs. + v2Changed := []byte(`apiVersion: chainloop.dev/v1 +kind: Contract +metadata: + name: my-contract +spec: + materials: + - name: my-image + type: SBOM_CYCLONEDX_JSON + optional: false + - name: source-code + type: ARTIFACT + optional: true + envAllowList: + - NODE_ENV +`) + + testCases := []struct { + name string + a []byte + b []byte + wantEqual bool + }{ + { + name: "identical bytes", + a: v2TwoSpace, + b: v2TwoSpace, + wantEqual: true, + }, + { + name: "same contract, different YAML indentation", + a: v2TwoSpace, + b: v2Reindented, + wantEqual: true, + }, + { + name: "same contract, YAML vs JSON", + a: v2TwoSpace, + b: v2JSON, + wantEqual: true, + }, + { + name: "genuine content change is detected", + a: v2TwoSpace, + b: v2Changed, + wantEqual: false, + }, + { + name: "unparseable input falls back to raw byte comparison (equal)", + a: []byte("not a contract"), + b: []byte("not a contract"), + wantEqual: true, + }, + { + name: "unparseable input falls back to raw byte comparison (different)", + a: []byte("not a contract"), + b: []byte("also not a contract"), + wantEqual: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.wantEqual, ContractRawEqual(tc.a, tc.b)) + // equality must be symmetric + assert.Equal(t, tc.wantEqual, ContractRawEqual(tc.b, tc.a)) + }) + } +} diff --git a/app/controlplane/pkg/data/workflowcontract.go b/app/controlplane/pkg/data/workflowcontract.go index 883427b19..68e430d56 100644 --- a/app/controlplane/pkg/data/workflowcontract.go +++ b/app/controlplane/pkg/data/workflowcontract.go @@ -16,7 +16,6 @@ package data import ( - "bytes" "context" "fmt" "time" @@ -263,8 +262,11 @@ func (r *WorkflowContractRepo) Update(ctx context.Context, orgID uuid.UUID, name return handleError(err) } - // Create a revision only if we are providing a new contract and it has changed - if opts.Contract != nil && !bytes.Equal(lv.RawBody, opts.Contract.Raw) { + // Create a revision only if we are providing a new contract and it has changed. + // The comparison is semantic (formatting-insensitive) so re-applying the same + // contract serialized differently (e.g. the batch apply path re-marshals YAML) + // does not create a spurious revision. + if opts.Contract != nil && !biz.ContractRawEqual(lv.RawBody, opts.Contract.Raw) { // TODO: Add pessimist locking to make sure we are incrementing the latest revision lv, err = tx.WorkflowContractVersion.Create(). SetRawBody(opts.Contract.Raw).