Skip to content

NE-975: Use ingress config to set default LB type on AWS #837

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
Oct 15, 2022
Merged

NE-975: Use ingress config to set default LB type on AWS #837

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
87 changes: 68 additions & 19 deletions pkg/operator/controller/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,9 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (

// Admit if necessary. Don't process until admission succeeds. If admission is
// successful, immediately re-queue to refresh state.
if !isAdmitted(ingress) || needsReadmission(ingress) {
if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig); err != nil {
alreadyAdmitted := isAdmitted(ingress)
if !alreadyAdmitted || needsReadmission(ingress) {
if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig, alreadyAdmitted); err != nil {
switch err := err.(type) {
case *admissionRejection:
r.recorder.Event(ingress, "Warning", "Rejected", err.Reason)
Expand Down Expand Up @@ -320,7 +321,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
// fields. Returns an error value, which will have a non-nil value of type
// admissionRejection if the ingresscontroller was rejected, or a non-nil
// value of a different type if the ingresscontroller could not be processed.
func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS) error {
func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS, alreadyAdmitted bool) error {
updated := current.DeepCopy()

setDefaultDomain(updated, ingressConfig)
Expand All @@ -329,7 +330,7 @@ func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig
// so that we can set the appropriate dnsManagementPolicy. This can only be
// done after status.domain has been updated in setDefaultDomain().
domainMatchesBaseDomain := manageDNSForDomain(updated.Status.Domain, platformStatus, dnsConfig)
setDefaultPublishingStrategy(updated, platformStatus, domainMatchesBaseDomain)
setDefaultPublishingStrategy(updated, platformStatus, domainMatchesBaseDomain, ingressConfig, alreadyAdmitted)

// The TLS security profile need not be defaulted. If none is set, we
// get the default from the APIServer config (which is assumed to be
Expand Down Expand Up @@ -410,7 +411,7 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1.
return false
}

func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool) bool {
func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool, ingressConfig *configv1.Ingress, alreadyAdmitted bool) bool {
effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy()
if effectiveStrategy == nil {
var strategyType operatorv1.EndpointPublishingStrategyType
Expand Down Expand Up @@ -441,6 +442,9 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}

// Set provider parameters based on the cluster ingress config.
setDefaultProviderParameters(effectiveStrategy.LoadBalancer, ingressConfig, alreadyAdmitted)

case operatorv1.NodePortServiceStrategyType:
if effectiveStrategy.NodePort == nil {
effectiveStrategy.NodePort = &operatorv1.NodePortStrategy{}
Expand Down Expand Up @@ -519,13 +523,8 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
if statusLB.ProviderParameters.AWS == nil {
statusLB.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{}
}
// Assume the LB type is "Classic" if the field is empty.
specLBType := operatorv1.AWSClassicLoadBalancer
if specLB.ProviderParameters.AWS != nil && len(specLB.ProviderParameters.AWS.Type) != 0 {
specLBType = specLB.ProviderParameters.AWS.Type
}
if specLBType != statusLB.ProviderParameters.AWS.Type {
statusLB.ProviderParameters.AWS.Type = specLBType
if specLB.ProviderParameters.AWS.Type != statusLB.ProviderParameters.AWS.Type {
statusLB.ProviderParameters.AWS.Type = specLB.ProviderParameters.AWS.Type
changed = true
}
if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer {
Expand All @@ -552,18 +551,17 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
case operatorv1.GCPLoadBalancerProvider:
// The only provider parameter that is supported
// for GCP is the ClientAccess parameter.
var specClientAccess, statusClientAccess operatorv1.GCPClientAccess
if specLB.ProviderParameters != nil && specLB.ProviderParameters.GCP != nil {
specClientAccess = specLB.ProviderParameters.GCP.ClientAccess
}
var statusClientAccess operatorv1.GCPClientAccess
specClientAccess := specLB.ProviderParameters.GCP.ClientAccess
if statusLB.ProviderParameters != nil && statusLB.ProviderParameters.GCP != nil {
statusClientAccess = statusLB.ProviderParameters.GCP.ClientAccess
}
if specClientAccess != statusClientAccess {
if statusLB.ProviderParameters == nil {
statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
Type: operatorv1.GCPLoadBalancerProvider,
}
statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{}
}
if len(statusLB.ProviderParameters.Type) == 0 {
statusLB.ProviderParameters.Type = operatorv1.GCPLoadBalancerProvider
}
if statusLB.ProviderParameters.GCP == nil {
statusLB.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{}
Expand Down Expand Up @@ -633,6 +631,57 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
return false
}

// setDefaultProviderParameters mutates the given LoadBalancerStrategy by
// defaulting its ProviderParameters field based on the defaults in the provided
// ingress config object.
func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress, alreadyAdmitted bool) {
var provider operatorv1.LoadBalancerProviderType
if lbs.ProviderParameters != nil {
provider = lbs.ProviderParameters.Type
}
if len(provider) == 0 && !alreadyAdmitted {
// Infer the LB type from the cluster ingress config, but only
// if the ingresscontroller isn't already admitted.
switch ingressConfig.Spec.LoadBalancer.Platform.Type {
case configv1.AWSPlatformType:
provider = operatorv1.AWSLoadBalancerProvider
}
}
switch provider {
case operatorv1.AWSLoadBalancerProvider:
if lbs.ProviderParameters == nil {
lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{}
}
lbs.ProviderParameters.Type = provider
defaultLBType := operatorv1.AWSClassicLoadBalancer
if p := ingressConfig.Spec.LoadBalancer.Platform; !alreadyAdmitted && p.Type == configv1.AWSPlatformType && p.AWS != nil {
if p.AWS.Type == configv1.NLB {
defaultLBType = operatorv1.AWSNetworkLoadBalancer
}
}
if lbs.ProviderParameters.AWS == nil {
lbs.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{}
}
if len(lbs.ProviderParameters.AWS.Type) == 0 {
lbs.ProviderParameters.AWS.Type = defaultLBType
}
switch lbs.ProviderParameters.AWS.Type {
case operatorv1.AWSClassicLoadBalancer:
if lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil {
lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{}
}
}
case operatorv1.GCPLoadBalancerProvider:
if lbs.ProviderParameters == nil {
lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{}
}
lbs.ProviderParameters.Type = provider
if lbs.ProviderParameters.GCP == nil {
lbs.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{}
}
}
}

// tlsProfileSpecForIngressController returns a TLS profile spec based on either
// the profile specified by the given ingresscontroller, the profile specified
// by the APIServer config if the ingresscontroller does not specify one, or the
Expand Down
155 changes: 153 additions & 2 deletions pkg/operator/controller/ingress/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,41 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
},
},
}
ingressControllerWithAWSClassicLB = &operatorv1.IngressController{
Status: operatorv1.IngressControllerStatus{
EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.LoadBalancerServiceStrategyType,
LoadBalancer: &operatorv1.LoadBalancerStrategy{
DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS,
Scope: operatorv1.ExternalLoadBalancer,
ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
Type: operatorv1.AWSLoadBalancerProvider,
AWS: &operatorv1.AWSLoadBalancerParameters{
Type: operatorv1.AWSClassicLoadBalancer,
ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{},
},
},
},
},
},
}
ingressControllerWithAWSNLB = &operatorv1.IngressController{
Status: operatorv1.IngressControllerStatus{
EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.LoadBalancerServiceStrategyType,
LoadBalancer: &operatorv1.LoadBalancerStrategy{
DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS,
Scope: operatorv1.ExternalLoadBalancer,
ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
Type: operatorv1.AWSLoadBalancerProvider,
AWS: &operatorv1.AWSLoadBalancerParameters{
Type: operatorv1.AWSNetworkLoadBalancer,
},
},
},
},
},
}
ingressControllerWithLoadBalancerUnmanagedDNS = &operatorv1.IngressController{
Status: operatorv1.IngressControllerStatus{
EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
Expand Down Expand Up @@ -152,11 +187,37 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
Type: platform,
}
}

ingressConfigWithDefaultClassicLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.Classic,
},
},
},
},
}
ingressConfigWithDefaultNLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.NLB,
},
},
},
},
}
)

testCases := []struct {
name string
platformStatus *configv1.PlatformStatus
ingressConfig *configv1.Ingress
expectedIC *operatorv1.IngressController
domainMatchesBaseDomain bool
}{
Expand All @@ -172,6 +233,20 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
expectedIC: ingressControllerWithLoadBalancer,
domainMatchesBaseDomain: true,
},
{
name: "AWS with ingress config specifying Classic LB",
platformStatus: makePlatformStatus(configv1.AWSPlatformType),
ingressConfig: ingressConfigWithDefaultClassicLB,
expectedIC: ingressControllerWithAWSClassicLB,
domainMatchesBaseDomain: true,
},
{
name: "AWS with ingress config specifying NLB",
platformStatus: makePlatformStatus(configv1.AWSPlatformType),
ingressConfig: ingressConfigWithDefaultNLB,
expectedIC: ingressControllerWithAWSNLB,
domainMatchesBaseDomain: true,
},
{
name: "Azure",
platformStatus: makePlatformStatus(configv1.AzurePlatformType),
Expand Down Expand Up @@ -255,7 +330,12 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
ic := &operatorv1.IngressController{}
platformStatus := tc.platformStatus.DeepCopy()
if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain); actualResult != true {
ingressConfig := tc.ingressConfig
if ingressConfig == nil {
ingressConfig = &configv1.Ingress{}
}
alreadyAdmitted := false
if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain, ingressConfig, alreadyAdmitted); actualResult != true {
t.Errorf("expected result %v, got %v", true, actualResult)
}
if diff := cmp.Diff(tc.expectedIC, ic); len(diff) != 0 {
Expand Down Expand Up @@ -407,11 +487,37 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
Type: operatorv1.PrivateStrategyType,
}
}

ingressConfigWithDefaultNLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.NLB,
},
},
},
},
}
ingressConfigWithDefaultClassicLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Platform: configv1.IngressPlatformSpec{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSIngressSpec{
Type: configv1.Classic,
},
},
},
},
}
)

testCases := []struct {
name string
ic *operatorv1.IngressController
ingressConfig *configv1.Ingress
expectedIC *operatorv1.IngressController
expectedResult bool
domainMatchesBaseDomain bool
Expand Down Expand Up @@ -458,6 +564,30 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer type changed from NLB to unset, with Classic LB as default",
ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
ingressConfig: ingressConfigWithDefaultClassicLB,
expectedResult: false,
expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer type changed from Classic LB to unset, with NLB as default",
ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(elb())),
ingressConfig: ingressConfigWithDefaultNLB,
expectedResult: false,
expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(elb())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer type changed from NLB to unset, with NLB as default",
ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
ingressConfig: ingressConfigWithDefaultNLB,
expectedResult: false,
expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer ELB connection idle timeout changed from unset with null provider parameters to 2m",
ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithNullParameters())),
Expand Down Expand Up @@ -649,14 +779,35 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &unmanagedDNS)))),
domainMatchesBaseDomain: false,
},
{
name: "when endpointPublishingStrategy is nil and lbType in ingress config is set to Classic",
ic: makeIC(spec(nil), status(nil)),
ingressConfig: ingressConfigWithDefaultClassicLB,
expectedResult: true,
expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS)))),
domainMatchesBaseDomain: true,
},
{
name: "when endpointPublishingStrategy is nil and lbType in ingress config is set to NLB",
ic: makeIC(spec(nil), status(nil)),
ingressConfig: ingressConfigWithDefaultNLB,
expectedResult: true,
expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS)))),
domainMatchesBaseDomain: true,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ic := tc.ic.DeepCopy()
platformStatus := &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
}
if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain); actualResult != tc.expectedResult {
ingressConfig := tc.ingressConfig
if ingressConfig == nil {
ingressConfig = &configv1.Ingress{}
}
alreadyAdmitted := true
if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain, ingressConfig, alreadyAdmitted); actualResult != tc.expectedResult {
t.Errorf("expected result %v, got %v", tc.expectedResult, actualResult)
}
if diff := cmp.Diff(tc.expectedIC, ic); len(diff) != 0 {
Expand Down
Loading