Skip to content

Commit db9e95e

Browse files
smarterclaytonsmarterclayton
committed
Image policy is resolving images on replica sets by default
Even when local names are not requested, the image policy plugin is deciding to rewrite image references in replica sets that point to the integrated registry (with tags) to use digests. This causes the deployment controller that created them to get wedged (because it detects a change to the template) and become unable to update status on that replica set. https://bugzilla.redhat.com/show_bug.cgi?id=1481801
1 parent 96571a2 commit db9e95e

File tree

7 files changed

+260
-30
lines changed

7 files changed

+260
-30
lines changed

‎pkg/image/admission/imagepolicy/api/types.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ type ImageResolutionPolicyRule struct {
6060
// stream tags, but only if the target image stream tag has the "resolveLocalNames" field
6161
// set.
6262
LocalNames bool
63+
// Type controls whether resolution will happen if the rule doesn't match local names. The default
64+
// value is DoNotAttempt. This overrides the global image policy for a matching resource. If there
65+
// are multiple rules matching a resource, any rule that requests resolution or rewrite will
66+
// result in rewrite.
67+
Type ImageResolutionType
6368
}
6469

6570
// ImageExecutionPolicyRule determines whether a provided image may be used on the platform.

‎pkg/image/admission/imagepolicy/api/v1/default-policy.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,3 @@ executionRules:
1717
- key: images.openshift.io/deny-execution
1818
value: "true"
1919
skipOnResolutionFailure: true
20-

‎pkg/image/admission/imagepolicy/api/v1/defaults.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,19 @@ func SetDefaults_ImagePolicyConfig(obj *ImagePolicyConfig) {
1616

1717
if obj.ResolutionRules == nil {
1818
obj.ResolutionRules = []ImageResolutionPolicyRule{
19-
{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true},
20-
{TargetResource: GroupResource{Group: "build.openshift.io", Resource: "builds"}, LocalNames: true},
21-
{TargetResource: GroupResource{Resource: "replicationcontrollers"}, LocalNames: true},
22-
{TargetResource: GroupResource{Group: "extensions", Resource: "replicasets"}, LocalNames: true},
19+
// pods are attempted for backwards compatibility.
20+
{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true, Type: Attempt},
21+
22+
{TargetResource: GroupResource{Group: "build.openshift.io", Resource: "builds"}, LocalNames: true, Type: AttemptRewrite},
2323
{TargetResource: GroupResource{Group: "batch", Resource: "jobs"}, LocalNames: true},
2424

25-
// TODO: consider adding these
26-
// {TargetResource: GroupResource{Group: "extensions", Resource: "deployments"}, LocalNames: true},
27-
// {TargetResource: GroupResource{Group: "apps", Resource: "statefulsets"}, LocalNames: true},
25+
{TargetResource: GroupResource{Group: "extensions", Resource: "replicasets"}, LocalNames: true},
26+
{TargetResource: GroupResource{Resource: "replicationcontrollers"}, LocalNames: true},
27+
28+
{TargetResource: GroupResource{Group: "apps", Resource: "deployments"}, LocalNames: true},
29+
{TargetResource: GroupResource{Group: "extensions", Resource: "deployments"}, LocalNames: true},
30+
{TargetResource: GroupResource{Group: "apps", Resource: "statefulsets"}, LocalNames: true},
31+
{TargetResource: GroupResource{Group: "extensions", Resource: "daemonsets"}, LocalNames: true},
2832
}
2933
}
3034

‎pkg/image/admission/imagepolicy/api/v1/swagger_doc.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ var map_ImageResolutionPolicyRule = map[string]string{
5757
"": "ImageResolutionPolicyRule describes resolution rules based on resource.",
5858
"targetResource": "TargetResource is the identified group and resource. If Resource is *, this rule will apply to all resources in that group.",
5959
"localNames": "LocalNames will allow single segment names to be interpreted as namespace local image stream tags, but only if the target image stream tag has the \"resolveLocalNames\" field set.",
60+
"type": "Type controls whether resolution will happen if the rule doesn't match local names. The default value is DoNotAttempt. This overrides the global image policy for a matching resource.",
6061
}
6162

6263
func (ImageResolutionPolicyRule) SwaggerDoc() map[string]string {

‎pkg/image/admission/imagepolicy/api/v1/types.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ type ImageResolutionPolicyRule struct {
4949
// stream tags, but only if the target image stream tag has the "resolveLocalNames" field
5050
// set.
5151
LocalNames bool `json:"localNames"`
52+
// Type controls whether resolution will happen if the rule doesn't match local names. The default
53+
// value is DoNotAttempt. This overrides the global image policy for a matching resource.
54+
Type ImageResolutionType `json:"type"`
5255
}
5356

5457
// ImageExecutionPolicyRule determines whether a provided image may be used on the platform.

‎pkg/image/admission/imagepolicy/imagepolicy.go

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -451,26 +451,32 @@ var skipImageRewriteOnUpdate = map[schema.GroupResource]struct{}{
451451
}
452452

453453
// RewriteImagePullSpec applies to implicit rewrite attributes and local resources as well as if the policy requires it.
454+
// If a local name check is requested and a rule matches true is returned. The policy default resolution is only respected
455+
// if a resource isn't covered by a rule - if pods have a rule with DoNotAttempt and the global policy is RequiredRewrite,
456+
// no pods will be rewritten.
454457
func (config resolutionConfig) RewriteImagePullSpec(attr *rules.ImagePolicyAttributes, isUpdate bool, gr schema.GroupResource) bool {
455458
if isUpdate {
456459
if _, ok := skipImageRewriteOnUpdate[gr]; ok {
457460
return false
458461
}
459462
}
460-
if api.RequestsResolution(config.config.ResolveImages) {
461-
return true
462-
}
463-
if attr.LocalRewrite {
464-
for _, rule := range config.config.ResolutionRules {
465-
if !rule.LocalNames {
466-
continue
467-
}
468-
if resolutionRuleCoversResource(rule.TargetResource, gr) {
469-
return true
470-
}
463+
hasMatchingRule := false
464+
for _, rule := range config.config.ResolutionRules {
465+
if !resolutionRuleCoversResource(rule.TargetResource, gr) {
466+
continue
471467
}
468+
if rule.LocalNames && attr.LocalRewrite {
469+
return true
470+
}
471+
if api.RewriteImagePullSpec(rule.Type) {
472+
return true
473+
}
474+
hasMatchingRule = true
472475
}
473-
return false
476+
if hasMatchingRule {
477+
return false
478+
}
479+
return api.RewriteImagePullSpec(config.config.ResolveImages)
474480
}
475481

476482
// resolutionRuleCoversResource implements wildcard checking on Resource names

‎pkg/image/admission/imagepolicy/imagepolicy_test.go

Lines changed: 222 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package imagepolicy
22

33
import (
4+
"bytes"
45
"os"
56
"reflect"
67
"strings"
@@ -436,9 +437,16 @@ func TestAdmissionResolveImages(t *testing.T) {
436437
DockerImageReference: "integrated.registry/image1/image1:latest",
437438
}
438439

440+
obj, err := configlatest.ReadYAML(bytes.NewBufferString(`{"kind":"ImagePolicyConfig","apiVersion":"v1"}`))
441+
if err != nil || obj == nil {
442+
t.Fatal(err)
443+
}
444+
defaultPolicyConfig := obj.(*api.ImagePolicyConfig)
445+
439446
testCases := []struct {
440447
client *testclient.Fake
441448
policy api.ImageResolutionType
449+
config *api.ImagePolicyConfig
442450
attrs admission.Attributes
443451
admit bool
444452
expect runtime.Object
@@ -585,6 +593,168 @@ func TestAdmissionResolveImages(t *testing.T) {
585593
},
586594
},
587595
},
596+
597+
// resolves images in the integrated registry on builds without altering their ref (avoids looking up the tag)
598+
{
599+
policy: api.RequiredRewrite,
600+
client: testclient.NewSimpleFake(
601+
image1,
602+
),
603+
attrs: admission.NewAttributesRecord(
604+
&buildapi.Build{
605+
Spec: buildapi.BuildSpec{
606+
CommonSpec: buildapi.CommonSpec{
607+
Strategy: buildapi.BuildStrategy{
608+
SourceStrategy: &buildapi.SourceBuildStrategy{
609+
From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/test/mysql@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
610+
},
611+
},
612+
},
613+
},
614+
}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
615+
"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
616+
"", admission.Create, nil,
617+
),
618+
admit: true,
619+
expect: &buildapi.Build{
620+
Spec: buildapi.BuildSpec{
621+
CommonSpec: buildapi.CommonSpec{
622+
Strategy: buildapi.BuildStrategy{
623+
SourceStrategy: &buildapi.SourceBuildStrategy{
624+
From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/test/mysql@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
625+
},
626+
},
627+
},
628+
},
629+
},
630+
},
631+
// does not rewrite the config because build has DoNotAttempt by default, which overrides global policy
632+
{
633+
config: &api.ImagePolicyConfig{
634+
ResolveImages: api.RequiredRewrite,
635+
ResolutionRules: []api.ImageResolutionPolicyRule{
636+
{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}},
637+
},
638+
},
639+
client: testclient.NewSimpleFake(
640+
&imageapi.ImageStreamTag{
641+
ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
642+
Image: *image1,
643+
},
644+
),
645+
attrs: admission.NewAttributesRecord(
646+
&buildapi.Build{
647+
Spec: buildapi.BuildSpec{
648+
CommonSpec: buildapi.CommonSpec{
649+
Strategy: buildapi.BuildStrategy{
650+
CustomStrategy: &buildapi.CustomBuildStrategy{
651+
From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
652+
},
653+
},
654+
},
655+
},
656+
}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
657+
"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
658+
"", admission.Create, nil,
659+
),
660+
admit: true,
661+
expect: &buildapi.Build{
662+
Spec: buildapi.BuildSpec{
663+
CommonSpec: buildapi.CommonSpec{
664+
Strategy: buildapi.BuildStrategy{
665+
CustomStrategy: &buildapi.CustomBuildStrategy{
666+
From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
667+
},
668+
},
669+
},
670+
},
671+
},
672+
},
673+
// does not rewrite the config because build has Attempt by default, which overrides global policy
674+
{
675+
config: &api.ImagePolicyConfig{
676+
ResolveImages: api.RequiredRewrite,
677+
ResolutionRules: []api.ImageResolutionPolicyRule{
678+
{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}, Type: api.Attempt},
679+
},
680+
},
681+
client: testclient.NewSimpleFake(
682+
&imageapi.ImageStreamTag{
683+
ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
684+
Image: *image1,
685+
},
686+
),
687+
attrs: admission.NewAttributesRecord(
688+
&buildapi.Build{
689+
Spec: buildapi.BuildSpec{
690+
CommonSpec: buildapi.CommonSpec{
691+
Strategy: buildapi.BuildStrategy{
692+
CustomStrategy: &buildapi.CustomBuildStrategy{
693+
From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
694+
},
695+
},
696+
},
697+
},
698+
}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
699+
"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
700+
"", admission.Create, nil,
701+
),
702+
admit: true,
703+
expect: &buildapi.Build{
704+
Spec: buildapi.BuildSpec{
705+
CommonSpec: buildapi.CommonSpec{
706+
Strategy: buildapi.BuildStrategy{
707+
CustomStrategy: &buildapi.CustomBuildStrategy{
708+
From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
709+
},
710+
},
711+
},
712+
},
713+
},
714+
},
715+
// does rewrite the config because build has AttemptRewrite
716+
{
717+
config: &api.ImagePolicyConfig{
718+
ResolveImages: api.DoNotAttempt,
719+
ResolutionRules: []api.ImageResolutionPolicyRule{
720+
{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}, Type: api.AttemptRewrite},
721+
},
722+
},
723+
client: testclient.NewSimpleFake(
724+
&imageapi.ImageStreamTag{
725+
ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
726+
Image: *image1,
727+
},
728+
),
729+
attrs: admission.NewAttributesRecord(
730+
&buildapi.Build{
731+
Spec: buildapi.BuildSpec{
732+
CommonSpec: buildapi.CommonSpec{
733+
Strategy: buildapi.BuildStrategy{
734+
CustomStrategy: &buildapi.CustomBuildStrategy{
735+
From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
736+
},
737+
},
738+
},
739+
},
740+
}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
741+
"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
742+
"", admission.Create, nil,
743+
),
744+
admit: true,
745+
expect: &buildapi.Build{
746+
Spec: buildapi.BuildSpec{
747+
CommonSpec: buildapi.CommonSpec{
748+
Strategy: buildapi.BuildStrategy{
749+
CustomStrategy: &buildapi.CustomBuildStrategy{
750+
From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/image1/image1@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
751+
},
752+
},
753+
},
754+
},
755+
},
756+
},
757+
588758
// resolves builds.build.openshift.io with image stream tags, uses the image DockerImageReference with SHA set.
589759
{
590760
policy: api.RequiredRewrite,
@@ -735,6 +905,43 @@ func TestAdmissionResolveImages(t *testing.T) {
735905
},
736906
},
737907
},
908+
// does not resolve replica sets by default
909+
{
910+
config: defaultPolicyConfig,
911+
client: testclient.NewSimpleFake(
912+
&imageapi.ImageStreamTag{
913+
ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
914+
Image: *image1,
915+
},
916+
),
917+
attrs: admission.NewAttributesRecord(
918+
&kapiextensions.ReplicaSet{
919+
Spec: kapiextensions.ReplicaSetSpec{
920+
Template: kapi.PodTemplateSpec{
921+
Spec: kapi.PodSpec{
922+
Containers: []kapi.Container{
923+
{Image: "integrated.registry/default/test:other"},
924+
},
925+
},
926+
},
927+
},
928+
}, nil, schema.GroupVersionKind{Version: "v1", Kind: "ReplicaSet", Group: "extensions"},
929+
"default", "rs1", schema.GroupVersionResource{Version: "v1", Resource: "replicasets", Group: "extensions"},
930+
"", admission.Create, nil,
931+
),
932+
admit: true,
933+
expect: &kapiextensions.ReplicaSet{
934+
Spec: kapiextensions.ReplicaSetSpec{
935+
Template: kapi.PodTemplateSpec{
936+
Spec: kapi.PodSpec{
937+
Containers: []kapi.Container{
938+
{Image: "integrated.registry/default/test:other"},
939+
},
940+
},
941+
},
942+
},
943+
},
944+
},
738945
// resolves replica sets that specifically request lookup
739946
{
740947
policy: api.RequiredRewrite,
@@ -925,16 +1132,21 @@ func TestAdmissionResolveImages(t *testing.T) {
9251132
}
9261133
for i, test := range testCases {
9271134
onResources := []schema.GroupResource{{Resource: "builds"}, {Resource: "pods"}}
928-
p, err := newImagePolicyPlugin(&api.ImagePolicyConfig{
929-
ResolveImages: test.policy,
930-
ResolutionRules: []api.ImageResolutionPolicyRule{
931-
{LocalNames: true, TargetResource: metav1.GroupResource{Resource: "*"}},
932-
{LocalNames: true, TargetResource: metav1.GroupResource{Group: "extensions", Resource: "*"}},
933-
},
934-
ExecutionRules: []api.ImageExecutionPolicyRule{
935-
{ImageCondition: api.ImageCondition{OnResources: onResources}},
936-
},
937-
})
1135+
config := test.config
1136+
if config == nil {
1137+
// old style config
1138+
config = &api.ImagePolicyConfig{
1139+
ResolveImages: test.policy,
1140+
ResolutionRules: []api.ImageResolutionPolicyRule{
1141+
{LocalNames: true, TargetResource: metav1.GroupResource{Resource: "*"}, Type: test.policy},
1142+
{LocalNames: true, TargetResource: metav1.GroupResource{Group: "extensions", Resource: "*"}, Type: test.policy},
1143+
},
1144+
ExecutionRules: []api.ImageExecutionPolicyRule{
1145+
{ImageCondition: api.ImageCondition{OnResources: onResources}},
1146+
},
1147+
}
1148+
}
1149+
p, err := newImagePolicyPlugin(config)
9381150
if err != nil {
9391151
t.Fatal(err)
9401152
}

0 commit comments

Comments
 (0)