Merge pull request #393 from openshift-cherrypick-robot/cherry-pick-3…

…87-to-release-4.14 [release-4.14] OCPBUGS-20150: pkg/image: avoid unnecessary service lookups when registry is removed
openshift · Nov 8, 2023 · 8e1cc19 · 8e1cc19
2 parents 064c2d0 + dd79463
commit 8e1cc19
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 357 deletions.
diff --git a/pkg/cmd/openshift-apiserver/openshiftapiserver/config.go b/pkg/cmd/openshift-apiserver/openshiftapiserver/config.go
@@ -57,41 +57,41 @@ func NewOpenshiftAPIConfig(config *openshiftcontrolplanev1.OpenShiftAPIServerCon
 
 	genericConfig := genericapiserver.NewRecommendedConfig(legacyscheme.Codecs)
 	// Current default values
-	//Serializer:                   codecs,
-	//ReadWritePort:                443,
-	//BuildHandlerChainFunc:        DefaultBuildHandlerChain,
-	//HandlerChainWaitGroup:        new(utilwaitgroup.SafeWaitGroup),
-	//LegacyAPIGroupPrefixes:       sets.NewString(DefaultLegacyAPIPrefix),
-	//DisabledPostStartHooks:       sets.NewString(),
-	//HealthzChecks:                []healthz.HealthzChecker{healthz.PingHealthz, healthz.LogHealthz},
-	//EnableIndex:                  true,
-	//EnableDiscovery:              true,
-	//EnableProfiling:              true,
-	//EnableMetrics:                true,
-	//MaxRequestsInFlight:          400,
-	//MaxMutatingRequestsInFlight:  200,
-	//RequestTimeout:               time.Duration(60) * time.Second,
-	//MinRequestTimeout:            1800,
-	//EnableAPIResponseCompression: utilfeature.DefaultFeatureGate.Enabled(features.APIResponseCompression),
-	//LongRunningFunc: genericfilters.BasicLongRunningRequestCheck(sets.NewString("watch"), sets.NewString()),
+	// Serializer:                   codecs,
+	// ReadWritePort:                443,
+	// BuildHandlerChainFunc:        DefaultBuildHandlerChain,
+	// HandlerChainWaitGroup:        new(utilwaitgroup.SafeWaitGroup),
+	// LegacyAPIGroupPrefixes:       sets.NewString(DefaultLegacyAPIPrefix),
+	// DisabledPostStartHooks:       sets.NewString(),
+	// HealthzChecks:                []healthz.HealthzChecker{healthz.PingHealthz, healthz.LogHealthz},
+	// EnableIndex:                  true,
+	// EnableDiscovery:              true,
+	// EnableProfiling:              true,
+	// EnableMetrics:                true,
+	// MaxRequestsInFlight:          400,
+	// MaxMutatingRequestsInFlight:  200,
+	// RequestTimeout:               time.Duration(60) * time.Second,
+	// MinRequestTimeout:            1800,
+	// EnableAPIResponseCompression: utilfeature.DefaultFeatureGate.Enabled(features.APIResponseCompression),
+	// LongRunningFunc: genericfilters.BasicLongRunningRequestCheck(sets.NewString("watch"), sets.NewString()),
 
 	// TODO this is actually specific to the kubeapiserver
-	//RuleResolver authorizer.RuleResolver
+	// RuleResolver authorizer.RuleResolver
 	genericConfig.SharedInformerFactory = kubeInformers
 	genericConfig.ClientConfig = kubeClientConfig
 
 	// these are set via options
-	//SecureServing *SecureServingInfo
-	//Authentication AuthenticationInfo
-	//Authorization AuthorizationInfo
-	//LoopbackClientConfig *restclient.Config
+	// SecureServing *SecureServingInfo
+	// Authentication AuthenticationInfo
+	// Authorization AuthorizationInfo
+	// LoopbackClientConfig *restclient.Config
 	// this is set after the options are overlayed to get the authorizer we need.
-	//AdmissionControl      admission.Interface
-	//ReadWritePort int
-	//PublicAddress net.IP
+	// AdmissionControl      admission.Interface
+	// ReadWritePort int
+	// PublicAddress net.IP
 
 	// these are defaulted sanely during complete
-	//DiscoveryAddresses discovery.Addresses
+	// DiscoveryAddresses discovery.Addresses
 
 	genericConfig.CorsAllowedOriginList = config.CORSAllowedOrigins
 	genericConfig.Version = &openshiftVersion
@@ -146,7 +146,7 @@ func NewOpenshiftAPIConfig(config *openshiftcontrolplanev1.OpenShiftAPIServerCon
 	}
 
 	// I'm just hoping this works.  I don't think we use it.
-	//MergedResourceConfig *serverstore.ResourceConfig
+	// MergedResourceConfig *serverstore.ResourceConfig
 
 	servingOptions, err := serving.ToServingOptions(config.ServingInfo)
 	if err != nil {
@@ -220,10 +220,7 @@ func NewOpenshiftAPIConfig(config *openshiftcontrolplanev1.OpenShiftAPIServerCon
 	if len(config.ImagePolicyConfig.ExternalRegistryHostnames) > 0 {
 		externalRegistryHostname = config.ImagePolicyConfig.ExternalRegistryHostnames[0]
 	}
-	registryHostnameRetriever, err := registryhostname.DefaultRegistryHostnameRetriever(kubeClientConfig, externalRegistryHostname, config.ImagePolicyConfig.InternalRegistryHostname)
-	if err != nil {
-		return nil, err
-	}
+	registryHostnameRetriever := registryhostname.DefaultRegistryHostnameRetriever(externalRegistryHostname, config.ImagePolicyConfig.InternalRegistryHostname)
 
 	var caData []byte
 	if len(config.ImagePolicyConfig.AdditionalTrustedCA) != 0 {

diff --git a/pkg/image/apiserver/registry/imagestream/etcd/etcd_test.go b/pkg/image/apiserver/registry/imagestream/etcd/etcd_test.go
@@ -34,11 +34,6 @@ const (
 	name = "foo"
 )
 
-var (
-	testDefaultRegistry = func(_ context.Context) (string, bool) { return "test", true }
-	noDefaultRegistry   = func(_ context.Context) (string, bool) { return "", false }
-)
-
 type fakeSubjectAccessReviewRegistry struct {
 	err              error
 	allow            bool
@@ -82,7 +77,7 @@ func newStorage(t *testing.T) (*REST, *LayersREST, *InternalREST, *MockImageLaye
 		DeleteCollectionWorkers: 1,
 		ResourcePrefix:          "imagestreams",
 	}
-	registry := registryhostname.TestingRegistryHostnameRetriever(noDefaultRegistry, "", "")
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", "")
 	imageIndex := NewMockImageLayerIndex()
 	imageStorage, layersStorage, _, internalStorage, err := NewRESTWithLimitVerifier(
 		imagestreamRESTOptions,

diff --git a/pkg/image/apiserver/registry/imagestream/strategy_test.go b/pkg/image/apiserver/registry/imagestream/strategy_test.go
@@ -33,8 +33,7 @@ import (
 	"github.com/openshift/openshift-apiserver/pkg/image/apiserver/testutil"
 )
 
-type fakeUser struct {
-}
+type fakeUser struct{}
 
 var _ user.Info = &fakeUser{}
 
@@ -56,14 +55,6 @@ func (u *fakeUser) GetExtra() map[string][]string {
 	}
 }
 
-type fakeDefaultRegistry struct {
-	registry string
-}
-
-func (f *fakeDefaultRegistry) DefaultRegistry(_ context.Context) (string, bool) {
-	return f.registry, len(f.registry) > 0
-}
-
 type subjectAccessReviewRecord struct {
 	err              error
 	allow            bool
@@ -149,7 +140,7 @@ func TestPublicDockerImageRepository(t *testing.T) {
 	}
 
 	for testName, test := range tests {
-		strategy := NewStrategy(registryhostname.TestingRegistryHostnameRetriever(nil, test.publicRegistry, ""), &fakeSubjectAccessReviewRegistry{}, &admfake.ImageStreamLimitVerifier{}, nil, nil)
+		strategy := NewStrategy(registryhostname.DefaultRegistryHostnameRetriever(test.publicRegistry, ""), &fakeSubjectAccessReviewRegistry{}, &admfake.ImageStreamLimitVerifier{}, nil, nil)
 		value := strategy.publicDockerImageRepository(test.stream)
 		if e, a := test.expected, value; e != a {
 			t.Errorf("%s: expected %q, got %q", testName, e, a)
@@ -219,8 +210,7 @@ func TestDockerImageRepository(t *testing.T) {
 	}
 
 	for testName, test := range tests {
-		fakeRegistry := &fakeDefaultRegistry{test.defaultRegistry}
-		strategy := NewStrategy(registryhostname.TestingRegistryHostnameRetriever(fakeRegistry.DefaultRegistry, "", ""), &fakeSubjectAccessReviewRegistry{}, &admfake.ImageStreamLimitVerifier{}, nil, nil)
+		strategy := NewStrategy(registryhostname.DefaultRegistryHostnameRetriever("", test.defaultRegistry), &fakeSubjectAccessReviewRegistry{}, &admfake.ImageStreamLimitVerifier{}, nil, nil)
 		value := strategy.dockerImageRepository(context.TODO(), test.stream, true)
 		if e, a := test.expected, value; e != a {
 			t.Errorf("%s: expected %q, got %q", testName, e, a)
@@ -694,14 +684,13 @@ func TestLimitVerifier(t *testing.T) {
 		sar := &fakeSubjectAccessReviewRegistry{}
 		sar.allow = true
 		tagVerifier := &TagVerifier{sar}
-		fakeRegistry := &fakeDefaultRegistry{}
 		s := &Strategy{
 			tagVerifier: tagVerifier,
 			limitVerifier: &admfake.ImageStreamLimitVerifier{
 				ImageStreamEvaluator: tc.isEvaluator,
 			},
 			registryWhitelister:       &fake.RegistryWhitelister{},
-			registryHostnameRetriever: registryhostname.TestingRegistryHostnameRetriever(fakeRegistry.DefaultRegistry, "", ""),
+			registryHostnameRetriever: registryhostname.DefaultRegistryHostnameRetriever("", ""),
 		}
 
 		ctx := apirequest.WithUser(apirequest.NewDefaultContext(), &fakeUser{})
@@ -1239,7 +1228,7 @@ func TestTagsChanged(t *testing.T) {
 			},
 		}
 		// we can't reuse the same map twice, it causes both to be modified during updates
-		var previousTagHistory = test.existingTagHistory
+		previousTagHistory := test.existingTagHistory
 		if previousTagHistory != nil {
 			previousTagHistoryCopy := map[string]imageapi.TagEventList{}
 			for k, v := range previousTagHistory {
@@ -1263,9 +1252,8 @@ func TestTagsChanged(t *testing.T) {
 			previousStream = nil
 		}
 
-		fakeRegistry := &fakeDefaultRegistry{}
 		s := &Strategy{
-			registryHostnameRetriever: registryhostname.TestingRegistryHostnameRetriever(fakeRegistry.DefaultRegistry, "", ""),
+			registryHostnameRetriever: registryhostname.DefaultRegistryHostnameRetriever("", ""),
 			imageStreamGetter:         &fakeImageStreamGetter{test.otherStream},
 		}
 		err := s.tagsChanged(context.TODO(), previousStream, stream)

diff --git a/pkg/image/apiserver/registry/imagestreamimage/rest_test.go b/pkg/image/apiserver/registry/imagestreamimage/rest_test.go
@@ -30,8 +30,6 @@ import (
 	_ "github.com/openshift/openshift-apiserver/pkg/api/install"
 )
 
-var testDefaultRegistry = func(_ context.Context) (string, bool) { return "defaultregistry:5000", true }
-
 type fakeSubjectAccessReviewRegistry struct{}
 
 func (f *fakeSubjectAccessReviewRegistry) Create(_ context.Context, subjectAccessReview *authorizationapi.SubjectAccessReview, _ metav1.CreateOptions) (*authorizationapi.SubjectAccessReview, error) {
@@ -264,7 +262,7 @@ func TestGet(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			defaultRegistry := registryhostname.TestingRegistryHostnameRetriever(testDefaultRegistry, "", "")
+			defaultRegistry := registryhostname.DefaultRegistryHostnameRetriever("", "defaultregistry:5000")
 			etcdStorageConfigForImageStreams := &storagebackend.ConfigForResource{
 				Config:        *etcdStorage,
 				GroupResource: schema.GroupResource{Group: "image.openshift.io", Resource: "imagestreams"},

diff --git a/pkg/image/apiserver/registry/imagestreammapping/rest_test.go b/pkg/image/apiserver/registry/imagestreammapping/rest_test.go
@@ -42,8 +42,6 @@ import (
 
 const testDefaultRegistryURL = "defaultregistry:5000"
 
-var testDefaultRegistry = func(_ context.Context) (string, bool) { return testDefaultRegistryURL, true }
-
 type fakeSubjectAccessReviewRegistry struct{}
 
 func (f *fakeSubjectAccessReviewRegistry) Create(_ context.Context, subjectAccessReview *authorizationapi.SubjectAccessReview, _ metav1.CreateOptions) (*authorizationapi.SubjectAccessReview, error) {
@@ -65,7 +63,7 @@ func setup(t *testing.T) (etcd.KV, *etcdtesting.EtcdTestServer, *REST) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	registry := registryhostname.TestingRegistryHostnameRetriever(testDefaultRegistry, "", "")
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", testDefaultRegistryURL)
 	etcdStorageConfigForImageStreams := &storagebackend.ConfigForResource{Config: *etcdStorage, GroupResource: schema.GroupResource{Group: "image.openshift.io", Resource: "imagestreams"}}
 	imagestreamRESTOptions := generic.RESTOptions{StorageConfig: etcdStorageConfigForImageStreams, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "imagestreams"}
 	imageStreamStorage, imageStreamLayersStorage, imageStreamStatus, internalStorage, err := imagestreametcd.NewRESTWithLimitVerifier(imagestreamRESTOptions, registry, &fakeSubjectAccessReviewRegistry{}, &admfake.ImageStreamLimitVerifier{}, &fake.RegistryWhitelister{}, imagestreametcd.NewMockImageLayerIndex())
@@ -604,7 +602,7 @@ func TestTrackingTags(t *testing.T) {
 // TestCreateRetryUnrecoverable ensures that an attempt to create a mapping
 // using failing registry update calls will return an error.
 func TestCreateRetryUnrecoverable(t *testing.T) {
-	registry := registryhostname.TestingRegistryHostnameRetriever(nil, "", testDefaultRegistryURL)
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", testDefaultRegistryURL)
 	restInstance := &REST{
 		strategy: NewStrategy(registry),
 		imageRegistry: &fakeImageRegistry{
@@ -638,7 +636,7 @@ func TestCreateRetryUnrecoverable(t *testing.T) {
 // that result in resource conflicts that do NOT include tag diffs causes the
 // create to be retried successfully.
 func TestCreateRetryConflictNoTagDiff(t *testing.T) {
-	registry := registryhostname.TestingRegistryHostnameRetriever(nil, "", testDefaultRegistryURL)
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", testDefaultRegistryURL)
 	firstUpdate := true
 	restInstance := &REST{
 		strategy: NewStrategy(registry),
@@ -684,7 +682,7 @@ func TestCreateRetryConflictTagDiff(t *testing.T) {
 	firstGet := true
 	firstUpdate := true
 	restInstance := &REST{
-		strategy: NewStrategy(registryhostname.TestingRegistryHostnameRetriever(nil, "", testDefaultRegistryURL)),
+		strategy: NewStrategy(registryhostname.DefaultRegistryHostnameRetriever("", testDefaultRegistryURL)),
 		imageRegistry: &fakeImageRegistry{
 			createImage: func(ctx context.Context, image *imageapi.Image) error {
 				return nil

diff --git a/pkg/image/apiserver/registry/imagestreamtag/rest_test.go b/pkg/image/apiserver/registry/imagestreamtag/rest_test.go
@@ -34,8 +34,6 @@ import (
 	_ "github.com/openshift/openshift-apiserver/pkg/api/install"
 )
 
-var testDefaultRegistry = func(_ context.Context) (string, bool) { return "defaultregistry:5000", true }
-
 type fakeSubjectAccessReviewRegistry struct{}
 
 func (f *fakeSubjectAccessReviewRegistry) Create(_ context.Context, subjectAccessReview *authorizationapi.SubjectAccessReview, _ metav1.CreateOptions) (*authorizationapi.SubjectAccessReview, error) {
@@ -85,7 +83,7 @@ func setup(
 	if err != nil {
 		t.Fatal(err)
 	}
-	registry := registryhostname.TestingRegistryHostnameRetriever(testDefaultRegistry, "", "")
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", "defaultregistry:5000")
 	imageStreamStorage, imageStreamLayersStorage, imageStreamStatus, internalStorage, err := imagestreametcd.NewRESTWithLimitVerifier(
 		imagestreamRESTOptions,
 		registry,

diff --git a/pkg/image/apiserver/registry/imagetag/rest_test.go b/pkg/image/apiserver/registry/imagetag/rest_test.go
@@ -36,8 +36,6 @@ import (
 	_ "github.com/openshift/openshift-apiserver/pkg/api/install"
 )
 
-var testDefaultRegistry = func(_ context.Context) (string, bool) { return "defaultregistry:5000", true }
-
 type fakeSubjectAccessReviewRegistry struct{}
 
 func (f *fakeSubjectAccessReviewRegistry) Create(_ context.Context, subjectAccessReview *authorizationapi.SubjectAccessReview, _ metav1.CreateOptions) (*authorizationapi.SubjectAccessReview, error) {
@@ -82,7 +80,7 @@ func setup(t *testing.T) (etcd.KV, *etcdtesting.EtcdTestServer, *REST) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	registry := registryhostname.TestingRegistryHostnameRetriever(testDefaultRegistry, "", "")
+	registry := registryhostname.DefaultRegistryHostnameRetriever("", "defaultregistry:5000")
 	imageStreamStorage, imageStreamLayersStorage, imageStreamStatus, internalStorage, err := imagestreametcd.NewRESTWithLimitVerifier(
 		imagestreamRESTOptions,
 		registry,