Handle duplicates in subjects and materials consistently

[chuangw6]

Changes

/kind bug

Fixes #925

Prior, deduplication handling for subjects and materials is different.

Now, we use consistent approach to handle the deduplication.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Has Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)

Release Notes

NONE

[chuangw6]

cc @lcarva could we possibly include this fix in the upcoming release? Thanks a lot!

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/slsa/extract/extract.go	69.1%	61.5%	-7.6
pkg/chains/formats/slsa/internal/artifact/append.go	Do not exist	100.0%
pkg/chains/formats/slsa/internal/material/material.go	91.2%	92.2%	1.0

[chuangw6]

This failure seems like a flaky unit test 😮

chains/pkg/chains/formats/slsa/v2alpha2/internal/external_parameters/external_parameters_test.go

Lines 28 to 48 in 659b32b

	func TestBuildConfigSource(t *testing.T) {
	provenance := &v1beta1.Provenance{
	RefSource: &v1beta1.RefSource{
	Digest: map[string]string{"alg1": "hex1", "alg2": "hex2"},
	URI: "https://tekton.com",
	EntryPoint: "/path/to/entry",
	},
	}
	want := map[string]string{
	"ref": "alg1:hex1",
	"repository": "https://tekton.com",
	"path": "/path/to/entry",
	}
	got := buildConfigSource(provenance)
	if diff := cmp.Diff(want, got); diff != "" {
	t.Errorf("buildConfigSource(): -want +got: %s", diff)
	}
	}

In the implementation, it always gets the first entry in the map and the unit test assumes it's always the first entry in the test map. But golang maps are unordered data structure, so the first entry is non-deterministic. That's why the test is flaky.

cc @joejstuart

[joejstuart]

This failure seems like a flaky unit test 😮

chains/pkg/chains/formats/slsa/v2alpha2/internal/external_parameters/external_parameters_test.go

Lines 28 to 48 in 659b32b

	func TestBuildConfigSource(t *testing.T) {
	provenance := &v1beta1.Provenance{
	RefSource: &v1beta1.RefSource{
	Digest: map[string]string{"alg1": "hex1", "alg2": "hex2"},
	URI: "https://tekton.com",
	EntryPoint: "/path/to/entry",
	},
	}
	want := map[string]string{
	"ref": "alg1:hex1",
	"repository": "https://tekton.com",
	"path": "/path/to/entry",
	}
	got := buildConfigSource(provenance)
	if diff := cmp.Diff(want, got); diff != "" {
	t.Errorf("buildConfigSource(): -want +got: %s", diff)
	}
	}

In the implementation, it always gets the first entry in the map and the unit test assumes it's always the first entry in the test map. But golang maps are unordered data structure, so the first entry is non-deterministic. That's why the test is flaky.

cc @joejstuart

I'm unsure why the implementation only wants the first entry. It's existing code I moved. I'm wondering if we should just copy the whole provenance.RefSource to externalParams["buildConfigSource"]?

 // PipelineRun adds the pipeline run spec and provenance if available
 func PipelineRun(pro *objects.PipelineRunObject) map[string]any {
        externalParams := make(map[string]any)
 
        if provenance := pro.GetRemoteProvenance(); provenance != nil {
-               externalParams["buildConfigSource"] = buildConfigSource(provenance)
+               externalParams["buildConfigSource"] = provenance.RefSource
        }
        externalParams["runSpec"] = pro.Spec
        return externalParams

[chuangw6]

That's a good idea, but currently slsa defines the ref field for other ci/cd as string i.e. workflow.ref for github

Maybe something we can flag to slsa team in this generic proposal? slsa-framework/slsa#940

cc @chitrangpatel for thoughts.

[chitrangpatel]

That's a good idea, but currently slsa defines the ref field for other ci/cd as string i.e. workflow.ref for github

Maybe something we can flag to slsa team in this generic proposal? slsa-framework/slsa#940

cc @chitrangpatel for thoughts.

I think we should add it to the generic propossal. RefSource is a very Tekton Specific field. I think a more generic format for cicd based systems will be more useful for verifiers.

[joejstuart]

That's a good idea, but currently slsa defines the ref field for other ci/cd as string i.e. workflow.ref for github
Maybe something we can flag to slsa team in this generic proposal? slsa-framework/slsa#940
cc @chitrangpatel for thoughts.

I think we should add it to the generic propossal. RefSource is a very Tekton Specific field. I think a more generic format for cicd based systems will be more useful for verifiers.

Here's a test that should work. #928.

+1 for adding the whole RefSource to the generic proposal.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/slsa/extract/extract.go	69.1%	61.5%	-7.6
pkg/chains/formats/slsa/internal/artifact/append.go	Do not exist	100.0%
pkg/chains/formats/slsa/internal/material/material.go	91.2%	92.2%	1.0

[chitrangpatel]

/lgtm

[chitrangpatel]

Thanks @chuangw6!

[chitrangpatel]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [chitrangpatel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment