From fbe9a3ecd4133898c5abd6fbba9d699f61d0e347 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Thu, 28 Jan 2021 09:20:16 +0200 Subject: [PATCH] Fix proxy and internal hostname case (#646) (#649) * Fix proxy and internal hostname case Signed-off-by: Anatolii Bazko --- pkg/controller/che/init_test.go | 23 +++ pkg/controller/che/proxy.go | 45 +++-- pkg/controller/che/proxy_test.go | 317 +++++++++++++++++++++++++++++++ pkg/deploy/defaults.go | 1 + pkg/deploy/init_test.go | 2 +- pkg/deploy/proxy.go | 18 +- pkg/deploy/proxy_test.go | 8 +- 7 files changed, 388 insertions(+), 26 deletions(-) create mode 100644 pkg/controller/che/init_test.go create mode 100644 pkg/controller/che/proxy_test.go diff --git a/pkg/controller/che/init_test.go b/pkg/controller/che/init_test.go new file mode 100644 index 000000000..8b649bb2e --- /dev/null +++ b/pkg/controller/che/init_test.go @@ -0,0 +1,23 @@ +// +// Copyright (c) 2021 Red Hat, Inc. +// This program and the accompanying materials are made +// available under the terms of the Eclipse Public License 2.0 +// which is available at https://www.eclipse.org/legal/epl-2.0/ +// +// SPDX-License-Identifier: EPL-2.0 +// +// Contributors: +// Red Hat, Inc. - initial API and implementation +// +package che + +import ( + "github.com/eclipse/che-operator/pkg/deploy" +) + +func init() { + err := deploy.InitTestDefaultsFromDeployment("../../../deploy/operator.yaml") + if err != nil { + panic(err) + } +} diff --git a/pkg/controller/che/proxy.go b/pkg/controller/che/proxy.go index 82672a74a..cb1c9439c 100644 --- a/pkg/controller/che/proxy.go +++ b/pkg/controller/che/proxy.go @@ -23,29 +23,46 @@ import ( ) func (r *ReconcileChe) getProxyConfiguration(checluster *orgv1.CheCluster) (*deploy.Proxy, error) { - proxy, err := deploy.ReadCheClusterProxyConfiguration(checluster) - if err != nil { - return nil, err - } - + // OpenShift 4.x if util.IsOpenShift4 { clusterProxy := &configv1.Proxy{} if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, clusterProxy); err != nil { return nil, err } - // If proxy configuration exists in CR then cluster wide proxy configuration is ignored - // otherwise cluster wide proxy configuration is used and non proxy hosts - // are merged with defined ones in CR - if proxy.HttpProxy == "" && clusterProxy.Status.HTTPProxy != "" { - proxy, err = deploy.ReadClusterWideProxyConfiguration(clusterProxy, proxy.NoProxy) - if err != nil { - return nil, err - } + clusterWideProxyConf, err := deploy.ReadClusterWideProxyConfiguration(clusterProxy) + if err != nil { + return nil, err } + + cheClusterProxyConf, err := deploy.ReadCheClusterProxyConfiguration(checluster) + if err != nil { + return nil, err + } + + // If proxy configuration exists in CR then cluster wide proxy configuration is ignored + // Non proxy hosts are merged + if cheClusterProxyConf.HttpProxy != "" { + cheClusterProxyConf.NoProxy = deploy.MergeNonProxy(cheClusterProxyConf.NoProxy, clusterWideProxyConf.NoProxy) + return cheClusterProxyConf, nil + } else if clusterWideProxyConf.HttpProxy != "" { + clusterWideProxyConf.NoProxy = deploy.MergeNonProxy(clusterWideProxyConf.NoProxy, cheClusterProxyConf.NoProxy) + return clusterWideProxyConf, nil + } + + // proxy isn't configured + return &deploy.Proxy{}, nil } - return proxy, nil + // OpenShift 3.x and k8s + cheClusterProxyConf, err := deploy.ReadCheClusterProxyConfiguration(checluster) + if err != nil { + return nil, err + } + if checluster.Spec.Server.UseInternalClusterSVCNames { + cheClusterProxyConf.NoProxy = deploy.MergeNonProxy(cheClusterProxyConf.NoProxy, ".svc") + } + return cheClusterProxyConf, nil } func (r *ReconcileChe) putOpenShiftCertsIntoConfigMap(deployContext *deploy.DeployContext) (bool, error) { diff --git a/pkg/controller/che/proxy_test.go b/pkg/controller/che/proxy_test.go new file mode 100644 index 000000000..42c45333e --- /dev/null +++ b/pkg/controller/che/proxy_test.go @@ -0,0 +1,317 @@ +// +// Copyright (c) 2021 Red Hat, Inc. +// This program and the accompanying materials are made +// available under the terms of the Eclipse Public License 2.0 +// which is available at https://www.eclipse.org/legal/epl-2.0/ +// +// SPDX-License-Identifier: EPL-2.0 +// +// Contributors: +// Red Hat, Inc. - initial API and implementation +// +package che + +import ( + "os" + "reflect" + "testing" + + orgv1 "github.com/eclipse/che-operator/pkg/apis/org/v1" + "github.com/eclipse/che-operator/pkg/deploy" + "github.com/eclipse/che-operator/pkg/util" + "github.com/google/go-cmp/cmp" + configv1 "github.com/openshift/api/config/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + fakeDiscovery "k8s.io/client-go/discovery/fake" + fakeclientset "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" +) + +func TestReadProxyConfiguration(t *testing.T) { + type testCase struct { + name string + openShiftVersion string + cheCluster *orgv1.CheCluster + clusterProxy *configv1.Proxy + initObjects []runtime.Object + expectedProxyConf *deploy.Proxy + } + + testCases := []testCase{ + { + name: "Test no proxy configured", + openShiftVersion: "4", + clusterProxy: &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + }, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{}, + }, + { + name: "Test checluster proxy configured, OpenShift 4.x", + openShiftVersion: "4", + clusterProxy: &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + }, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + ProxyURL: "http://proxy", + ProxyPort: "3128", + NonProxyHosts: "host1", + }, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1", + TrustedCAMapName: "", + }, + }, + { + name: "Test checluster proxy configured, nonProxy merged, OpenShift 4.x", + openShiftVersion: "4", + clusterProxy: &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: configv1.ProxyStatus{ + HTTPProxy: "http://proxy:3128", + HTTPSProxy: "http://proxy:3128", + NoProxy: "host2", + }, + }, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + ProxyURL: "http://proxy", + ProxyPort: "3128", + NonProxyHosts: "host1", + }, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1,host2", + TrustedCAMapName: "", + }, + }, + { + name: "Test cluster wide proxy configured, OpenShift 4.x", + openShiftVersion: "4", + clusterProxy: &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: configv1.ProxyStatus{ + HTTPProxy: "http://proxy:3128", + HTTPSProxy: "http://proxy:3128", + NoProxy: "host1", + }, + }, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{}, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1", + TrustedCAMapName: "", + }, + }, + { + name: "Test cluster wide proxy configured, nonProxy merged, OpenShift 4.x", + openShiftVersion: "4", + clusterProxy: &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: configv1.ProxyStatus{ + HTTPProxy: "http://proxy:3128", + HTTPSProxy: "http://proxy:3128", + NoProxy: "host1", + }, + }, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + NonProxyHosts: "host2", + }, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1,host2", + TrustedCAMapName: "", + }, + }, + { + name: "Test checluster proxy configured, OpenShift 3.x", + openShiftVersion: "3", + clusterProxy: &configv1.Proxy{}, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + ProxyURL: "http://proxy", + ProxyPort: "3128", + NonProxyHosts: "host1", + }, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1", + TrustedCAMapName: "", + }, + }, + { + name: "Test checluster proxy configured, OpenShift 3.x and k8s, svc usage", + openShiftVersion: "3", + clusterProxy: &configv1.Proxy{}, + cheCluster: &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "eclipse-che", + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + ProxyURL: "http://proxy", + ProxyPort: "3128", + NonProxyHosts: "host1", + UseInternalClusterSVCNames: true, + }, + }, + }, + initObjects: []runtime.Object{}, + expectedProxyConf: &deploy.Proxy{ + HttpProxy: "http://proxy:3128", + HttpUser: "", + HttpPassword: "", + HttpHost: "proxy", + HttpPort: "3128", + HttpsProxy: "http://proxy:3128", + HttpsUser: "", + HttpsPassword: "", + HttpsHost: "proxy", + HttpsPort: "3128", + NoProxy: "host1,.svc", + TrustedCAMapName: "", + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + logf.SetLogger(zap.LoggerTo(os.Stdout, true)) + orgv1.SchemeBuilder.AddToScheme(scheme.Scheme) + testCase.initObjects = append(testCase.initObjects, testCase.clusterProxy, testCase.cheCluster) + + scheme := scheme.Scheme + orgv1.SchemeBuilder.AddToScheme(scheme) + scheme.AddKnownTypes(configv1.SchemeGroupVersion, &configv1.Proxy{}) + + cli := fake.NewFakeClientWithScheme(scheme, testCase.initObjects...) + nonCachedClient := fake.NewFakeClientWithScheme(scheme, testCase.initObjects...) + clientSet := fakeclientset.NewSimpleClientset() + fakeDiscovery, _ := clientSet.Discovery().(*fakeDiscovery.FakeDiscovery) + fakeDiscovery.Fake.Resources = []*metav1.APIResourceList{} + + os.Setenv("OPENSHIFT_VERSION", testCase.openShiftVersion) + util.IsOpenShift, util.IsOpenShift4, _ = util.DetectOpenShift() + + r := &ReconcileChe{ + client: cli, + nonCachedClient: nonCachedClient, + discoveryClient: fakeDiscovery, + scheme: scheme, + } + + actualProxyConf, err := r.getProxyConfiguration(testCase.cheCluster) + if err != nil { + t.Fatalf("Error reading proxy configuration: %v", err) + } + + if !reflect.DeepEqual(testCase.expectedProxyConf, actualProxyConf) { + t.Errorf("Expected deployment and deployment returned from API server differ (-want, +got): %v", cmp.Diff(testCase.expectedProxyConf, actualProxyConf)) + } + }) + } +} diff --git a/pkg/deploy/defaults.go b/pkg/deploy/defaults.go index 6d6b41c9d..f75a0edd6 100644 --- a/pkg/deploy/defaults.go +++ b/pkg/deploy/defaults.go @@ -434,6 +434,7 @@ func InitTestDefaultsFromDeployment(deploymentFile string) error { } } + os.Setenv("MOCK_API", "1") InitDefaultsFromEnv() return nil } diff --git a/pkg/deploy/init_test.go b/pkg/deploy/init_test.go index 22c06f95a..51f0fc32f 100644 --- a/pkg/deploy/init_test.go +++ b/pkg/deploy/init_test.go @@ -1,5 +1,5 @@ // -// Copyright (c) 2020-2020 Red Hat, Inc. +// Copyright (c) 2021 Red Hat, Inc. // This program and the accompanying materials are made // available under the terms of the Eclipse Public License 2.0 // which is available at https://www.eclipse.org/legal/epl-2.0/ diff --git a/pkg/deploy/proxy.go b/pkg/deploy/proxy.go index 87ded64a3..81b82205a 100644 --- a/pkg/deploy/proxy.go +++ b/pkg/deploy/proxy.go @@ -28,7 +28,7 @@ import ( "github.com/sirupsen/logrus" ) -func ReadClusterWideProxyConfiguration(clusterProxy *configv1.Proxy, noProxy string) (*Proxy, error) { +func ReadClusterWideProxyConfiguration(clusterProxy *configv1.Proxy) (*Proxy, error) { proxy := &Proxy{} // Cluster components consume the status values to configure the proxy for their component. @@ -38,12 +38,6 @@ func ReadClusterWideProxyConfiguration(clusterProxy *configv1.Proxy, noProxy str proxy.HttpsProxy = proxy.HttpProxy } proxy.NoProxy = clusterProxy.Status.NoProxy - if proxy.NoProxy == "" { - proxy.NoProxy = noProxy - } else if noProxy != "" { - proxy.NoProxy += "," + noProxy - } - httpProxy, err := url.Parse(proxy.HttpProxy) if err != nil { return nil, err @@ -121,6 +115,16 @@ func ReadCheClusterProxyConfiguration(checluster *orgv1.CheCluster) (*Proxy, err }, nil } +func MergeNonProxy(noProxy1 string, noProxy2 string) string { + if noProxy1 == "" { + return noProxy2 + } else if noProxy2 == "" { + return noProxy1 + } + + return noProxy1 + "," + noProxy2 +} + // GenerateProxyJavaOpts converts given proxy configuration into Java format. func GenerateProxyJavaOpts(proxy *Proxy, noProxy string) (javaOpts string, err error) { if noProxy == "" { diff --git a/pkg/deploy/proxy_test.go b/pkg/deploy/proxy_test.go index 3087500f7..290da69ab 100644 --- a/pkg/deploy/proxy_test.go +++ b/pkg/deploy/proxy_test.go @@ -222,7 +222,7 @@ func TestReadClusterWideProxyConfiguration(t *testing.T) { NoProxy: "host1,host2", } - actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy, "") + actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy) if !reflect.DeepEqual(actualProxy, expectedProxy) { t.Errorf("Test failed. Expected '%v', but got '%v'", expectedProxy, actualProxy) @@ -248,7 +248,7 @@ func TestReadClusterWideProxyConfigurationNoUser(t *testing.T) { HttpsPort: "1234", } - actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy, "") + actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy) if !reflect.DeepEqual(actualProxy, expectedProxy) { t.Errorf("Test failed. Expected '%v', but got '%v'", expectedProxy, actualProxy) @@ -274,10 +274,10 @@ func TestReadClusterWideProxyConfigurationNoPort(t *testing.T) { HttpsPassword: "password", HttpsHost: "myproxy.com", - NoProxy: "host1,host2,host3", + NoProxy: "host1,host2", } - actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy, "host3") + actualProxy, _ := ReadClusterWideProxyConfiguration(clusterProxy) if !reflect.DeepEqual(actualProxy, expectedProxy) { t.Errorf("Test failed. Expected '%v', but got '%v'", expectedProxy, actualProxy)