diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b67a07ea7..37168c8509 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,16 +159,12 @@ Adding a new version? You'll need three changes: - Event messages for invalid multi-Service backends now indicate their derived Kong resource name. [#3318](https://github.com/Kong/kubernetes-ingress-controller/pull/3318) -- `--konnect-sync-enabled` feature flag has been introduced. It enables the - integration with Kong's Konnect cloud. It's turned off by default. - When enabled, it allows synchronising data-plane configuration with - a Konnect Runtime Group specified by `--konnect-runtime-group-id`. - It requires `--konnect-tls-client-*` set of flags to be set to provide - Runtime Group's TLS client certificates for authentication. - [#3455](https://github.com/Kong/kubernetes-ingress-controller/pull/3455) - Removed a duplicated status update of the HTTPRoute, which led to a potential status flickering. [#3451](https://github.com/Kong/kubernetes-ingress-controller/pull/3451) +- Made Admission Webhook fetch the latest list of Gateways to avoid calling + outdated services set statically during the setup. + [#3601](https://github.com/Kong/kubernetes-ingress-controller/pull/3601) ### Under the hood diff --git a/internal/admission/adminapi_provider.go b/internal/admission/adminapi_provider.go new file mode 100644 index 0000000000..217e157082 --- /dev/null +++ b/internal/admission/adminapi_provider.go @@ -0,0 +1,55 @@ +package admission + +import ( + "github.com/kong/go-kong/kong" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/adminapi" +) + +// GatewayClientsProvider returns the most recent set of Gateway Admin API clients. +type GatewayClientsProvider interface { + GatewayClients() []*adminapi.Client +} + +// DefaultAdminAPIServicesProvider allows getting Admin API services that require having at least one Gateway discovered. +// In the case there's no Gateways, it will return `false` from every method, signalling there's no Gateway available. +type DefaultAdminAPIServicesProvider struct { + gatewayClientsProvider GatewayClientsProvider +} + +func NewDefaultAdminAPIServicesProvider(gatewaysProvider GatewayClientsProvider) *DefaultAdminAPIServicesProvider { + return &DefaultAdminAPIServicesProvider{gatewayClientsProvider: gatewaysProvider} +} + +func (p DefaultAdminAPIServicesProvider) GetConsumersService() (kong.AbstractConsumerService, bool) { + c, ok := p.designatedAdminAPIClient() + if !ok { + return nil, ok + } + return c.Consumers, true +} + +func (p DefaultAdminAPIServicesProvider) GetPluginsService() (kong.AbstractPluginService, bool) { + c, ok := p.designatedAdminAPIClient() + if !ok { + return nil, ok + } + return c.Plugins, true +} + +func (p DefaultAdminAPIServicesProvider) designatedAdminAPIClient() (*kong.Client, bool) { + gwClients := p.gatewayClientsProvider.GatewayClients() + if len(gwClients) == 0 { + return nil, false + } + + // For now using first client is kind of OK. Using Consumer and Plugin + // services from first kong client should theoretically return the same + // results as for all other clients. There might be instances where + // configurations in different Kong Gateways are ever so slightly + // different but that shouldn't cause a fatal failure. + // + // TODO: We should take a look at this sooner rather than later. + // https://github.com/Kong/kubernetes-ingress-controller/issues/3363 + return gwClients[0].AdminAPIClient(), true +} diff --git a/internal/admission/adminapi_provider_test.go b/internal/admission/adminapi_provider_test.go new file mode 100644 index 0000000000..d797ee24ef --- /dev/null +++ b/internal/admission/adminapi_provider_test.go @@ -0,0 +1,49 @@ +package admission_test + +import ( + "testing" + + "github.com/samber/lo" + "github.com/stretchr/testify/require" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/adminapi" + "github.com/kong/kubernetes-ingress-controller/v2/internal/admission" +) + +type fakeGatewayClientsProvider struct { + clients []*adminapi.Client +} + +func (f fakeGatewayClientsProvider) GatewayClients() []*adminapi.Client { + return f.clients +} + +func TestDefaultAdminAPIServicesProvider(t *testing.T) { + t.Run("no clients available should return false from methods", func(t *testing.T) { + p := admission.NewDefaultAdminAPIServicesProvider(fakeGatewayClientsProvider{}) + + _, ok := p.GetConsumersService() + require.False(t, ok) + + _, ok = p.GetPluginsService() + require.False(t, ok) + }) + + t.Run("when clients available should return first one", func(t *testing.T) { + firstClient := lo.Must(adminapi.NewTestClient("localhost:8080")) + p := admission.NewDefaultAdminAPIServicesProvider(fakeGatewayClientsProvider{ + clients: []*adminapi.Client{ + firstClient, + lo.Must(adminapi.NewTestClient("localhost:8081")), + }, + }) + + consumersSvc, ok := p.GetConsumersService() + require.True(t, ok) + require.Equal(t, firstClient.AdminAPIClient().Consumers, consumersSvc) + + pluginsSvc, ok := p.GetPluginsService() + require.True(t, ok) + require.Equal(t, firstClient.AdminAPIClient().Plugins, pluginsSvc) + }) +} diff --git a/internal/admission/validator.go b/internal/admission/validator.go index 26265e2454..f3e25edf94 100644 --- a/internal/admission/validator.go +++ b/internal/admission/validator.go @@ -30,14 +30,20 @@ type KongValidator interface { ValidateHTTPRoute(ctx context.Context, httproute gatewaycontroller.HTTPRoute) (bool, string, error) } +// AdminAPIServicesProvider provides KongHTTPValidator with Kong Admin API services that are needed to perform +// validation against entities stored by the Gateway. +type AdminAPIServicesProvider interface { + GetConsumersService() (kong.AbstractConsumerService, bool) + GetPluginsService() (kong.AbstractPluginService, bool) +} + // KongHTTPValidator implements KongValidator interface to validate Kong // entities using the Admin API of Kong. type KongHTTPValidator struct { - ConsumerSvc kong.AbstractConsumerService - PluginSvc kong.AbstractPluginService - Logger logrus.FieldLogger - SecretGetter kongstate.SecretGetter - ManagerClient client.Client + Logger logrus.FieldLogger + SecretGetter kongstate.SecretGetter + ManagerClient client.Client + AdminAPIServicesProvider AdminAPIServicesProvider ingressClassMatcher func(*metav1.ObjectMeta, string, annotations.ClassMatching) bool } @@ -47,19 +53,17 @@ type KongHTTPValidator struct { // such as consumer credentials secrets. If you do not pass a cached client // here, the performance of this validator can get very poor at high scales. func NewKongHTTPValidator( - consumerSvc kong.AbstractConsumerService, - pluginSvc kong.AbstractPluginService, logger logrus.FieldLogger, managerClient client.Client, ingressClass string, + servicesProvider AdminAPIServicesProvider, ) KongHTTPValidator { matcher := annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass) return KongHTTPValidator{ - ConsumerSvc: consumerSvc, - PluginSvc: pluginSvc, - Logger: logger, - SecretGetter: &managerClientSecretGetter{managerClient: managerClient}, - ManagerClient: managerClient, + Logger: logger, + SecretGetter: &managerClientSecretGetter{managerClient: managerClient}, + ManagerClient: managerClient, + AdminAPIServicesProvider: servicesProvider, ingressClassMatcher: matcher, } @@ -84,16 +88,9 @@ func (validator KongHTTPValidator) ValidateConsumer( return false, ErrTextConsumerUsernameEmpty, nil } - // verify that the consumer is not already otherwise present in the data-plane - c, err := validator.ConsumerSvc.Get(ctx, &consumer.Username) - if err != nil { - if !kong.IsNotFoundErr(err) { - validator.Logger.WithError(err).Error("failed to fetch consumer from kong") - return false, ErrTextConsumerUnretrievable, err - } - } - if c != nil { - return false, ErrTextConsumerExists, nil + errText, err := validator.ensureConsumerDoesNotExistInGateway(ctx, consumer.Username) + if err != nil || errText != "" { + return false, errText, err } // if there are no credentials for this consumer, there's no need to move on @@ -254,14 +251,12 @@ func (validator KongHTTPValidator) ValidatePlugin( if len(k8sPlugin.Protocols) > 0 { plugin.Protocols = kong.StringSlice(kongv1.KongProtocolsToStrings(k8sPlugin.Protocols)...) } - isValid, msg, err := validator.PluginSvc.Validate(ctx, &plugin) - if err != nil { - return false, ErrTextPluginConfigValidationFailed, err - } - if !isValid { - return isValid, fmt.Sprintf(ErrTextPluginConfigViolatesSchema, msg), nil + errText, err := validator.validatePluginAgainstGatewaySchema(ctx, plugin) + if err != nil || errText != "" { + return false, errText, err } - return isValid, "", nil + + return true, "", nil } // ValidateClusterPlugin transfers relevant fields from a KongClusterPlugin into a KongPlugin and then returns @@ -395,6 +390,41 @@ func (validator KongHTTPValidator) listManagedConsumers(ctx context.Context) ([] return managedConsumers, nil } +func (validator KongHTTPValidator) ensureConsumerDoesNotExistInGateway(ctx context.Context, username string) (string, error) { + if consumerSvc, hasClient := validator.AdminAPIServicesProvider.GetConsumersService(); hasClient { + // verify that the consumer is not already present in the data-plane + c, err := consumerSvc.Get(ctx, &username) + if err != nil { + if !kong.IsNotFoundErr(err) { + validator.Logger.WithError(err).Error("failed to fetch consumer from kong") + return ErrTextConsumerUnretrievable, err + } + } + if c != nil { + return ErrTextConsumerExists, nil + } + } + + // if there's no client, do not verify existence with data-plane as there's none available + return "", nil +} + +func (validator KongHTTPValidator) validatePluginAgainstGatewaySchema(ctx context.Context, plugin kong.Plugin) (string, error) { + pluginService, hasClient := validator.AdminAPIServicesProvider.GetPluginsService() + if hasClient { + isValid, msg, err := pluginService.Validate(ctx, &plugin) + if err != nil { + return ErrTextPluginConfigValidationFailed, err + } + if !isValid { + return fmt.Sprintf(ErrTextPluginConfigViolatesSchema, msg), nil + } + } + + // if there's no client, do not verify with data-plane as there's none available + return "", nil +} + // ----------------------------------------------------------------------------- // Private - Manager Client Secret Getter // ----------------------------------------------------------------------------- diff --git a/internal/admission/validator_test.go b/internal/admission/validator_test.go index 446a4ff38a..b0ed7d13a8 100644 --- a/internal/admission/validator_test.go +++ b/internal/admission/validator_test.go @@ -3,9 +3,12 @@ package admission import ( "context" "fmt" + "net/http" "testing" "github.com/kong/go-kong/kong" + "github.com/samber/lo" + "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,10 +25,41 @@ type fakePluginSvc struct { valid bool } -func (f *fakePluginSvc) Validate(ctx context.Context, plugin *kong.Plugin) (bool, string, error) { +func (f *fakePluginSvc) Validate(context.Context, *kong.Plugin) (bool, string, error) { return f.valid, f.msg, f.err } +type fakeConsumersSvc struct { + kong.AbstractConsumerService + consumer *kong.Consumer +} + +func (f fakeConsumersSvc) Get(context.Context, *string) (*kong.Consumer, error) { + if f.consumer != nil { + return f.consumer, nil + } + return nil, kong.NewAPIError(http.StatusNotFound, "no consumer found") +} + +type fakeServicesProvider struct { + pluginSvc kong.AbstractPluginService + consumerSvc kong.AbstractConsumerService +} + +func (f fakeServicesProvider) GetConsumersService() (kong.AbstractConsumerService, bool) { + if f.consumerSvc != nil { + return f.consumerSvc, true + } + return nil, false +} + +func (f fakeServicesProvider) GetPluginsService() (kong.AbstractPluginService, bool) { + if f.pluginSvc != nil { + return f.pluginSvc, true + } + return nil, false +} + func TestKongHTTPValidator_ValidatePlugin(t *testing.T) { store, _ := store.NewFakeStore(store.FakeObjects{}) type args struct { @@ -137,8 +171,10 @@ func TestKongHTTPValidator_ValidatePlugin(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { validator := KongHTTPValidator{ - SecretGetter: store, - PluginSvc: tt.PluginSvc, + SecretGetter: store, + AdminAPIServicesProvider: fakeServicesProvider{ + pluginSvc: tt.PluginSvc, + }, ingressClassMatcher: fakeClassMatcher, } got, got1, err := validator.ValidatePlugin(context.Background(), tt.args.plugin) @@ -265,12 +301,24 @@ func TestKongHTTPValidator_ValidateClusterPlugin(t *testing.T) { wantMessage: ErrTextPluginConfigValidationFailed, wantErr: true, }, + { + name: "no gateway was available at the time of validation", + PluginSvc: nil, // no plugin service is available as there's no gateways + args: args{ + plugin: configurationv1.KongClusterPlugin{PluginName: "foo"}, + }, + wantOK: true, + wantMessage: "", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { validator := KongHTTPValidator{ - SecretGetter: store, - PluginSvc: tt.PluginSvc, + SecretGetter: store, + AdminAPIServicesProvider: fakeServicesProvider{ + pluginSvc: tt.PluginSvc, + }, ingressClassMatcher: fakeClassMatcher, } got, got1, err := validator.ValidateClusterPlugin(context.Background(), tt.args.plugin) @@ -288,4 +336,52 @@ func TestKongHTTPValidator_ValidateClusterPlugin(t *testing.T) { } } +func TestKongHTTPValidator_ValidateConsumer(t *testing.T) { + t.Run("passes with and without consumers service available", func(t *testing.T) { + s, _ := store.NewFakeStore(store.FakeObjects{}) + validator := KongHTTPValidator{ + SecretGetter: s, + AdminAPIServicesProvider: fakeServicesProvider{ + consumerSvc: fakeConsumersSvc{consumer: nil}, + }, + ingressClassMatcher: fakeClassMatcher, + } + + valid, errText, err := validator.ValidateConsumer(context.Background(), configurationv1.KongConsumer{ + Username: "username", + }) + require.NoError(t, err) + require.True(t, valid) + require.Empty(t, errText) + + // make services unavailable + validator.AdminAPIServicesProvider = fakeServicesProvider{} + + valid, errText, err = validator.ValidateConsumer(context.Background(), configurationv1.KongConsumer{ + Username: "username", + }) + require.NoError(t, err) + require.True(t, valid) + require.Empty(t, errText) + }) + + t.Run("fails when services available and consumer exists", func(t *testing.T) { + s, _ := store.NewFakeStore(store.FakeObjects{}) + validator := KongHTTPValidator{ + SecretGetter: s, + AdminAPIServicesProvider: fakeServicesProvider{ + consumerSvc: fakeConsumersSvc{consumer: &kong.Consumer{Username: lo.ToPtr("username")}}, + }, + ingressClassMatcher: fakeClassMatcher, + } + + valid, errText, err := validator.ValidateConsumer(context.Background(), configurationv1.KongConsumer{ + Username: "username", + }) + require.NoError(t, err) + require.False(t, valid) + require.Equal(t, ErrTextConsumerExists, errText) + }) +} + func fakeClassMatcher(*metav1.ObjectMeta, string, annotations.ClassMatching) bool { return true } diff --git a/internal/konnect/client.go b/internal/konnect/client.go index ad16d7573f..3b46ef8ed9 100644 --- a/internal/konnect/client.go +++ b/internal/konnect/client.go @@ -169,7 +169,7 @@ func (c *NodeAPIClient) ListAllNodes() ([]*NodeItem, error) { return nil, err } nodes = append(nodes, resp.Items...) - if resp.Page.NextPageNum == 0 { + if resp.Page == nil || resp.Page.NextPageNum == 0 { return nodes, nil } // if konnect returns a non-0 NextPageNum, the node are not all listed diff --git a/internal/manager/run.go b/internal/manager/run.go index ee59f2e38e..096483e354 100644 --- a/internal/manager/run.go +++ b/internal/manager/run.go @@ -93,11 +93,6 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d return fmt.Errorf("unable to start controller manager: %w", err) } - setupLog.Info("Starting Admission Server") - if err := setupAdmissionServer(ctx, c, mgr.GetClient(), deprecatedLogger); err != nil { - return err - } - setupLog.Info("Initializing Dataplane Client") eventRecorder := mgr.GetEventRecorderFor(KongClientEventRecorderComponentName) @@ -115,6 +110,11 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d clientsManager.RunNotifyLoop() } + setupLog.Info("Starting Admission Server") + if err := setupAdmissionServer(ctx, c, clientsManager, mgr.GetClient(), deprecatedLogger); err != nil { + return err + } + dataplaneClient, err := dataplane.NewKongClient( deprecatedLogger, time.Duration(c.ProxyTimeoutSeconds*float32(time.Second)), @@ -215,7 +215,7 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d FeatureGates: featureGates, MeshDetection: len(c.WatchNamespaces) == 0, KonnectSyncEnabled: c.Konnect.ConfigSynchronizationEnabled, - GatewayServiceDiscoveryEnabled: c.KongAdminSvc.String() != "", + GatewayServiceDiscoveryEnabled: c.KongAdminSvc.Name != "", }, ) if err != nil { diff --git a/internal/manager/setup.go b/internal/manager/setup.go index 6187028f49..65ad77af18 100644 --- a/internal/manager/setup.go +++ b/internal/manager/setup.go @@ -157,6 +157,7 @@ func setupDataplaneSynchronizer( func setupAdmissionServer( ctx context.Context, managerConfig *Config, + clientsManager *dataplane.AdminAPIClientsManager, managerClient client.Client, deprecatedLogger logrus.FieldLogger, ) error { @@ -167,26 +168,13 @@ func setupAdmissionServer( return nil } - kongclients, err := managerConfig.adminAPIClients(ctx) - if err != nil { - return err - } - // For now using first client is kind of OK. Using Consumer and Plugin - // services from first kong client should theoretically return the same - // results as for all other clients. There might be instances where - // configurations in different Kong Gateways are ever so slightly - // different but that shouldn't cause a fatal failure. - // - // TODO: We should take a look at this sooner rather than later. - // https://github.com/Kong/kubernetes-ingress-controller/issues/3363 - designatedKongClient := kongclients[0].AdminAPIClient() + adminAPIServicesProvider := admission.NewDefaultAdminAPIServicesProvider(clientsManager) srv, err := admission.MakeTLSServer(ctx, &managerConfig.AdmissionServer, &admission.RequestHandler{ Validator: admission.NewKongHTTPValidator( - designatedKongClient.Consumers, - designatedKongClient.Plugins, logger, managerClient, managerConfig.IngressClassName, + adminAPIServicesProvider, ), Logger: logger, }, logger)