From 4bcecaa7c9af883c07918cf4a69b6e49449157b5 Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Thu, 19 Feb 2026 15:38:23 +0100 Subject: [PATCH] fix: preserve user-specified registry on OpenShift redeploy --- cmd/deploy.go | 4 +- cmd/deploy_test.go | 72 ++++++++++++++++++++++++++++++++++ pkg/config/config_test.go | 5 +-- pkg/k8s/openshift.go | 16 ++++++++ pkg/k8s/openshift_unit_test.go | 26 ++++++++++++ 5 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 pkg/k8s/openshift_unit_test.go diff --git a/cmd/deploy.go b/cmd/deploy.go index 3306f1e8bf..a98d3ce110 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -291,9 +291,7 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // also update the registry because there is a registry per namespace, // and their name includes the namespace. // This saves needing a manual flag ``--registry={destination namespace registry}`` - if changingNamespace(f) && k8s.IsOpenShift() { - // TODO(lkingland): this appears to force use of the openshift - // internal registry. + if changingNamespace(f) && k8s.IsOpenShift() && k8s.IsOpenShiftInternalRegistry(f.Registry) { f.Registry = "image-registry.openshift-image-registry.svc:5000/" + f.Namespace if cfg.Verbose { fmt.Fprintf(cmd.OutOrStdout(), "Info: Overriding openshift registry to %s\n", f.Registry) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index bdbdfbd260..d4894e4ca1 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -1337,6 +1337,78 @@ func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) { } } +// TestDeploy_NamespaceChangePreservesExternalRegistry ensures that changing +// namespace on OpenShift does not overwrite an external registry (e.g. +// docker.io/user) with the internal OpenShift registry. +// Regression test for https://github.com/knative/func/issues/2172 +func TestDeploy_NamespaceChangePreservesExternalRegistry(t *testing.T) { + root := FromTempDirectory(t) + + cleanup := k8s.SetOpenShiftForTest(true) + defer cleanup() + + // Create a function deployed to "ns1" with an external registry + f := fn.Function{ + Runtime: "go", + Root: root, + Registry: "docker.io/user", + Deploy: fn.DeploySpec{Namespace: "ns1"}, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // Deploy to a different namespace + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + cmd.SetArgs([]string{"--namespace=ns2"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // Reload and verify the external registry was preserved + f, _ = fn.NewFunction(root) + if f.Registry != "docker.io/user" { + t.Errorf("expected registry 'docker.io/user' to be preserved, got %q", f.Registry) + } +} + +// TestDeploy_NamespaceChangeUpdatesInternalRegistry ensures that changing +// namespace on OpenShift DOES update the registry when the function uses +// the internal OpenShift registry (the namespace is part of the registry path). +func TestDeploy_NamespaceChangeUpdatesInternalRegistry(t *testing.T) { + root := FromTempDirectory(t) + + cleanup := k8s.SetOpenShiftForTest(true) + defer cleanup() + + // Create a function deployed to "ns1" using the internal registry + f := fn.Function{ + Runtime: "go", + Root: root, + Registry: "image-registry.openshift-image-registry.svc:5000/ns1", + Deploy: fn.DeploySpec{Namespace: "ns1"}, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // Deploy to a different namespace + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + cmd.SetArgs([]string{"--namespace=ns2"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // Reload and verify the internal registry was updated to the new namespace + f, _ = fn.NewFunction(root) + expected := "image-registry.openshift-image-registry.svc:5000/ns2" + if f.Registry != expected { + t.Errorf("expected registry to update to %q, got %q", expected, f.Registry) + } +} + // TestDeploy_Registry ensures that a function's registry member is kept in // sync with the image tag. // During normal operation (using the client API) a function's state on disk diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 8a91f707e4..33803a30f3 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -226,7 +226,7 @@ func TestApply(t *testing.T) { } } -// TestConfigyre ensures that configuring a function results in every member +// TestConfigure ensures that configuring a function results in every member // of the function in the intersection of the two sets, global config and function // members, to be set to the values of the config. // (See the associated cfg.Apply) @@ -253,7 +253,7 @@ func TestConfigure(t *testing.T) { t.Error("configure missing map for f.Registry") } - // empty values in the global config shoul not zero out function values + // empty values in the global config should not zero out function values // when configuring. f = config.Global{}.Configure(f) if f.Build.Builder == "" { @@ -268,7 +268,6 @@ func TestConfigure(t *testing.T) { if f.Registry == "" { t.Error("empty cfg.Registry should not mutate f") } - } // TestGet_Invalid ensures that attempting to get the value of a nonexistent diff --git a/pkg/k8s/openshift.go b/pkg/k8s/openshift.go index 87fa7be491..b537ca47fc 100644 --- a/pkg/k8s/openshift.go +++ b/pkg/k8s/openshift.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/pem" "errors" + "strings" "sync" "time" @@ -93,6 +94,12 @@ func GetDefaultOpenShiftRegistry() string { return openShiftRegistryHostPort + "/" + ns } +// IsOpenShiftInternalRegistry returns true if the given registry string +// refers to the OpenShift internal image registry. +func IsOpenShiftInternalRegistry(registry string) bool { + return strings.HasPrefix(registry, openShiftRegistryHost) +} + func GetOpenShiftDockerCredentialLoaders() []creds.CredentialsCallback { conf := GetClientConfig() @@ -126,6 +133,15 @@ func GetOpenShiftDockerCredentialLoaders() []creds.CredentialsCallback { var isOpenShift bool var checkOpenShiftOnce sync.Once +// SetOpenShiftForTest overrides OpenShift detection for testing. +// Returns a cleanup function that restores the previous state. +func SetOpenShiftForTest(val bool) func() { + checkOpenShiftOnce.Do(func() {}) // ensure real detection won't run + prev := isOpenShift + isOpenShift = val + return func() { isOpenShift = prev } +} + func IsOpenShift() bool { checkOpenShiftOnce.Do(func() { isOpenShift = false diff --git a/pkg/k8s/openshift_unit_test.go b/pkg/k8s/openshift_unit_test.go new file mode 100644 index 0000000000..fdae044d3b --- /dev/null +++ b/pkg/k8s/openshift_unit_test.go @@ -0,0 +1,26 @@ +package k8s + +import "testing" + +func TestIsOpenShiftInternalRegistry(t *testing.T) { + tests := []struct { + name string + registry string + want bool + }{ + {"internal with port and namespace", "image-registry.openshift-image-registry.svc:5000/mynamespace", true}, + {"internal without port", "image-registry.openshift-image-registry.svc/mynamespace", true}, + {"internal host only", "image-registry.openshift-image-registry.svc", true}, + {"docker.io", "docker.io/user", false}, + {"ghcr.io", "ghcr.io/user", false}, + {"quay.io", "quay.io/user", false}, + {"empty string", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsOpenShiftInternalRegistry(tt.registry); got != tt.want { + t.Errorf("expected IsOpenShiftInternalRegistry(%q) = %v, got %v", tt.registry, got, tt.want) + } + }) + } +}