Skip to content

Vendor bump and fixups #1

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

Closed
Miciah wants to merge 3 commits into from
Closed

Vendor bump and fixups #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bb32ad
Use ingress config to set default LB type on AWS
miheer Jul 4, 2022
73211ef
fixup
Miciah Oct 13, 2022
bf864a7
Bump openshift/api for loadbalancer → loadBalancer
Miciah Oct 13, 2022
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ require (
github.com/go-logr/zapr v1.2.3
github.com/google/go-cmp v0.5.8
github.com/kevinburke/go-bindata v3.11.0+incompatible
github.com/openshift/api v0.0.0-20220907152121-48d78630feb3
github.com/openshift/api v0.0.0-20220927015011-24ee13c10f3b
Copy link
Copy Markdown
Owner

@gcs278 gcs278 Oct 13, 2022

Choose a reason for hiding this comment

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

Miheer original vendor the merge commit github.com/openshift/api v0.0.0-20220929023536-18c298295790 but look like you took the individual commit. Does that matter?

github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.12.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,8 +539,8 @@ github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7J
github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
github.com/onsi/gomega v1.10.5/go.mod h1:gza4q3jKQJijlu05nKWRCW/GavJumGt8aNRxWg7mt48=
github.com/onsi/gomega v1.20.1 h1:PA/3qinGoukvymdIDV8pii6tiZgC8kbmJO6Z5+b002Q=
github.com/openshift/api v0.0.0-20220907152121-48d78630feb3 h1:7y9VOklNufIkJ8pQprlrMq908m16iO11MnP4Ut1rS/E=
github.com/openshift/api v0.0.0-20220907152121-48d78630feb3/go.mod h1:9JWn+H7X8wEPPc9D63krigXl8r3F1Mt6/lC98brUyhQ=
github.com/openshift/api v0.0.0-20220927015011-24ee13c10f3b h1:nRP5WLPvPdqgYBGa6uouWMj0AsOI/oYAxinCEhvGeMA=
github.com/openshift/api v0.0.0-20220927015011-24ee13c10f3b/go.mod h1:HJAEIh4gLXPDdWxgCbvmJjzd9QIxyPZJtPU0u2W4vH4=
github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b h1:LWwB7uN91G/JsMnZFd0+q6ZzAXlB4/oUOfpZWA585gw=
github.com/openshift/library-go v0.0.0-20220920133651-093893cf326b/go.mod h1:KPBAXGaq7pPmA+1wUVtKr5Axg3R68IomWDkzaOxIhxM=
github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=
Expand Down
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 @@ -264,8 +264,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 @@ -319,7 +320,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 @@ -328,7 +329,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 @@ -409,7 +410,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 @@ -440,6 +441,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 @@ -518,13 +522,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 @@ -551,18 +550,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 @@ -632,6 +630,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