Skip to content

Commit

Permalink
Merge pull request #817 from openshift-cherrypick-robot/cherry-pick-8…
Browse files Browse the repository at this point in the history
…15-to-release-4.14

[release-4.14] OCPBUGS-23968: Disable route controller health check for NLB setup
  • Loading branch information
openshift-merge-bot[bot] committed Dec 26, 2023
2 parents b421447 + cf42bdb commit a5176b4
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 8 deletions.
46 changes: 38 additions & 8 deletions pkg/console/controllers/healthcheck/controller.go
Expand Up @@ -16,9 +16,11 @@ import (
"k8s.io/klog/v2"

// openshift
configv1 "github.com/openshift/api/config/v1"
operatorsv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
configinformer "github.com/openshift/client-go/config/informers/externalversions"
operatorclientv1 "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1"
v1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
routeclientv1 "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
Expand All @@ -39,6 +41,7 @@ type HealthCheckController struct {
// clients
operatorClient v1helpers.OperatorClient
operatorConfigClient operatorclientv1.ConsoleInterface
infrastructureClient configclientv1.InfrastructureInterface
ingressClient configclientv1.IngressInterface
routeClient routeclientv1.RoutesGetter
configMapClient coreclientv1.ConfigMapsGetter
Expand All @@ -54,6 +57,7 @@ func NewHealthCheckController(
configMapClient coreclientv1.ConfigMapsGetter,
// informers
operatorConfigInformer v1.ConsoleInformer,
configInformer configinformer.SharedInformerFactory,
coreInformer coreinformersv1.Interface,
routeInformer routesinformersv1.RouteInformer,
// events
Expand All @@ -62,29 +66,39 @@ func NewHealthCheckController(
ctrl := &HealthCheckController{
operatorClient: operatorClient,
operatorConfigClient: operatorConfigClient,
infrastructureClient: configClient.Infrastructures(),
ingressClient: configClient.Ingresses(),
routeClient: routev1Client,
configMapClient: configMapClient,
}

configMapInformer := coreInformer.ConfigMaps()
configV1Informers := configInformer.Config().V1()

return factory.New().
WithFilteredEventsInformers( // service
util.IncludeNamesFilter(api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName),
configMapInformer.Informer(),
).WithFilteredEventsInformers( // route
WithFilteredEventsInformers( // configs
util.IncludeNamesFilter(api.ConfigResourceName),
operatorConfigInformer.Informer(),
configV1Informers.Ingresses().Informer(),
configV1Informers.Infrastructures().Informer(),
).WithFilteredEventsInformers( // service
util.IncludeNamesFilter(api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName),
configMapInformer.Informer(),
).WithFilteredEventsInformers( // route
util.IncludeNamesFilter(api.OpenShiftConsoleRouteName, api.OpenshiftConsoleCustomRouteName),
routeInformer.Informer(),
).ResyncEvery(30*time.Second).WithSync(ctrl.Sync).
ToController("HealthCheckController", recorder.WithComponentSuffix("health-check-controller"))
}

func (c *HealthCheckController) Sync(ctx context.Context, controllerContext factory.SyncContext) error {
statusHandler := status.NewStatusHandler(c.operatorClient)
operatorConfig, err := c.operatorConfigClient.Get(ctx, api.ConfigResourceName, metav1.GetOptions{})
if err != nil {
return err
klog.Errorf("operator config error: %v", err)
return statusHandler.FlushAndReturn(err)
}

updatedOperatorConfig := operatorConfig.DeepCopy()

switch updatedOperatorConfig.Spec.ManagementState {
Expand All @@ -99,14 +113,23 @@ func (c *HealthCheckController) Sync(ctx context.Context, controllerContext fact
default:
return fmt.Errorf("unknown state: %v", updatedOperatorConfig.Spec.ManagementState)
}

statusHandler := status.NewStatusHandler(c.operatorClient)

ingressConfig, err := c.ingressClient.Get(ctx, api.ConfigResourceName, metav1.GetOptions{})
if err != nil {
klog.Errorf("ingress config error: %v", err)
return statusHandler.FlushAndReturn(err)
}
infrastructureConfig, err := c.infrastructureClient.Get(ctx, api.ConfigResourceName, metav1.GetOptions{})
if err != nil {
klog.Errorf("infrastructure config error: %v", err)
return statusHandler.FlushAndReturn(err)
}

// Disable the health check for external control plane topology (hypershift) and ingress NLB.
// This is to avoid an issue with internal NLB see https://issues.redhat.com/browse/OCPBUGS-23300
if isExternalControlPlaneWithNLB(infrastructureConfig, ingressConfig) {
return nil
}

activeRouteName := api.OpenShiftConsoleRouteName
routeConfig := routesub.NewRouteConfig(updatedOperatorConfig, ingressConfig, activeRouteName)
if routeConfig.IsCustomHostnameSet() {
Expand Down Expand Up @@ -189,3 +212,10 @@ func clientWithCA(caPool *x509.CertPool) *http.Client {
},
}
}

func isExternalControlPlaneWithNLB(infrastructureConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress) bool {
return infrastructureConfig.Status.ControlPlaneTopology == configv1.ExternalTopologyMode &&
infrastructureConfig.Status.PlatformStatus.Type == configv1.AWSPlatformType &&
ingressConfig.Spec.LoadBalancer.Platform.Type == configv1.AWSPlatformType &&
ingressConfig.Spec.LoadBalancer.Platform.AWS.Type == configv1.NLB
}
102 changes: 102 additions & 0 deletions pkg/console/controllers/healthcheck/controller_test.go
@@ -0,0 +1,102 @@
package healthcheck

import (
"testing"

"github.com/go-test/deep"
configv1 "github.com/openshift/api/config/v1"
)

func TestGetPlatformURL(t *testing.T) {
type args struct {
ingressConfig *configv1.Ingress
infrastructureConfig *configv1.Infrastructure
}
tests := []struct {
name string
args args
want bool
}{
{
name: "Test NLB cluster setup",
args: args{
ingressConfig: &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.NLB,
},
},
},
},
},
infrastructureConfig: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
},
ControlPlaneTopology: configv1.ExternalTopologyMode,
},
},
},
want: true,
},
{
name: "Test non-NLB cluster setup with classic AWS LoadBalancer type",
args: args{
ingressConfig: &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.Classic,
},
},
},
},
},
infrastructureConfig: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
},
ControlPlaneTopology: configv1.ExternalTopologyMode,
},
},
},
want: false,
},
{
name: "Test non-NLB cluster setup with GCP LoadBalancer type",
args: args{
ingressConfig: &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.GCPPlatformType,
},
},
},
},
infrastructureConfig: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
},
},
},
},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if diff := deep.Equal(isExternalControlPlaneWithNLB(tt.args.infrastructureConfig, tt.args.ingressConfig), tt.want); diff != nil {
t.Error(diff)
}
})
}
}
1 change: 1 addition & 0 deletions pkg/console/starter/starter.go
Expand Up @@ -322,6 +322,7 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
kubeClient.CoreV1(),
// route
operatorConfigInformers.Operator().V1().Consoles(),
configInformers, // Config
kubeInformersNamespaced.Core().V1(), // `openshift-console` namespace informers
routesInformersNamespaced.Route().V1().Routes(),
// events
Expand Down

0 comments on commit a5176b4

Please sign in to comment.