From 4149933ed2ec0585d49444d230e79b44a5f42f40 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 4 Jul 2023 17:49:20 +0200 Subject: [PATCH] kube-apiserver: move "public IP matches IP family" check to option validation --- cmd/kube-apiserver/app/options/validation.go | 26 ++- .../app/options/validation_test.go | 134 ++++++++++++- pkg/controlplane/controller_test.go | 176 ------------------ pkg/controlplane/instance.go | 12 -- 4 files changed, 158 insertions(+), 190 deletions(-) delete mode 100644 pkg/controlplane/controller_test.go diff --git a/cmd/kube-apiserver/app/options/validation.go b/cmd/kube-apiserver/app/options/validation.go index 4767ea5cabe..48461b75001 100644 --- a/cmd/kube-apiserver/app/options/validation.go +++ b/cmd/kube-apiserver/app/options/validation.go @@ -22,9 +22,13 @@ import ( "net" "strings" + genericoptions "k8s.io/apiserver/pkg/server/options" utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/kubernetes/pkg/features" netutils "k8s.io/utils/net" + + controlplaneapiserver "k8s.io/kubernetes/pkg/controlplane/apiserver/options" + "k8s.io/kubernetes/pkg/controlplane/reconcilers" + "k8s.io/kubernetes/pkg/features" ) // TODO: Longer term we should read this from some config store, rather than a flag. @@ -102,6 +106,25 @@ func validateServiceNodePort(options Extra) []error { return errs } +func validatePublicIPServiceClusterIPRangeIPFamilies(extra Extra, generic genericoptions.ServerRunOptions) []error { + // The "kubernetes.default" Service is SingleStack based on the configured ServiceIPRange. + // If the bootstrap controller reconcile the kubernetes.default Service and Endpoints, it must + // guarantee that the Service ClusterIPRepairConfig and the associated Endpoints have the same IP family, or + // it will not work for clients because of the IP family mismatch. + // TODO: revisit for dual-stack https://github.com/kubernetes/enhancements/issues/2438 + if reconcilers.Type(extra.EndpointReconcilerType) != reconcilers.NoneEndpointReconcilerType { + serviceIPRange, _, err := controlplaneapiserver.ServiceIPRange(extra.PrimaryServiceClusterIPRange) + if err != nil { + return []error{fmt.Errorf("error determining service IP ranges: %w", err)} + } + if netutils.IsIPv4CIDR(&serviceIPRange) != netutils.IsIPv4(generic.AdvertiseAddress) { + return []error{fmt.Errorf("service IP family %q must match public address family %q", extra.PrimaryServiceClusterIPRange.String(), generic.AdvertiseAddress.String())} + } + } + + return nil +} + // Validate checks ServerRunOptions and return a slice of found errs. func (s CompletedOptions) Validate() []error { var errs []error @@ -109,6 +132,7 @@ func (s CompletedOptions) Validate() []error { errs = append(errs, s.CompletedOptions.Validate()...) errs = append(errs, validateClusterIPFlags(s.Extra)...) errs = append(errs, validateServiceNodePort(s.Extra)...) + errs = append(errs, validatePublicIPServiceClusterIPRangeIPFamilies(s.Extra, *s.GenericServerRunOptions)...) return errs } diff --git a/cmd/kube-apiserver/app/options/validation_test.go b/cmd/kube-apiserver/app/options/validation_test.go index 71843eca1be..f8454b0ec49 100644 --- a/cmd/kube-apiserver/app/options/validation_test.go +++ b/cmd/kube-apiserver/app/options/validation_test.go @@ -21,10 +21,12 @@ import ( "testing" utilnet "k8s.io/apimachinery/pkg/util/net" + apiserveroptions "k8s.io/apiserver/pkg/server/options" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/kubernetes/pkg/features" netutils "k8s.io/utils/net" + + "k8s.io/kubernetes/pkg/features" ) func makeOptionsWithCIDRs(serviceCIDR string, secondaryServiceCIDR string) *ServerRunOptions { @@ -155,6 +157,136 @@ func TestClusterServiceIPRange(t *testing.T) { } } +func TestValidatePublicIPServiceClusterIPRangeIPFamilies(t *testing.T) { + _, ipv4cidr, err := netutils.ParseCIDRSloppy("192.168.0.0/24") + if err != nil { + t.Fatalf("Unexpected error %v", err) + } + + _, ipv6cidr, err := netutils.ParseCIDRSloppy("2001:db8::/112") + if err != nil { + t.Fatalf("Unexpected error %v", err) + } + + ipv4address := netutils.ParseIPSloppy("192.168.1.1") + ipv6address := netutils.ParseIPSloppy("2001:db8::1") + + tests := []struct { + name string + generic apiserveroptions.ServerRunOptions + extra Extra + wantErr bool + }{ + { + name: "master endpoint reconciler - IPv4 families", + extra: Extra{ + EndpointReconcilerType: "master-count", + PrimaryServiceClusterIPRange: *ipv4cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv4address, + }, + wantErr: false, + }, + { + name: "master endpoint reconciler - IPv6 families", + extra: Extra{ + EndpointReconcilerType: "master-count", + PrimaryServiceClusterIPRange: *ipv6cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv6address, + }, + wantErr: false, + }, + { + name: "master endpoint reconciler - wrong IP families", + extra: Extra{ + EndpointReconcilerType: "master-count", + PrimaryServiceClusterIPRange: *ipv4cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv6address, + }, + wantErr: true, + }, + { + name: "master endpoint reconciler - wrong IP families", + extra: Extra{ + EndpointReconcilerType: "master-count", + PrimaryServiceClusterIPRange: *ipv6cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv4address, + }, + wantErr: true, + }, + { + name: "lease endpoint reconciler - IPv4 families", + extra: Extra{ + EndpointReconcilerType: "lease", + PrimaryServiceClusterIPRange: *ipv4cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv4address, + }, + wantErr: false, + }, + { + name: "lease endpoint reconciler - IPv6 families", + extra: Extra{ + EndpointReconcilerType: "lease", + PrimaryServiceClusterIPRange: *ipv6cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv6address, + }, + wantErr: false, + }, + { + name: "lease endpoint reconciler - wrong IP families", + extra: Extra{ + EndpointReconcilerType: "lease", + PrimaryServiceClusterIPRange: *ipv4cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv6address, + }, + wantErr: true, + }, + { + name: "lease endpoint reconciler - wrong IP families", + extra: Extra{ + EndpointReconcilerType: "lease", + PrimaryServiceClusterIPRange: *ipv6cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv4address, + }, + wantErr: true, + }, + { + name: "none endpoint reconciler - wrong IP families", + extra: Extra{ + EndpointReconcilerType: "none", + PrimaryServiceClusterIPRange: *ipv4cidr, + }, + generic: apiserveroptions.ServerRunOptions{ + AdvertiseAddress: ipv6address, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validatePublicIPServiceClusterIPRangeIPFamilies(tt.extra, tt.generic) + if (len(errs) > 0) != tt.wantErr { + t.Fatalf("completedConfig.New() errors = %+v, wantErr %v", errs, tt.wantErr) + } + }) + } +} + func getIPnetFromCIDR(cidr string) *net.IPNet { _, ipnet, _ := netutils.ParseCIDRSloppy(cidr) return ipnet diff --git a/pkg/controlplane/controller_test.go b/pkg/controlplane/controller_test.go deleted file mode 100644 index d6929d95e1f..00000000000 --- a/pkg/controlplane/controller_test.go +++ /dev/null @@ -1,176 +0,0 @@ -/* -Copyright 2014 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controlplane - -import ( - "testing" - - genericapiserver "k8s.io/apiserver/pkg/server" - "k8s.io/client-go/kubernetes" - netutils "k8s.io/utils/net" - - "k8s.io/kubernetes/pkg/controlplane/reconcilers" -) - -func Test_completedConfig_NewBootstrapController(t *testing.T) { - _, ipv4cidr, err := netutils.ParseCIDRSloppy("192.168.0.0/24") - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - - _, ipv6cidr, err := netutils.ParseCIDRSloppy("2001:db8::/112") - if err != nil { - t.Fatalf("Unexpected error %v", err) - } - - ipv4address := netutils.ParseIPSloppy("192.168.1.1") - ipv6address := netutils.ParseIPSloppy("2001:db8::1") - - type args struct { - client kubernetes.Interface - } - tests := []struct { - name string - config genericapiserver.Config - extraConfig *ExtraConfig - args args - wantErr bool - }{ - { - name: "master endpoint reconciler - IPv4 families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.MasterCountReconcilerType, - ServiceIPRange: *ipv4cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv4address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: false, - }, - { - name: "master endpoint reconciler - IPv6 families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.MasterCountReconcilerType, - ServiceIPRange: *ipv6cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv6address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: false, - }, - { - name: "master endpoint reconciler - wrong IP families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.MasterCountReconcilerType, - ServiceIPRange: *ipv4cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv6address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: true, - }, - { - name: "master endpoint reconciler - wrong IP families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.MasterCountReconcilerType, - ServiceIPRange: *ipv6cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv4address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: true, - }, - { - name: "lease endpoint reconciler - IPv4 families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.LeaseEndpointReconcilerType, - ServiceIPRange: *ipv4cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv4address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: false, - }, - { - name: "lease endpoint reconciler - IPv6 families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.LeaseEndpointReconcilerType, - ServiceIPRange: *ipv6cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv6address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: false, - }, - { - name: "lease endpoint reconciler - wrong IP families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.LeaseEndpointReconcilerType, - ServiceIPRange: *ipv4cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv6address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: true, - }, - { - name: "lease endpoint reconciler - wrong IP families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.LeaseEndpointReconcilerType, - ServiceIPRange: *ipv6cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv4address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: true, - }, - { - name: "none endpoint reconciler - wrong IP families", - extraConfig: &ExtraConfig{ - EndpointReconcilerType: reconcilers.NoneEndpointReconcilerType, - ServiceIPRange: *ipv4cidr, - }, - config: genericapiserver.Config{ - PublicAddress: ipv6address, - SecureServing: &genericapiserver.SecureServingInfo{Listener: fakeLocalhost443Listener{}}, - }, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := &completedConfig{ - GenericConfig: tt.config.Complete(nil), - ExtraConfig: tt.extraConfig, - } - _, err := c.newKubernetesServiceControllerConfig(tt.args.client) - if (err != nil) != tt.wantErr { - t.Errorf("completedConfig.NewBootstrapController() error = %v, wantErr %v", err, tt.wantErr) - return - } - - }) - } -} diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index 64a91b960c8..5ca59e089c1 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -89,7 +89,6 @@ import ( "k8s.io/kubernetes/pkg/routes" "k8s.io/kubernetes/pkg/serviceaccount" "k8s.io/utils/clock" - netutils "k8s.io/utils/net" // RESTStorage installers admissionregistrationrest "k8s.io/kubernetes/pkg/registry/admissionregistration/rest" @@ -631,17 +630,6 @@ func (c completedConfig) newKubernetesServiceControllerConfig(client kubernetes. return nil, fmt.Errorf("failed to get listener address: %w", err) } - // The "kubernetes.default" Service is SingleStack based on the configured ServiceIPRange. - // If the bootstrap controller reconcile the kubernetes.default Service and Endpoints, it must - // guarantee that the Service ClusterIP and the associated Endpoints have the same IP family, or - // it will not work for clients because of the IP family mismatch. - // TODO: revisit for dual-stack https://github.com/kubernetes/enhancements/issues/2438 - if c.ExtraConfig.EndpointReconcilerType != reconcilers.NoneEndpointReconcilerType { - if netutils.IsIPv4CIDR(&c.ExtraConfig.ServiceIPRange) != netutils.IsIPv4(c.GenericConfig.PublicAddress) { - return nil, fmt.Errorf("service IP family %q must match public address family %q", c.ExtraConfig.ServiceIPRange.String(), c.GenericConfig.PublicAddress.String()) - } - } - return &kubernetesservice.Config{ Client: client, Informers: c.ExtraConfig.VersionedInformers,