Skip to content

Add proxy support #254

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

Merged
openshift-merge-robot merged 1 commit into from
Jul 12, 2019
Merged

Add proxy support #254

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
8 changes: 8 additions & 0 deletions manifests/03-rbac-role-cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ rules:
- get
- list
- watch
- apiGroups:
- config.openshift.io
resources:
- proxies
verbs:
- get
- list
- watch
- apiGroups:
- config.openshift.io
resources:
Expand Down
21 changes: 21 additions & 0 deletions pkg/console/operator/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
corev1 "k8s.io/client-go/informers/core/v1"
appsv1 "k8s.io/client-go/kubernetes/typed/apps/v1"
coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/tools/cache"
"k8s.io/klog"

// openshift
Expand All @@ -32,13 +33,15 @@ import (

// informers
configinformer "github.com/openshift/client-go/config/informers/externalversions"
configinformerv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
operatorinformerv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"

routesinformersv1 "github.com/openshift/client-go/route/informers/externalversions/route/v1"
appsinformersv1 "k8s.io/client-go/informers/apps/v1"

// clients
configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
configlisterv1 "github.com/openshift/client-go/config/listers/config/v1"
operatorclientv1 "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1"

// operator
Expand Down Expand Up @@ -71,6 +74,9 @@ type consoleOperator struct {
// recorder
recorder events.Recorder
resourceSyncer resourcesynccontroller.ResourceSyncer
//proxy
proxyCfgLister configlisterv1.ProxyLister
proxyCfgInformer cache.SharedIndexInformer
}

func NewConsoleOperator(
Expand All @@ -83,6 +89,7 @@ func NewConsoleOperator(
deployments appsinformersv1.DeploymentInformer,
routes routesinformersv1.RouteInformer,
oauthClients oauthinformersv1.OAuthClientInformer,
proxyCfgInformer configinformerv1.ProxyInformer,

// clients
operatorConfigClient operatorclientv1.OperatorV1Interface,
Expand Down Expand Up @@ -114,6 +121,9 @@ func NewConsoleOperator(
// recorder
recorder: recorder,
resourceSyncer: resourceSyncer,
// proxy
proxyCfgLister: proxyCfgInformer.Lister(),
proxyCfgInformer: proxyCfgInformer.Informer(),
Copy link
Copy Markdown
Contributor

@benjaminapetersen benjaminapetersen Aug 27, 2019

Choose a reason for hiding this comment

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

I don't believe proxyCfgInformer: proxyCfgInformer.Informer(), is needed here. You have the WithInformer() call with the proxyCfgInformer below.

}

secretsInformer := coreV1.Secrets()
Expand All @@ -130,6 +140,7 @@ func NewConsoleOperator(
operator.WithInformer(configV1Informers.Consoles(), configNameFilter),
operator.WithInformer(operatorConfigInformer, configNameFilter),
operator.WithInformer(configV1Informers.Infrastructures(), configNameFilter),
operator.WithInformer(proxyCfgInformer, configNameFilter),
// console resources
operator.WithInformer(deployments, targetNameFilter),
operator.WithInformer(routes, targetNameFilter),
Expand All @@ -151,6 +162,7 @@ type configSet struct {
Console *configv1.Console
Operator *operatorsv1.Console
Infrastructure *configv1.Infrastructure
Proxy *configv1.Proxy
}

func (c *consoleOperator) Sync(obj metav1.Object) error {
Expand All @@ -175,10 +187,19 @@ func (c *consoleOperator) Sync(obj metav1.Object) error {
return err
}

proxyConfig, err := c.proxyCfgLister.Get("cluster")
if err != nil {
if !errors.IsNotFound(err) {
Copy link
Copy Markdown
Member

@spadgett spadgett Jul 12, 2019

Choose a reason for hiding this comment

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

👍 The proxy config might not be there always on upgrade, and we shouldn't error. Good catch.

I believe we can remove this in 4.3.

return err
}
proxyConfig = nil
}

configs := configSet{
Console: consoleConfig,
Operator: operatorConfig,
Infrastructure: infrastructureConfig,
Proxy: proxyConfig,
}

if err := c.handleSync(configs); err != nil {
Expand Down
6 changes: 3 additions & 3 deletions pkg/console/operator/sync_v400.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (co *consoleOperator) sync_v400(updatedOperatorConfig *operatorv1.Console,
}
toUpdate = toUpdate || oauthChanged

actualDeployment, depChanged, depErr := co.SyncDeployment(set.Operator, cm, serviceCAConfigMap, sec, rt, customLogoCanMount)
actualDeployment, depChanged, depErr := co.SyncDeployment(set.Operator, cm, serviceCAConfigMap, sec, rt, set.Proxy, customLogoCanMount)
if depErr != nil {
msg := fmt.Sprintf("%q: %v", "deployment", depErr)
klog.V(4).Infof("incomplete sync: %v", msg)
Expand Down Expand Up @@ -227,8 +227,8 @@ func (co *consoleOperator) SyncConsolePublicConfig(consoleURL string) (*corev1.C
return resourceapply.ApplyConfigMap(co.configMapClient, co.recorder, requiredConfigMap)
}

func (co *consoleOperator) SyncDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, canMountCustomLogo bool) (*appsv1.Deployment, bool, error) {
requiredDeployment := deploymentsub.DefaultDeployment(operatorConfig, cm, serviceCAConfigMap, sec, rt, canMountCustomLogo)
func (co *consoleOperator) SyncDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, proxyConfig *configv1.Proxy, canMountCustomLogo bool) (*appsv1.Deployment, bool, error) {
requiredDeployment := deploymentsub.DefaultDeployment(operatorConfig, cm, serviceCAConfigMap, sec, rt, proxyConfig, canMountCustomLogo)
expectedGeneration := getDeploymentGeneration(co)
genChanged := operatorConfig.ObjectMeta.Generation != operatorConfig.Status.ObservedGeneration

Expand Down
1 change: 1 addition & 0 deletions pkg/console/starter/starter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ func RunOperator(ctx *controllercmd.ControllerContext) error {
kubeInformersNamespaced.Apps().V1().Deployments(), // Deployments
routesInformersNamespaced.Route().V1().Routes(), // Route
oauthInformers.Oauth().V1().OAuthClients(), // OAuth clients
configInformers.Config().V1().Proxies(), // Proxy
// clients
operatorConfigClient.OperatorV1(),
configClient.ConfigV1(),
Expand Down
34 changes: 31 additions & 3 deletions pkg/console/subresource/deployment/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/klog"

// openshift
configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
"github.com/openshift/console-operator/pkg/api"
Expand Down Expand Up @@ -54,7 +55,7 @@ type volumeConfig struct {
isConfigMap bool
}

func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, canMountCustomLogo bool) *appsv1.Deployment {
func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, proxyConfig *configv1.Proxy, canMountCustomLogo bool) *appsv1.Deployment {
Copy link
Copy Markdown
Member

@zherman0 zherman0 Jul 10, 2019

Choose a reason for hiding this comment

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

I think this function call has gotten long enough that we should pass in a struct. Maybe add that to tech debt?

Copy link
Copy Markdown
Member Author

@jhadvig jhadvig Jul 11, 2019

Choose a reason for hiding this comment

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

Yeah definitely would add it as techdebt 👍

labels := util.LabelsForConsole()
meta := util.SharedMeta()
meta.Labels = labels
Expand Down Expand Up @@ -137,7 +138,7 @@ func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap,
TerminationGracePeriodSeconds: &gracePeriod,
SecurityContext: &corev1.PodSecurityContext{},
Containers: []corev1.Container{
consoleContainer(operatorConfig, volumeConfig),
consoleContainer(operatorConfig, volumeConfig, proxyConfig),
},
Volumes: consoleVolumes(volumeConfig),
},
Expand Down Expand Up @@ -230,7 +231,7 @@ func GetLogLevelFlag(logLevel operatorv1.LogLevel) string {
return flag
}

func consoleContainer(cr *operatorv1.Console, volConfigList []volumeConfig) corev1.Container {
func consoleContainer(cr *operatorv1.Console, volConfigList []volumeConfig, proxyConfig *configv1.Proxy) corev1.Container {
volumeMounts := consoleVolumeMounts(volConfigList)
// Since the console-operator logging has different logging levels then the capnslog,
// that we use for console server(bridge) we need to map them to each other
Expand All @@ -252,6 +253,7 @@ func consoleContainer(cr *operatorv1.Console, volConfigList []volumeConfig) core
// Name: publicURLName,
// Value: consoleURL(),
//}},
Env: setEnvironmentVariables(proxyConfig),
Ports: []corev1.ContainerPort{{
Name: consolePortName,
Protocol: corev1.ProtocolTCP,
Expand All @@ -270,6 +272,32 @@ func consoleContainer(cr *operatorv1.Console, volConfigList []volumeConfig) core
}
}

func setEnvironmentVariables(proxyConfig *configv1.Proxy) []corev1.EnvVar {
envVars := []corev1.EnvVar{}
if proxyConfig == nil {
return envVars
}
if len(proxyConfig.Status.HTTPSProxy) != 0 {
Copy link
Copy Markdown
Member

@zherman0 zherman0 Jul 10, 2019

Choose a reason for hiding this comment

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

You do a check on proxyConfig if it is nil, but what about proxyConfig.Status? Is that guaranteed to be there?

Copy link
Copy Markdown
Member

@spadgett spadgett Jul 10, 2019

Choose a reason for hiding this comment

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

Shouldn't be needed since it's not a pointer

envVars = append(envVars, corev1.EnvVar{
Name: "HTTPS_PROXY",
Value: proxyConfig.Status.HTTPSProxy,
})
}
if len(proxyConfig.Status.HTTPProxy) != 0 {
envVars = append(envVars, corev1.EnvVar{
Name: "HTTP_PROXY",
Value: proxyConfig.Status.HTTPProxy,
})
}
if len(proxyConfig.Status.NoProxy) != 0 {
envVars = append(envVars, corev1.EnvVar{
Name: "NO_PROXY",
Value: proxyConfig.Status.NoProxy,
})
}
return envVars
}

func defaultProbe() *corev1.Probe {
return &corev1.Probe{
Handler: corev1.Handler{
Expand Down
18 changes: 16 additions & 2 deletions pkg/console/subresource/deployment/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

configv1 "github.com/openshift/api/config/v1"
operatorsv1 "github.com/openshift/api/operator/v1"
v1 "github.com/openshift/api/route/v1"
"github.com/openshift/console-operator/pkg/api"
Expand All @@ -27,6 +28,7 @@ func TestDefaultDeployment(t *testing.T) {
ca *corev1.ConfigMap
sec *corev1.Secret
rt *v1.Route
proxy *configv1.Proxy
canMountCustomLogo bool
}

Expand All @@ -40,6 +42,17 @@ func TestDefaultDeployment(t *testing.T) {
},
Status: operatorsv1.ConsoleStatus{},
}

proxyConfig := &configv1.Proxy{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{},
Spec: configv1.ProxySpec{
HTTPSProxy: "https://testurl.openshift.com",
},
Status: configv1.ProxyStatus{
HTTPSProxy: "https://testurl.openshift.com",
},
}
tests := []struct {
name string
args args
Expand Down Expand Up @@ -84,6 +97,7 @@ func TestDefaultDeployment(t *testing.T) {
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{},
},
proxy: proxyConfig,
},
want: &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{},
Expand Down Expand Up @@ -171,7 +185,7 @@ func TestDefaultDeployment(t *testing.T) {
TerminationGracePeriodSeconds: &gracePeriod,
SecurityContext: &corev1.PodSecurityContext{},
Containers: []corev1.Container{
consoleContainer(consoleOperatorConfig, defaultVolumeConfig()),
consoleContainer(consoleOperatorConfig, defaultVolumeConfig(), proxyConfig),
},
Volumes: consoleVolumes(defaultVolumeConfig()),
},
Expand All @@ -188,7 +202,7 @@ func TestDefaultDeployment(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if diff := deep.Equal(DefaultDeployment(tt.args.config, tt.args.cm, tt.args.cm, tt.args.sec, tt.args.rt, tt.args.canMountCustomLogo), tt.want); diff != nil {
if diff := deep.Equal(DefaultDeployment(tt.args.config, tt.args.cm, tt.args.cm, tt.args.sec, tt.args.rt, tt.args.proxy, tt.args.canMountCustomLogo), tt.want); diff != nil {
t.Error(diff)
}
})
Expand Down