Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUILD-187: Detect if image trigger ID was cleared #150

Merged
merged 1 commit into from
Dec 5, 2020
Merged

BUILD-187: Detect if image trigger ID was cleared #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 45 additions & 0 deletions pkg/build/controller/buildconfig/buildconfig_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
buildclientv1 "github.com/openshift/client-go/build/clientset/versioned/typed/build/v1"
buildinformer "github.com/openshift/client-go/build/informers/externalversions/build/v1"
buildlister "github.com/openshift/client-go/build/listers/build/v1"
lgbuildutil "github.com/openshift/library-go/pkg/build/buildutil"
"github.com/openshift/openshift-controller-manager/pkg/build/buildscheme"
"github.com/openshift/openshift-controller-manager/pkg/build/buildutil"
buildcommon "github.com/openshift/openshift-controller-manager/pkg/build/controller/common"
Expand Down Expand Up @@ -174,9 +175,53 @@ func (c *BuildConfigController) buildConfigAdded(obj interface{}) {
// buildconfig is updated or there is a relist of buildconfigs
func (c *BuildConfigController) buildConfigUpdated(old, cur interface{}) {
bc := cur.(*buildv1.BuildConfig)
oldBuildConfig := old.(*buildv1.BuildConfig)
// TODO: Remove the event after the image trigger behavior is removed.
adambkaplan marked this conversation as resolved.
Show resolved Hide resolved
// See https://issues.redhat.com/browse/BUILD-188
if c.imageChangeTriggerCleared(oldBuildConfig, bc) {
c.recorder.Event(bc,
corev1.EventTypeWarning,
"ImageChangeTriggerCleared",
"spec.triggers[*].imagechange.lastTriggeredImageID was cleared, which will trigger a build. This behavior is deprecated and will be removed in a future OpenShift release.")
Comment on lines +182 to +185
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Record a warning event. This will bubble up to the web console and reference the BuildConfig.

}
c.enqueueBuildConfig(bc)
}

func (c *BuildConfigController) imageChangeTriggerCleared(old, cur *buildv1.BuildConfig) bool {
if old == nil || cur == nil {
return false
}
imageLastTriggers := map[corev1.ObjectReference]string{}
for _, oldTrigger := range old.Spec.Triggers {
if oldTrigger.ImageChange == nil {
continue
}
from := c.getImageChangeTriggerInputReference(old, oldTrigger)
imageLastTriggers[*from] = oldTrigger.ImageChange.LastTriggeredImageID
}
for _, currentTrigger := range cur.Spec.Triggers {
if currentTrigger.ImageChange == nil {
continue
}
from := c.getImageChangeTriggerInputReference(cur, currentTrigger)
prev, found := imageLastTriggers[*from]
if found && len(prev) > 0 && len(currentTrigger.ImageChange.LastTriggeredImageID) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care if the devops user set it to some non-zero "dummy" value vs. clearing it out

i.e. alert that they should not be setting it in any fashion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we care less about this scenario, but is something we can check. If the current is an invalid image ref, we have a pretty good idea that the image trigger controller didn't do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I don't think we can reliably do this other than with the empty string. "foo" is a perfectly valid pull spec - otherwise would would need to assume that the image trigger always adds the full SHA reference to this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

the validity of the image ref was not a much a thought for me as the fact that they really should not be setting it at all IMO

that said, upon further review, it could get kludgy at best figuring out if a user set it vs. the controller set it

that, and don't feel strongly about it really anyway ... so let's move on .... an edge case at most, if a concern at all, so we can wait for it to crop up in a customer case before worrying about it at most

return true
}
Comment on lines +202 to +210
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of the logic here was lifted from the ImageChange trigger controller code.

}
return false
}

func (c *BuildConfigController) getImageChangeTriggerInputReference(bc *buildv1.BuildConfig, trigger buildv1.BuildTriggerPolicy) *corev1.ObjectReference {
if trigger.ImageChange == nil {
return nil
}
if trigger.ImageChange.From != nil {
return trigger.ImageChange.From
}
return lgbuildutil.GetInputReference(bc.Spec.Strategy)
}

// enqueueBuild adds the given build to the queue.
func (c *BuildConfigController) enqueueBuildConfig(bc *buildv1.BuildConfig) {
key, err := kcontroller.KeyFunc(bc)
Expand Down
163 changes: 163 additions & 0 deletions pkg/build/controller/buildconfig/buildconfig_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
ktesting "k8s.io/client-go/testing"
Expand Down Expand Up @@ -105,6 +106,143 @@ func TestHandleBuildConfig(t *testing.T) {

}

func TestCheckImageChangeTriggerCleared(t *testing.T) {
cases := []struct {
name string
oldTriggers []tagTriggerID
currentTriggers []tagTriggerID
setOldNil bool
setCurrentNil bool
expectedResult bool
}{
{
name: "old nil",
setOldNil: true,
},
{
name: "current nil",
setCurrentNil: true,
},
{
name: "both nil",
setOldNil: true,
setCurrentNil: true,
},
{
name: "no trigger changes",
oldTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
},
currentTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
},
},
{
name: "empty to populated",
oldTriggers: []tagTriggerID{
{
LastTriggeredId: "",
},
},
currentTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
},
},
{
name: "populated to empty",
oldTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
},
currentTriggers: []tagTriggerID{
{
LastTriggeredId: "",
},
},
expectedResult: true,
},
{
name: "multi empty to populated",
oldTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
{
ImageStreamTag: "test-build:latest",
LastTriggeredId: "",
},
},
currentTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
{
ImageStreamTag: "test-build:latest",
LastTriggeredId: "abcdef0",
},
},
},
{
name: "multi populated to empty",
oldTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
{
ImageStreamTag: "test-build:latest",
LastTriggeredId: "abcdef0",
},
},
currentTriggers: []tagTriggerID{
{
LastTriggeredId: "abcdef0",
},
{
ImageStreamTag: "test-build:latest",
LastTriggeredId: "",
},
},
expectedResult: true,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
objects := []runtime.Object{}

var current *buildv1.BuildConfig
if !tc.setCurrentNil {
current = buildConfigWithImageChangeTriggers(tc.currentTriggers)
}
if current != nil {
objects = append(objects, current)
}
buildClient := fake.NewSimpleClientset(objects...)
controller := &BuildConfigController{
buildLister: &okBuildLister{},
buildConfigGetter: buildClient.BuildV1(),
buildGetter: buildClient.BuildV1(),
buildConfigLister: &okBuildConfigGetter{BuildConfig: current},
recorder: &record.FakeRecorder{},
}
var old *buildv1.BuildConfig
if !tc.setOldNil {
old = buildConfigWithImageChangeTriggers(tc.oldTriggers)
}
changed := controller.imageChangeTriggerCleared(old, current)
if changed != tc.expectedResult {
t.Errorf("expected ImageChangeTriggerCleared to be %v, got %v", tc.expectedResult, changed)
}
})
}
}

func baseBuildConfig() *buildv1.BuildConfig {
bc := &buildv1.BuildConfig{}
bc.Name = "testBuildConfig"
Expand All @@ -128,6 +266,31 @@ func buildConfigWithNonZeroLastVersion() *buildv1.BuildConfig {
return bc
}

func buildConfigWithImageChangeTriggers(triggers []tagTriggerID) *buildv1.BuildConfig {
bc := baseBuildConfig()
for _, trigger := range triggers {
imageChangeTrigger := &buildv1.ImageChangeTrigger{
LastTriggeredImageID: trigger.LastTriggeredId,
}
if len(trigger.ImageStreamTag) > 0 {
imageChangeTrigger.From = &corev1.ObjectReference{
Kind: "ImageStreamTag",
Name: trigger.ImageStreamTag,
}
}
bc.Spec.Triggers = append(bc.Spec.Triggers, buildv1.BuildTriggerPolicy{
Type: buildv1.ImageChangeBuildTriggerType,
ImageChange: imageChangeTrigger,
})
}
return bc
}

type tagTriggerID struct {
ImageStreamTag string
LastTriggeredId string
}

type okBuildLister struct{}

func (okc *okBuildLister) List(label labels.Selector) ([]*buildv1.Build, error) {
Expand Down
1 change: 1 addition & 0 deletions pkg/build/metrics/prometheus/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var (
[]string{"namespace", "name", "phase", "reason", "strategy"},
nil,
)

bc = buildCollector{}
cancelledPhase = string(buildv1.BuildPhaseCancelled)
completePhase = string(buildv1.BuildPhaseComplete)
Expand Down
2 changes: 1 addition & 1 deletion pkg/build/metrics/prometheus/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func (f *fakeResponseWriter) WriteHeader(statusCode int) {
f.statusCode = statusCode
}

func TestMetrics(t *testing.T) {
func TestBuildMetrics(t *testing.T) {
// went per line vs. a block of expected test in case assumptions on ordering are subverted, as well as defer on
// getting newlines right
expectedResponse := []string{
Expand Down