Skip to content

Deprecate OPENSHIFT_DEFAULT_REGISTRY environment variable #20150

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

Closed
wants to merge 1 commit into from
Closed

Deprecate OPENSHIFT_DEFAULT_REGISTRY environment variable #20150

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions pkg/cmd/server/apis/config/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,6 @@ type ImagePolicyConfig struct {
AllowedRegistriesForImport *AllowedRegistries
// InternalRegistryHostname sets the hostname for the default internal image
// registry. The value must be in "hostname[:port]" format.
// For backward compatibility, users can still use OPENSHIFT_DEFAULT_REGISTRY
// environment variable but this setting overrides the environment variable.
InternalRegistryHostname string
// ExternalRegistryHostname sets the hostname for the default external image
// registry. The external hostname should be set only when the image registry
Expand Down
2 changes: 0 additions & 2 deletions pkg/cmd/server/apis/config/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,6 @@ type ImagePolicyConfig struct {
AllowedRegistriesForImport *AllowedRegistries `json:"allowedRegistriesForImport,omitempty"`
// InternalRegistryHostname sets the hostname for the default internal image
// registry. The value must be in "hostname[:port]" format.
// For backward compatibility, users can still use OPENSHIFT_DEFAULT_REGISTRY
// environment variable but this setting overrides the environment variable.
InternalRegistryHostname string `json:"internalRegistryHostname,omitempty"`
// ExternalRegistryHostname sets the hostname for the default external image
// registry. The external hostname should be set only when the image registry
Expand Down
23 changes: 2 additions & 21 deletions pkg/cmd/server/origin/admission/plugin_initializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package admission
import (
"fmt"
"io/ioutil"
"os"

authorizationclient "github.com/openshift/client-go/authorization/clientset/versioned"
buildclient "github.com/openshift/client-go/build/clientset/versioned"
Expand All @@ -21,10 +20,8 @@ import (
quotaclient "github.com/openshift/origin/pkg/quota/generated/internalclientset"
"github.com/openshift/origin/pkg/quota/image"
securityinformer "github.com/openshift/origin/pkg/security/generated/informers/internalversion"
"github.com/openshift/origin/pkg/service"
templateclient "github.com/openshift/origin/pkg/template/generated/internalclientset"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/admission/initializer"
Expand Down Expand Up @@ -104,13 +101,6 @@ func NewPluginInitializer(
quotaRegistry.Add(imageEvaluators[i])
}

defaultRegistry := env("OPENSHIFT_DEFAULT_REGISTRY", "${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}")
svcCache := service.NewServiceResolverCache(kubeInternalClient.Core().Services(metav1.NamespaceDefault).Get)
defaultRegistryFunc, err := svcCache.Defer(defaultRegistry)
if err != nil {
return nil, fmt.Errorf("OPENSHIFT_DEFAULT_REGISTRY variable is invalid %q: %v", defaultRegistry, err)
}

// punch through layers to build this in order to get a string for a cloud provider file
// TODO refactor us into a forward building flow with a side channel like this
kubeOptions, err := kubernetes.BuildKubeAPIserverOptions(options)
Expand All @@ -123,7 +113,7 @@ func NewPluginInitializer(
var err error
cloudConfig, err = ioutil.ReadFile(kubeOptions.CloudProvider.CloudConfigFile)
if err != nil {
return nil, fmt.Errorf("Error reading from cloud configuration file %s: %v", kubeOptions.CloudProvider.CloudConfigFile, err)
return nil, fmt.Errorf("error reading from cloud configuration file %s: %v", kubeOptions.CloudProvider.CloudConfigFile, err)
}
}
// note: we are passing a combined quota registry here...
Expand Down Expand Up @@ -177,19 +167,10 @@ func NewPluginInitializer(
Informers: informers.GetInternalKubeInformers(),
ClusterResourceQuotaInformer: informers.GetQuotaInformers().Quota().InternalVersion().ClusterResourceQuotas(),
ClusterQuotaMapper: clusterQuotaMappingController.GetClusterQuotaMapper(),
RegistryHostnameRetriever: imageapi.DefaultRegistryHostnameRetriever(defaultRegistryFunc, options.ImagePolicyConfig.ExternalRegistryHostname, options.ImagePolicyConfig.InternalRegistryHostname),
RegistryHostnameRetriever: imageapi.DefaultRegistryHostnameRetriever(options.ImagePolicyConfig.ExternalRegistryHostname, options.ImagePolicyConfig.InternalRegistryHostname),
SecurityInformers: informers.GetSecurityInformers(),
UserInformers: informers.GetUserInformers(),
}

return admission.PluginInitializers{genericInitializer, webhookInitializer, kubePluginInitializer, openshiftPluginInitializer}, nil
}

// env returns an environment variable, or the defaultValue if it is not set.
func env(key string, defaultValue string) string {
val := os.Getenv(key)
if len(val) == 0 {
return defaultValue
}
return val
}
15 changes: 3 additions & 12 deletions pkg/cmd/server/origin/master_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"k8s.io/client-go/restmapper"

kapierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/admission"
Expand Down Expand Up @@ -54,7 +53,6 @@ import (
templateinformer "github.com/openshift/origin/pkg/template/generated/informers/internalversion"

securityinformer "github.com/openshift/origin/pkg/security/generated/informers/internalversion"
"github.com/openshift/origin/pkg/service"
"github.com/openshift/origin/pkg/util/restoptions"
)

Expand Down Expand Up @@ -150,13 +148,6 @@ func BuildMasterConfig(
return nil, err
}

defaultRegistry := env("OPENSHIFT_DEFAULT_REGISTRY", "${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}")
svcCache := service.NewServiceResolverCache(kubeInternalClient.Core().Services(metav1.NamespaceDefault).Get)
defaultRegistryFunc, err := svcCache.Defer(defaultRegistry)
if err != nil {
return nil, fmt.Errorf("OPENSHIFT_DEFAULT_REGISTRY variable is invalid %q: %v", defaultRegistry, err)
}

authenticator, authenticatorPostStartHooks, err := NewAuthenticator(options, privilegedLoopbackConfig, informers)
if err != nil {
return nil, err
Expand Down Expand Up @@ -184,14 +175,14 @@ func BuildMasterConfig(
admission.DecoratorFunc(namespaceLabelDecorator.WithNamespaceLabelConditions),
admission.DecoratorFunc(admissionmetrics.WithControllerMetrics),
}
admission, err := originadmission.NewAdmissionChains(options, admissionInitializer, admissionDecorators)
admissionChains, err := originadmission.NewAdmissionChains(options, admissionInitializer, admissionDecorators)
if err != nil {
return nil, err
}

kubeAPIServerConfig, err := kubernetes.BuildKubernetesMasterConfig(
options,
admission,
admissionChains,
authenticator,
authorizer,
)
Expand Down Expand Up @@ -235,7 +226,7 @@ func BuildMasterConfig(
ClusterQuotaMappingController: clusterQuotaMappingController,
RESTMapper: restMapper,

RegistryHostnameRetriever: imageapi.DefaultRegistryHostnameRetriever(defaultRegistryFunc, options.ImagePolicyConfig.ExternalRegistryHostname, options.ImagePolicyConfig.InternalRegistryHostname),
RegistryHostnameRetriever: imageapi.DefaultRegistryHostnameRetriever(options.ImagePolicyConfig.ExternalRegistryHostname, options.ImagePolicyConfig.InternalRegistryHostname),

PrivilegedLoopbackClientConfig: *privilegedLoopbackConfig,
PrivilegedLoopbackKubernetesClientsetInternal: kubeInternalClient,
Expand Down
32 changes: 6 additions & 26 deletions pkg/image/apis/image/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ const (

// TagReferenceAnnotationTagHidden indicates that a given TagReference is hidden from search results
TagReferenceAnnotationTagHidden = "hidden"

// ImportRegistryNotAllowed indicates that the image tag was not imported due to
// untrusted registry.
ImportRegistryNotAllowed = "registry is not allowed for import"
)

var errNoRegistryURLPathAllowed = errors.New("no path after <host>[:<port>] is allowed")
Expand Down Expand Up @@ -67,36 +63,24 @@ type RegistryHostnameRetriever interface {

// DefaultRegistryHostnameRetriever is a default implementation of
// RegistryHostnameRetriever.
// The first argument is a function that lazy-loads the value of
// OPENSHIFT_DEFAULT_REGISTRY environment variable which should be deprecated in
// future.
func DefaultRegistryHostnameRetriever(deprecatedDefaultRegistryEnvFn func() (string, bool), external, internal string) RegistryHostnameRetriever {
func DefaultRegistryHostnameRetriever(external, internal string) RegistryHostnameRetriever {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any value to this anymore. Why not just pass the two names around?

return &defaultRegistryHostnameRetriever{
deprecatedDefaultFn: deprecatedDefaultRegistryEnvFn,
externalHostname: external,
internalHostname: internal,
externalHostname: external,
internalHostname: internal,
}
}

type defaultRegistryHostnameRetriever struct {
// deprecatedDefaultFn points to a function that will lazy-load the value of
// OPENSHIFT_DEFAULT_REGISTRY.
deprecatedDefaultFn func() (string, bool)
internalHostname string
externalHostname string
internalHostname string
externalHostname string
}

// InternalRegistryHostnameFn returns a function that can be used to lazy-load
// the internal Docker Registry hostname. If the master configuration properly
// InternalRegistryHostname is set, it will prefer that over the lazy-loaded
// environment variable 'OPENSHIFT_DEFAULT_REGISTRY'.
// the internal Docker Registry hostname.
func (r *defaultRegistryHostnameRetriever) InternalRegistryHostname() (string, bool) {
if len(r.internalHostname) > 0 {
return r.internalHostname, true
}
if r.deprecatedDefaultFn != nil {
return r.deprecatedDefaultFn()
}
return "", false
}

Expand Down Expand Up @@ -813,10 +797,6 @@ func PrioritizeTags(tags []string) {
}
}

func LabelForStream(stream *ImageStream) string {
return fmt.Sprintf("%s/%s", stream.Namespace, stream.Name)
}

// JoinImageSignatureName joins image name and custom signature name into one string with @ separator.
func JoinImageSignatureName(imageName, signatureName string) (string, error) {
if len(imageName) == 0 {
Expand Down