Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/build/generator/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (

buildapi "github.com/openshift/origin/pkg/build/api"
buildutil "github.com/openshift/origin/pkg/build/util"
"github.com/openshift/origin/pkg/cmd/admin/policy"
"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
imageapi "github.com/openshift/origin/pkg/image/api"
"github.com/openshift/origin/pkg/util/namer"
Expand Down Expand Up @@ -235,6 +236,11 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe
return nil, err
}

// Add labels and annotations from the buildrequest. Existing label/annotations will take
// precedence because we don't want system annotations/labels (eg buildname) to get stomped on.
newBuild.Annotations = policy.MergeMaps(request.Annotations, newBuild.Annotations)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bparees does this also mean you can override the build config name annotation using this? that does not sound secure... Can we blacklist our API annotations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, that's the whole reason the merge is done in the order it's done in.... the user provided annotations have the lowest priority, they can't override any existing annotation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bparees thanks for clarification :-) 👍

newBuild.Labels = policy.MergeMaps(request.Labels, newBuild.Labels)

if len(request.Env) > 0 {
updateBuildEnv(&newBuild.Spec.Strategy, request.Env)
}
Expand Down
46 changes: 45 additions & 1 deletion pkg/build/generator/generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,45 @@ func TestInstantiateWithLastVersion(t *testing.T) {
}
}

func TestInstantiateWithLabelsAndAnnotations(t *testing.T) {
g := mockBuildGenerator()
c := g.Client.(Client)
c.GetBuildConfigFunc = func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
bc := mocks.MockBuildConfig(mocks.MockSource(), mocks.MockSourceStrategyForImageRepository(), mocks.MockOutput())
bc.Status.LastVersion = 1
return bc, nil
}
g.Client = c

req := &buildapi.BuildRequest{
ObjectMeta: kapi.ObjectMeta{
Annotations: map[string]string{
"a_1": "a_value1",
// build number is set as an annotation on the generated build, so we
// shouldn't be able to ovewrite it here.
buildapi.BuildNumberAnnotation: "bad_annotation",
},
Labels: map[string]string{
"l_1": "l_value1",
// testbclabel is defined as a label on the mockBuildConfig so we shouldn't
// be able to overwrite it here.
"testbclabel": "bad_label",
},
},
}

build, err := g.Instantiate(kapi.NewDefaultContext(), req)
if err != nil {
t.Errorf("Unexpected error %v", err)
}
if build.Annotations["a_1"] != "a_value1" || build.Annotations[buildapi.BuildNumberAnnotation] == "bad_annotation" {
t.Errorf("Build annotations were merged incorrectly: %v", build.Annotations)
}
if build.Labels["l_1"] != "l_value1" || build.Labels[buildapi.BuildLabel] == "bad_label" {
t.Errorf("Build labels were merged incorrectly: %v", build.Labels)
}
}

func TestFindImageTrigger(t *testing.T) {
defaultTrigger := &buildapi.ImageChangeTrigger{}
image1Trigger := &buildapi.ImageChangeTrigger{
Expand Down Expand Up @@ -1352,6 +1391,7 @@ func mockBuildGenerator() *BuildGenerator {
for _, s := range mocks.MockBuilderSecrets() {
fakeSecrets = append(fakeSecrets, s)
}
var b *buildapi.Build
return &BuildGenerator{
Secrets: testclient.NewSimpleFake(fakeSecrets...),
ServiceAccounts: mocks.MockBuilderServiceAccount(mocks.MockBuilderSecrets()),
Expand All @@ -1363,10 +1403,14 @@ func mockBuildGenerator() *BuildGenerator {
return nil
},
CreateBuildFunc: func(ctx kapi.Context, build *buildapi.Build) error {
b = build
return nil
},
GetBuildFunc: func(ctx kapi.Context, name string) (*buildapi.Build, error) {
return &buildapi.Build{}, nil
if b == nil {
return &buildapi.Build{}, nil
}
return b, nil
},
GetImageStreamFunc: func(ctx kapi.Context, name string) (*imageapi.ImageStream, error) {
if name != imageRepoName {
Expand Down
3 changes: 3 additions & 0 deletions pkg/build/generator/test/mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ func MockBuildConfig(source buildapi.BuildSource, strategy buildapi.BuildStrateg
return &buildapi.BuildConfig{
ObjectMeta: kapi.ObjectMeta{
Name: "test-build-config",
Labels: map[string]string{
"testbclabel": "testbcvalue",
},
},
Spec: buildapi.BuildConfigSpec{
BuildSpec: buildapi.BuildSpec{
Expand Down
9 changes: 6 additions & 3 deletions pkg/cmd/admin/policy/reconcile_sccs.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ func (o *ReconcileSCCOptions) computeUpdatedSCC(expected kapi.SecurityContextCon
}

// preserve labels and annotations
expected.Labels = mergeMaps(expected.Labels, actual.Labels)
expected.Annotations = mergeMaps(expected.Annotations, actual.Annotations)
expected.Labels = MergeMaps(expected.Labels, actual.Labels)
expected.Annotations = MergeMaps(expected.Annotations, actual.Annotations)
}

// sort volumes to remove variants in order
Expand Down Expand Up @@ -289,7 +289,10 @@ func sliceToFSType(s []string) []kapi.FSType {
return fsTypes
}

func mergeMaps(a, b map[string]string) map[string]string {
// MergeMaps will merge to map[string]string instances, with
// keys from the second argument overwriting keys from the
// first argument, in case of duplicates.
func MergeMaps(a, b map[string]string) map[string]string {
if a == nil && b == nil {
return nil
}
Expand Down