Skip to content

Commit

Permalink
Merge pull request #1167 from sadasu/aws-custom-dns
Browse files Browse the repository at this point in the history 
CORS-3755: AWS: Add Ingress LB IPs to Infra CR when in-cluster DNS is enabled
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Nov 22, 2024
2 parents fb5c5d3 + a8096c0 commit 8be1749
Show file tree
Hide file tree
Showing 10 changed files with 171 additions and 1,778 deletions.
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ require (
github.com/summerwind/h2spec v0.0.0-20200804131034-70ac22940108
github.com/tcnksm/go-httpstat v0.2.1-0.20191008022543-e866bb274419
go.uber.org/zap v1.26.0
golang.org/x/exp v0.0.0-20231006140011-7918f672742d
golang.org/x/time v0.3.0
google.golang.org/api v0.126.0
google.golang.org/grpc v1.65.0
Expand Down Expand Up @@ -128,6 +127,7 @@ require (
go.opencensus.io v0.24.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/crypto v0.24.0 // indirect
golang.org/x/exp v0.0.0-20231006140011-7918f672742d // indirect
golang.org/x/net v0.26.0 // indirect
golang.org/x/oauth2 v0.21.0 // indirect
golang.org/x/sync v0.7.0 // indirect
Expand Down
63 changes: 49 additions & 14 deletions pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ingress
import (
"context"
"fmt"
"net"
"regexp"
"regexp/syntax"
"strings"
Expand Down Expand Up @@ -475,10 +476,17 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
}

// When the platform's default DNS solution cannot be used, set the DNSManagementPolicy
// accordingly. This feature is currently being implemented first for GCP. Will be
// extended to AWS and Azure platforms later.
if platformStatus.Type == configv1.GCPPlatformType && platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil {
if platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
// accordingly. This feature is currently being implemented for GCP and AWS. Will be
// extended to the Azure platform later.
switch platformStatus.Type {
case configv1.AWSPlatformType:
if platformStatus.AWS != nil && platformStatus.AWS.CloudLoadBalancerConfig != nil &&
platformStatus.AWS.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}
case configv1.GCPPlatformType:
if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil &&
platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}
}
Expand Down Expand Up @@ -1272,18 +1280,51 @@ func (r *reconciler) allRouterPodsDeleted(ingress *operatorv1.IngressController)
return true, nil
}

// computeUpdatedInfraFromService updates GCP's PlatformStatus with Ingress LB IPs when the DNSType is `ClusterHosted`.
// computeUpdatedInfraFromService updates PlatformStatus for GCP and AWS with Ingress LB IPs when the DNSType is `ClusterHosted`.
func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *configv1.Infrastructure) (bool, error) {
platformStatus := infraConfig.Status.PlatformStatus
if platformStatus == nil {
return false, fmt.Errorf("invalid PlatformStatus within Infrastructure config")
}
ipCmpOpts := []cmp.Option{
cmpopts.EquateEmpty(),
cmpopts.SortSlices(func(a, b configv1.IP) bool {
return a < b
}),
}
// The cluster has to run its own CoreDNS pod for DNS. Update Infra CR with the Ingress LB IPs.
// These values are used to configure the in-cluster DNS to provide resolution for *.apps.
switch platformStatus.Type {
case configv1.AWSPlatformType:
// On the AWS platform, only the Ingress LB's Hostname is available.
// Resolve this Hostname to its IPs before adding to the Infra CR.
if platformStatus.AWS != nil && platformStatus.AWS.CloudLoadBalancerConfig != nil && platformStatus.AWS.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
if platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted == nil {
platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
}
ingresses := service.Status.LoadBalancer.Ingress
ingressLBIPs := []configv1.IP{}
for _, ingress := range ingresses {
// Resolving the LoadBalancer's IPs is not ideal because they may change, but currently there is no better alternative.
ingressIPs, err := net.LookupIP(ingress.Hostname)
if err != nil {
return false, fmt.Errorf("failed to lookup IP addresses corresponding to AWS LB hostname: %w", err)
}

if len(ingressIPs) > 0 {
for _, ingressIP := range ingressIPs {
ingressLBIPs = append(ingressLBIPs, configv1.IP(ingressIP.String()))
}
}
}
if !cmp.Equal(platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
return true, nil
}
}
return false, nil
case configv1.GCPPlatformType:
if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil && platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
// The cluster has to run its own CoreDNS pod for DNS. Update Infra CR
// with the Ingress LB IPs. These values are used to configure the
// in-cluster DNS to provide resolution for *.apps.
if platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted == nil {
platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
}
Expand All @@ -1294,12 +1335,6 @@ func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *config
ingressLBIPs = append(ingressLBIPs, configv1.IP(ingress.IP))
}
}
ipCmpOpts := []cmp.Option{
cmpopts.EquateEmpty(),
cmpopts.SortSlices(func(a, b configv1.IP) bool {
return a < b
}),
}
if !cmp.Equal(platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
return true, nil
Expand Down
134 changes: 121 additions & 13 deletions pkg/operator/controller/ingress/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"golang.org/x/exp/slices"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
Expand Down Expand Up @@ -215,6 +214,29 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
}
}

makeDefaultAWSPlatformStatus = func(platform configv1.PlatformType) *configv1.PlatformStatus {
return &configv1.PlatformStatus{
Type: platform,
AWS: &configv1.AWSPlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.PlatformDefaultDNSType,
},
},
}
}

makeBYODNSAWSPlatformStatus = func(platform configv1.PlatformType) *configv1.PlatformStatus {
return &configv1.PlatformStatus{
Type: platform,
AWS: &configv1.AWSPlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
ClusterHosted: &configv1.CloudLoadBalancerIPs{},
},
},
}
}

ingressConfigWithDefaultClassicLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Expand Down Expand Up @@ -292,6 +314,18 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
expectedIC: ingressControllerWithHostNetwork,
domainMatchesBaseDomain: true,
},
{
name: "AWS",
platformStatus: makeDefaultAWSPlatformStatus(configv1.AWSPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
domainMatchesBaseDomain: true,
},
{
name: "AWS With BYO DNS",
platformStatus: makeBYODNSAWSPlatformStatus(configv1.AWSPlatformType),
expectedIC: ingressControllerWithLoadBalancerUnmanagedDNS,
domainMatchesBaseDomain: true,
},
{
name: "GCP",
platformStatus: makeDefaultGCPPlatformStatus(configv1.GCPPlatformType),
Expand Down Expand Up @@ -1582,9 +1616,34 @@ func Test_IsProxyProtocolNeeded(t *testing.T) {

func Test_computeUpdatedInfraFromService(t *testing.T) {
var (
IngressLBIP = configv1.IP("196.78.125.4")
awsPlatform = configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
}
awsPlatformWithDNSType = configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSPlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
},
},
}
awsPlatformWithLBIP = configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
AWS: &configv1.AWSPlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
ClusterHosted: &configv1.CloudLoadBalancerIPs{
IngressLoadBalancerIPs: []configv1.IP{
IngressLBIP,
},
},
},
},
}
azurePlatform = configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
}
gcpPlatform = configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
}
Expand All @@ -1596,10 +1655,6 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
},
},
}
ingresses = []corev1.LoadBalancerIngress{
{IP: "196.78.125.4"},
}
IngressLBIP = configv1.IP("196.78.125.4")
gcpPlatformWithLBIP = configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
GCP: &configv1.GCPPlatformStatus{
Expand All @@ -1613,16 +1668,26 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
},
},
}
ingresses = []corev1.LoadBalancerIngress{
{IP: "196.78.125.4"},
}
ingressesWithMultipleIPs = []corev1.LoadBalancerIngress{
{IP: "196.78.125.4"},
{IP: "10.10.10.4"},
}
// Hostname is intentionally assigned an IP address for unit testing purposes since net.LookupIP simply returns the provided IP.
awsIngresses = []corev1.LoadBalancerIngress{
{Hostname: "196.78.125.4"},
}
awsUpdatedIngresses = []corev1.LoadBalancerIngress{
{Hostname: "10.10.10.4"},
}
)
testCases := []struct {
description string
platform *configv1.PlatformStatus
ingresses []corev1.LoadBalancerIngress
expectedInfra configv1.Infrastructure
expectedLBIPs []configv1.IP
expectUpdated bool
expectError bool
}{
Expand All @@ -1635,7 +1700,7 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
},
{
description: "unsupported platform should not cause an error",
platform: &awsPlatform,
platform: &azurePlatform,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
Expand All @@ -1658,20 +1723,61 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
description: "gcp platform with DNSType and no LB IP in infra config, service has 1 IP",
platform: &gcpPlatformWithDNSType,
ingresses: ingresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: true,
expectError: false,
},
{
description: "gcp platform with no change to LB IPs",
platform: &gcpPlatformWithLBIP,
ingresses: ingresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: false,
expectError: false,
},
{
description: "gcp platform with DNSType and LB IP",
platform: &gcpPlatformWithLBIP,
ingresses: ingressesWithMultipleIPs,
expectedLBIPs: []configv1.IP{IngressLBIP, configv1.IP("10.10.10.4")},
expectUpdated: true,
expectError: false,
},
{
description: "aws platform without DNSType set",
platform: &awsPlatform,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
},
{
description: "aws platform with DNSType and no LB IP",
platform: &awsPlatformWithDNSType,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
},
{
description: "aws platform with DNSType and no LB IP in infra config, service has 1 IP",
platform: &awsPlatformWithDNSType,
ingresses: awsIngresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: true,
expectError: false,
},
{
description: "aws platform with no change to LB IPs",
platform: &awsPlatformWithLBIP,
ingresses: awsIngresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: false,
expectError: false,
},
{
description: "aws platform with 1 LB IP with change in hostname",
platform: &awsPlatformWithLBIP,
ingresses: awsUpdatedIngresses,
expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4")},
expectUpdated: true,
expectError: false,
},
Expand All @@ -1698,12 +1804,14 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
t.Errorf("expected %t, got %t", tc.expectUpdated, updated)
}
if updated {
ingressLBs := service.Status.LoadBalancer.Ingress
for _, ingress := range ingressLBs {
if len(ingress.IP) > 0 {
if !slices.Contains(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, configv1.IP(ingress.IP)) {
t.Errorf("expected Infra CR to contain %s", ingress.IP)
}
switch tc.platform.Type {
case configv1.AWSPlatformType:
if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {
t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)
}
case configv1.GCPPlatformType:
if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {
t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)
}
}
}
Expand Down
50 changes: 0 additions & 50 deletions vendor/golang.org/x/exp/constraints/constraints.go
View file

This file was deleted.

Loading

0 comments on commit 8be1749

Please sign in to comment.