Skip to content

Bug 2041616: Do not create DNS records for IngressControllers with domain not matching baseDomain #761

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-ci merged 1 commit into from
Jun 22, 2022
Merged

Bug 2041616: Do not create DNS records for IngressControllers with domain not matching baseDomain #761

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
32 changes: 21 additions & 11 deletions pkg/operator/controller/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
logf "github.com/openshift/cluster-ingress-operator/pkg/log"
"github.com/openshift/cluster-ingress-operator/pkg/manifests"
operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
oputil "github.com/openshift/cluster-ingress-operator/pkg/util"
retryable "github.com/openshift/cluster-ingress-operator/pkg/util/retryableerror"
"github.com/openshift/cluster-ingress-operator/pkg/util/slice"

Expand Down Expand Up @@ -255,11 +256,15 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, networkConfig); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to get network 'cluster': %v", err)
}
platformStatus, err := oputil.GetPlatformStatus(r.client, infraConfig)
if err != nil {
return reconcile.Result{}, fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %w", ingress.Namespace, ingress.Name, err)
}

// 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, infraConfig); err != nil {
if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig); err != nil {
switch err := err.(type) {
case *admissionRejection:
r.recorder.Event(ingress, "Warning", "Rejected", err.Reason)
Expand All @@ -286,7 +291,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
}

// The ingresscontroller is safe to process, so ensure it.
if err := r.ensureIngressController(ingress, dnsConfig, infraConfig, ingressConfig, apiConfig, networkConfig); err != nil {
if err := r.ensureIngressController(ingress, dnsConfig, infraConfig, platformStatus, ingressConfig, apiConfig, networkConfig); err != nil {
switch e := err.(type) {
case retryable.Error:
log.Error(e, "got retryable error; requeueing", "after", e.After())
Expand All @@ -302,11 +307,11 @@ 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, infraConfig *configv1.Infrastructure) error {
func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS) error {
updated := current.DeepCopy()

setDefaultDomain(updated, ingressConfig)
setDefaultPublishingStrategy(updated, infraConfig)
setDefaultPublishingStrategy(updated, platformStatus)

// 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 @@ -337,6 +342,11 @@ func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig
Reason: "Valid",
})
updated.Status.ObservedGeneration = updated.Generation

if !manageDNSForDomain(updated.Status.Domain, platformStatus, dnsConfig) {
r.recorder.Eventf(updated, "Warning", "DomainNotMatching", fmt.Sprintf("Domain [%s] of ingresscontroller does not match the baseDomain [%s] of the cluster DNS config, so DNS management is not supported.", updated.Status.Domain, dnsConfig.Spec.BaseDomain))
}

if !IngressStatusesEqual(current.Status, updated.Status) {
if err := r.client.Status().Update(context.TODO(), updated); err != nil {
return fmt.Errorf("failed to update status: %v", err)
Expand Down Expand Up @@ -382,11 +392,11 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1.
return false
}

func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig *configv1.Infrastructure) bool {
func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus) bool {
effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy()
if effectiveStrategy == nil {
var strategyType operatorv1.EndpointPublishingStrategyType
switch infraConfig.Status.Platform {
switch platformStatus.Type {
case configv1.AWSPlatformType, configv1.AzurePlatformType, configv1.GCPPlatformType, configv1.IBMCloudPlatformType, configv1.PowerVSPlatformType, configv1.AlibabaCloudPlatformType:
strategyType = operatorv1.LoadBalancerServiceStrategyType
case configv1.LibvirtPlatformType:
Expand Down Expand Up @@ -841,7 +851,7 @@ func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController)
// given ingresscontroller. Any error values are collected into either a
// retryable.Error value, if any of the error values are retryable, or else an
// Aggregate error value.
func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network) error {
func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure, platformStatus *configv1.PlatformStatus, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network) error {
// Before doing anything at all with the controller, ensure it has a finalizer
// so we can clean up later.
if !slice.ContainsString(ci.Finalizers, manifests.IngressControllerFinalizer) {
Expand Down Expand Up @@ -892,7 +902,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
haveClientCAConfigmap = true
}

haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap)
haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus)
if err != nil {
errs = append(errs, fmt.Errorf("failed to ensure deployment: %v", err))
return utilerrors.NewAggregate(errs)
Expand All @@ -912,11 +922,11 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d

var lbService *corev1.Service
var wildcardRecord *iov1.DNSRecord
if haveLB, lb, err := r.ensureLoadBalancerService(ci, deploymentRef, infraConfig); err != nil {
if haveLB, lb, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus); err != nil {
errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err))
} else {
lbService = lb
if _, record, err := r.ensureWildcardDNSRecord(ci, lbService, haveLB); err != nil {
if _, record, err := r.ensureWildcardDNSRecord(ci, platformStatus, dnsConfig, lbService, haveLB); err != nil {
errs = append(errs, fmt.Errorf("failed to ensure wildcard dnsrecord for %s: %v", ci.Name, err))
} else {
wildcardRecord = record
Expand Down Expand Up @@ -951,7 +961,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err))
}

syncStatusErr, updated := r.syncIngressControllerStatus(ci, deployment, deploymentRef, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig, infraConfig)
syncStatusErr, updated := r.syncIngressControllerStatus(ci, deployment, deploymentRef, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig, platformStatus)
errs = append(errs, syncStatusErr)

// If syncIngressControllerStatus updated our ingress status, it's important we query for that new object.
Expand Down
110 changes: 53 additions & 57 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 @@ -135,96 +135,94 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
},
},
}
makeInfra = func(platform configv1.PlatformType) *configv1.Infrastructure {
return &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
Platform: platform,
},
makePlatformStatus = func(platform configv1.PlatformType) *configv1.PlatformStatus {
return &configv1.PlatformStatus{
Type: platform,
}
}
)

testCases := []struct {
name string
infraConfig *configv1.Infrastructure
expectedIC *operatorv1.IngressController
name string
platformStatus *configv1.PlatformStatus
expectedIC *operatorv1.IngressController
}{
{
name: "Alibaba",
infraConfig: makeInfra(configv1.AlibabaCloudPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "Alibaba",
platformStatus: makePlatformStatus(configv1.AlibabaCloudPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "AWS",
infraConfig: makeInfra(configv1.AWSPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "AWS",
platformStatus: makePlatformStatus(configv1.AWSPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "Azure",
infraConfig: makeInfra(configv1.AzurePlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "Azure",
platformStatus: makePlatformStatus(configv1.AzurePlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "Bare metal",
infraConfig: makeInfra(configv1.BareMetalPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "Bare metal",
platformStatus: makePlatformStatus(configv1.BareMetalPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "Equinix Metal",
infraConfig: makeInfra(configv1.EquinixMetalPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "Equinix Metal",
platformStatus: makePlatformStatus(configv1.EquinixMetalPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "GCP",
infraConfig: makeInfra(configv1.GCPPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "GCP",
platformStatus: makePlatformStatus(configv1.GCPPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "IBM Cloud",
infraConfig: makeInfra(configv1.IBMCloudPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "IBM Cloud",
platformStatus: makePlatformStatus(configv1.IBMCloudPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "Libvirt",
infraConfig: makeInfra(configv1.LibvirtPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "Libvirt",
platformStatus: makePlatformStatus(configv1.LibvirtPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "No platform",
infraConfig: makeInfra(configv1.NonePlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "No platform",
platformStatus: makePlatformStatus(configv1.NonePlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "OpenStack",
infraConfig: makeInfra(configv1.OpenStackPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "OpenStack",
platformStatus: makePlatformStatus(configv1.OpenStackPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "Power VS",
infraConfig: makeInfra(configv1.PowerVSPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
name: "Power VS",
platformStatus: makePlatformStatus(configv1.PowerVSPlatformType),
expectedIC: ingressControllerWithLoadBalancer,
},
{
name: "RHV",
infraConfig: makeInfra(configv1.OvirtPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "RHV",
platformStatus: makePlatformStatus(configv1.OvirtPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "vSphere",
infraConfig: makeInfra(configv1.VSpherePlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "vSphere",
platformStatus: makePlatformStatus(configv1.VSpherePlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
{
name: "Nutanix",
infraConfig: makeInfra(configv1.NutanixPlatformType),
expectedIC: ingressControllerWithHostNetwork,
name: "Nutanix",
platformStatus: makePlatformStatus(configv1.NutanixPlatformType),
expectedIC: ingressControllerWithHostNetwork,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ic := &operatorv1.IngressController{}
infraConfig := tc.infraConfig.DeepCopy()
if actualResult := setDefaultPublishingStrategy(ic, infraConfig); actualResult != true {
platformStatus := tc.platformStatus.DeepCopy()
if actualResult := setDefaultPublishingStrategy(ic, platformStatus); 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 @@ -506,12 +504,10 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ic := tc.ic.DeepCopy()
infraConfig := &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
Platform: configv1.NonePlatformType,
},
platformStatus := &configv1.PlatformStatus{
Type: configv1.NonePlatformType,
}
if actualResult := setDefaultPublishingStrategy(ic, infraConfig); actualResult != tc.expectedResult {
if actualResult := setDefaultPublishingStrategy(ic, platformStatus); 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
10 changes: 2 additions & 8 deletions pkg/operator/controller/ingress/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import (

"github.com/openshift/cluster-ingress-operator/pkg/manifests"
"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
oputil "github.com/openshift/cluster-ingress-operator/pkg/util"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"

Expand Down Expand Up @@ -109,16 +107,12 @@ const (

// ensureRouterDeployment ensures the router deployment exists for a given
// ingresscontroller.
func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap) (bool, *appsv1.Deployment, error) {
func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus) (bool, *appsv1.Deployment, error) {
haveDepl, current, err := r.currentRouterDeployment(ci)
if err != nil {
return false, nil, err
}
platform, err := oputil.GetPlatformStatus(r.client, infraConfig)
if err != nil {
return false, nil, fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err)
}
proxyNeeded, err := IsProxyProtocolNeeded(ci, platform)
proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus)
if err != nil {
return false, nil, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err)
}
Expand Down
30 changes: 26 additions & 4 deletions pkg/operator/controller/ingress/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ package ingress
import (
"context"
"fmt"
"strings"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
iov1 "github.com/openshift/api/operatoringress/v1"
"github.com/openshift/cluster-ingress-operator/pkg/manifests"
"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"

operatorv1 "github.com/openshift/api/operator/v1"

corev1 "k8s.io/api/core/v1"

"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -29,11 +29,15 @@ const defaultRecordTTL int64 = 30

// ensureWildcardDNSRecord will create DNS records for the given LB service.
// If service is nil (haveLBS is false), nothing is done.
func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service, haveLBS bool) (bool, *iov1.DNSRecord, error) {
func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS, service *corev1.Service, haveLBS bool) (bool, *iov1.DNSRecord, error) {
if !haveLBS {
return false, nil, nil
}

if !manageDNSForDomain(ic.Status.Domain, platformStatus, dnsConfig) {
return false, nil, nil
}

wantWC, desired := desiredWildcardDNSRecord(ic, service)
haveWC, current, err := r.currentWildcardDNSRecord(ic)
if err != nil {
Expand Down Expand Up @@ -189,3 +193,21 @@ func dnsRecordChanged(current, expected *iov1.DNSRecord) (bool, *iov1.DNSRecord)
updated.Spec = expected.Spec
return true, updated
}

// manageDNSForDomain returns true if the given domain contains the baseDomain
// of the cluster DNS config. It is only used for AWS in the beginning, and will be expanded to other clouds
// once we know there are no users depending on this.
// See https://bugzilla.redhat.com/show_bug.cgi?id=2041616
func manageDNSForDomain(domain string, status *configv1.PlatformStatus, dnsConfig *configv1.DNS) bool {
if len(domain) == 0 {
return false
}

mustContain := "." + dnsConfig.Spec.BaseDomain
switch status.Type {
case configv1.AWSPlatformType:
return strings.HasSuffix(domain, mustContain)
default:
return true
}
}
Loading