diff --git a/pkg/build/admission/admission.go b/pkg/build/admission/admission.go index 5569941600de..abe466d0f115 100644 --- a/pkg/build/admission/admission.go +++ b/pkg/build/admission/admission.go @@ -20,7 +20,7 @@ func init() { admission.RegisterPlugin("BuildByStrategy", func(c kclient.Interface, config io.Reader) (admission.Interface, error) { osClient, ok := c.(client.Interface) if !ok { - return nil, errors.New("client is not an Origin client") + return nil, errors.New("client is not an Openshift client") } return NewBuildByStrategy(osClient), nil }) @@ -35,7 +35,7 @@ type buildByStrategy struct { // on policy based on the build strategy type func NewBuildByStrategy(client client.Interface) admission.Interface { return &buildByStrategy{ - Handler: admission.NewHandler(admission.Create), + Handler: admission.NewHandler(admission.Create, admission.Update), client: client, } } @@ -57,7 +57,7 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error { case *buildapi.BuildRequest: return a.checkBuildRequestAuthorization(obj, attr) default: - return admission.NewForbidden(attr, fmt.Errorf("Unrecognized request object %#v", obj)) + return admission.NewForbidden(attr, fmt.Errorf("unrecognized request object %#v", obj)) } } diff --git a/pkg/build/admission/admission_test.go b/pkg/build/admission/admission_test.go index 0269be091e0a..779e4ee406dc 100644 --- a/pkg/build/admission/admission_test.go +++ b/pkg/build/admission/admission_test.go @@ -130,19 +130,22 @@ func TestBuildAdmission(t *testing.T) { }, } + ops := []admission.Operation{admission.Create, admission.Update} for _, test := range tests { - c := NewBuildByStrategy(fakeClient(test.expectedResource, test.reviewResponse, test.responseObject)) - attrs := admission.NewAttributesRecord(test.object, test.kind, "default", "name", test.resource, test.subResource, admission.Create, fakeUser()) - err := c.Admit(attrs) - if err != nil && test.expectAccept { - t.Errorf("%s: unexpected error: %v", test.name, err) - } + for _, op := range ops { + c := NewBuildByStrategy(fakeClient(test.expectedResource, test.reviewResponse, test.responseObject)) + attrs := admission.NewAttributesRecord(test.object, test.kind, "default", "name", test.resource, test.subResource, op, fakeUser()) + err := c.Admit(attrs) + if err != nil && test.expectAccept { + t.Errorf("%s: unexpected error: %v", test.name, err) + } - if !apierrors.IsForbidden(err) && !test.expectAccept { - if (len(test.expectedError) != 0) || (test.expectedError == err.Error()) { - continue + if !apierrors.IsForbidden(err) && !test.expectAccept { + if (len(test.expectedError) != 0) || (test.expectedError == err.Error()) { + continue + } + t.Errorf("%s: expecting reject error, got %v", test.name, err) } - t.Errorf("%s: expecting reject error, got %v", test.name, err) } } } diff --git a/test/integration/build_admission_test.go b/test/integration/build_admission_test.go index a37499b3885d..93031d46310f 100644 --- a/test/integration/build_admission_test.go +++ b/test/integration/build_admission_test.go @@ -54,6 +54,15 @@ func TestPolicyBasedRestrictionOfBuildCreateAndCloneByStrategy(t *testing.T) { } } + // make sure build updates are rejected + for _, strategy := range buildStrategyTypes() { + for clientType, client := range clients { + if _, err := updateBuild(t, client.Builds(testutil.Namespace()), builds[string(strategy)+clientType]); !kapierror.IsForbidden(err) { + t.Errorf("expected forbidden for strategy %s and client %s: got %v", strategy, clientType, err) + } + } + } + // make sure clone is rejected for _, strategy := range buildStrategyTypes() { for clientType, client := range clients { @@ -100,6 +109,15 @@ func TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy(t *te } } + // make sure buildconfig updates are rejected + for _, strategy := range buildStrategyTypes() { + for clientType, client := range clients { + if _, err := updateBuildConfig(t, client.BuildConfigs(testutil.Namespace()), buildConfigs[string(strategy)+clientType]); !kapierror.IsForbidden(err) { + t.Errorf("expected forbidden for strategy %s and client %s: got %v", strategy, clientType, err) + } + } + } + // make sure instantiate is rejected for _, strategy := range buildStrategyTypes() { for clientType, client := range clients { @@ -226,6 +244,11 @@ func createBuild(t *testing.T, buildInterface client.BuildInterface, strategy st return buildInterface.Create(build) } +func updateBuild(t *testing.T, buildInterface client.BuildInterface, build *buildapi.Build) (*buildapi.Build, error) { + build.Labels = map[string]string{"updated": "true"} + return buildInterface.Update(build) +} + func createBuildConfig(t *testing.T, buildConfigInterface client.BuildConfigInterface, strategy string) (*buildapi.BuildConfig, error) { buildConfig := &buildapi.BuildConfig{} buildConfig.GenerateName = strings.ToLower(string(strategy)) + "-buildconfig-" @@ -246,3 +269,8 @@ func instantiateBuildConfig(t *testing.T, buildConfigInterface client.BuildConfi req.Name = buildConfig.Name return buildConfigInterface.Instantiate(req) } + +func updateBuildConfig(t *testing.T, buildConfigInterface client.BuildConfigInterface, buildConfig *buildapi.BuildConfig) (*buildapi.BuildConfig, error) { + buildConfig.Labels = map[string]string{"updated": "true"} + return buildConfigInterface.Update(buildConfig) +}