-
Notifications
You must be signed in to change notification settings - Fork 182
CNTRLPLANE-1616: add event-ttl config observer #1938
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||||||||||||||||||||
| package apiserver | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| "github.com/openshift/api/features" | ||||||||||||||||||||||||
| "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation" | ||||||||||||||||||||||||
| "github.com/openshift/library-go/pkg/operator/configobserver" | ||||||||||||||||||||||||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||||||||||||||||||||||||
| "github.com/openshift/library-go/pkg/operator/events" | ||||||||||||||||||||||||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| var eventTTLPath = []string{"apiServerArguments", "event-ttl"} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // NewObserveEventTTL returns a config observation function that observes | ||||||||||||||||||||||||
| // the EventTTLMinutes field from the KubeAPIServer operator CRD | ||||||||||||||||||||||||
| func NewObserveEventTTL(featureGateAccessor featuregates.FeatureGateAccess) configobserver.ObserveConfigFunc { | ||||||||||||||||||||||||
| return (&eventTTLObserver{ | ||||||||||||||||||||||||
| featureGateAccessor: featureGateAccessor, | ||||||||||||||||||||||||
| }).ObserveEventTTL | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| type eventTTLObserver struct { | ||||||||||||||||||||||||
| featureGateAccessor featuregates.FeatureGateAccess | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // ObserveEventTTL reads the eventTTLMinutes from the KubeAPIServer operator CRD | ||||||||||||||||||||||||
| func (o *eventTTLObserver) ObserveEventTTL(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (ret map[string]interface{}, errs []error) { | ||||||||||||||||||||||||
| defer func() { | ||||||||||||||||||||||||
| // Prune the observed config to only include the event-ttl path | ||||||||||||||||||||||||
| ret = configobserver.Pruned(ret, eventTTLPath) | ||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if !o.featureGateAccessor.AreInitialFeatureGatesObserved() { | ||||||||||||||||||||||||
| // if we haven't observed featuregates yet, return the existing | ||||||||||||||||||||||||
| return existingConfig, nil | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| featureGates, err := o.featureGateAccessor.CurrentFeatureGates() | ||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||
| return existingConfig, []error{err} | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if !featureGates.Enabled(features.FeatureEventTTL) { | ||||||||||||||||||||||||
| // Feature disabled: return no opinion so any previously observed value is removed. | ||||||||||||||||||||||||
| // Pruning in defer will ensure only the relevant path is considered. | ||||||||||||||||||||||||
| return map[string]interface{}{}, nil | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Feature Gate Disables Event TTL RemovalWhen the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, the default value is set in the default config of the apiserver. Setting nothing, will set 3h
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually sounds plausible to me. If an admin configures event-ttl through the API, it should be observed and persisted to https://github.com/openshift/api/blob/6a0c921fc0f5b430b07b54578f9961222787f965/operator/v1/types.go#L90. When the effective KAS config is rendered, the values from the observed config will take priority over the static values from the default config ( cluster-kube-apiserver-operator/pkg/operator/targetconfigcontroller/targetconfigcontroller.go Lines 289 to 299 in adc82f6
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a test case for this and explicitly return empty in the existing configuration, in case the feature gate is disabled. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| kubeAPIServer, err := genericListers.(configobservation.Listers).KubeAPIServerOperatorLister().Get("cluster") | ||||||||||||||||||||||||
tjungblu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||
| return existingConfig, []error{err} | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Determine the event TTL value to use | ||||||||||||||||||||||||
| var eventTTLValue string | ||||||||||||||||||||||||
| if kubeAPIServer.Spec.EventTTLMinutes > 0 { | ||||||||||||||||||||||||
| observedConfig := map[string]interface{}{} | ||||||||||||||||||||||||
| // Use the specified value, convert minutes to duration string (e.g., "180m" for 180 minutes) | ||||||||||||||||||||||||
| eventTTLValue = fmt.Sprintf("%dm", kubeAPIServer.Spec.EventTTLMinutes) | ||||||||||||||||||||||||
| if err := unstructured.SetNestedStringSlice(observedConfig, []string{eventTTLValue}, eventTTLPath...); err != nil { | ||||||||||||||||||||||||
| return existingConfig, []error{err} | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return observedConfig, nil | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Use default value from the defaultconfig.yaml when EventTTLMinutes is 0 or not set | ||||||||||||||||||||||||
| return map[string]interface{}{}, nil | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| package apiserver | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| configv1 "github.com/openshift/api/config/v1" | ||
| "github.com/openshift/api/features" | ||
| operatorv1 "github.com/openshift/api/operator/v1" | ||
| operatorlistersv1 "github.com/openshift/client-go/operator/listers/operator/v1" | ||
| "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/tools/cache" | ||
| clocktesting "k8s.io/utils/clock/testing" | ||
| ) | ||
|
|
||
| func TestObserveEventTTL(t *testing.T) { | ||
| scenarios := []struct { | ||
| name string | ||
| existingKubeAPIConfig map[string]interface{} | ||
| expectedKubeAPIConfig map[string]interface{} | ||
| eventTTLMinutes int32 | ||
| featureOn bool | ||
| }{ | ||
| { | ||
| name: "feature gate disabled", | ||
| existingKubeAPIConfig: map[string]interface{}{}, | ||
| expectedKubeAPIConfig: map[string]interface{}{}, | ||
| eventTTLMinutes: 120, | ||
| featureOn: false, | ||
| }, | ||
| { | ||
| name: "feature gate disabled clears existing event-ttl", | ||
| existingKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"120m"}, | ||
| }, | ||
| }, | ||
| expectedKubeAPIConfig: map[string]interface{}{}, | ||
| eventTTLMinutes: 0, | ||
| featureOn: false, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, no event TTL set - use default from defaultconfig.yaml", | ||
| existingKubeAPIConfig: map[string]interface{}{}, | ||
| expectedKubeAPIConfig: map[string]interface{}{}, | ||
| eventTTLMinutes: 0, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, event TTL set to 60 minutes", | ||
| existingKubeAPIConfig: map[string]interface{}{}, | ||
| expectedKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"60m"}, | ||
| }, | ||
| }, | ||
| eventTTLMinutes: 60, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, event TTL set to 180 minutes (maximum)", | ||
| existingKubeAPIConfig: map[string]interface{}{}, | ||
| expectedKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"180m"}, | ||
| }, | ||
| }, | ||
| eventTTLMinutes: 180, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, event TTL set to 5 minutes (minimum)", | ||
| existingKubeAPIConfig: map[string]interface{}{}, | ||
| expectedKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"5m"}, | ||
| }, | ||
| }, | ||
| eventTTLMinutes: 5, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, update existing config", | ||
| existingKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"120m"}, | ||
| }, | ||
| }, | ||
| expectedKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"90m"}, | ||
| }, | ||
| }, | ||
| eventTTLMinutes: 90, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, no change needed", | ||
| existingKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"120m"}, | ||
| }, | ||
| }, | ||
| expectedKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"120m"}, | ||
| }, | ||
| }, | ||
| eventTTLMinutes: 120, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, set default event-ttl when set to 0", | ||
| existingKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"120m"}, | ||
| }, | ||
| }, | ||
| expectedKubeAPIConfig: map[string]interface{}{}, | ||
| eventTTLMinutes: 0, | ||
| featureOn: true, | ||
| }, | ||
| { | ||
| name: "feature gate enabled, no change needed when already at default, returning empty", | ||
| existingKubeAPIConfig: map[string]interface{}{ | ||
| "apiServerArguments": map[string]interface{}{ | ||
| "event-ttl": []interface{}{"3h"}, | ||
| }, | ||
| }, | ||
| expectedKubeAPIConfig: map[string]interface{}{}, | ||
| eventTTLMinutes: 0, | ||
| featureOn: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, scenario := range scenarios { | ||
| t.Run(scenario.name, func(t *testing.T) { | ||
| // test data | ||
| eventRecorder := events.NewInMemoryRecorder("", clocktesting.NewFakePassiveClock(time.Now())) | ||
| kubeAPIServerIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) | ||
|
|
||
| // Add KubeAPIServer resource | ||
| _ = kubeAPIServerIndexer.Add(&operatorv1.KubeAPIServer{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, | ||
| Spec: operatorv1.KubeAPIServerSpec{ | ||
| EventTTLMinutes: scenario.eventTTLMinutes, | ||
| }, | ||
| }) | ||
|
|
||
| listers := configobservation.Listers{ | ||
| KubeAPIServerOperatorLister_: operatorlistersv1.NewKubeAPIServerLister(kubeAPIServerIndexer), | ||
| } | ||
|
|
||
| // Set up feature gate accessor | ||
| var fg featuregates.FeatureGateAccess | ||
| if scenario.featureOn { | ||
| fg = featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{features.FeatureEventTTL}, []configv1.FeatureGateName{}) | ||
| } else { | ||
| fg = featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{}, []configv1.FeatureGateName{features.FeatureEventTTL}) | ||
| } | ||
|
|
||
| observer := NewObserveEventTTL(fg) | ||
| observedKubeAPIConfig, errs := observer(listers, eventRecorder, scenario.existingKubeAPIConfig) | ||
|
|
||
| if len(errs) > 0 { | ||
| t.Fatalf("unexpected errors: %v", errs) | ||
| } | ||
| if diff := cmp.Diff(scenario.expectedKubeAPIConfig, observedKubeAPIConfig); diff != "" { | ||
| t.Fatalf("unexpected configuration, diff = %s", diff) | ||
| } | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,7 @@ func NewConfigObserver(operatorClient v1helpers.StaticPodOperatorClient, kubeInf | |
| ResourceSync: resourceSyncer, | ||
| PreRunCachesSynced: append(preRunCacheSynced, | ||
| operatorClient.Informer().HasSynced, | ||
| operatorInformer.Operator().V1().KubeAPIServers().Informer().HasSynced, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the same as the informer returned by
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, also used in ObserveServiceAccountIssuer that way, which would indicate this one was missing before already
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice find! |
||
|
|
||
| kubeInformersForNamespaces.InformersFor("openshift-etcd").Core().V1().ConfigMaps().Informer().HasSynced, | ||
| kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Informer().HasSynced, | ||
|
|
@@ -125,6 +126,7 @@ func NewConfigObserver(operatorClient v1helpers.StaticPodOperatorClient, kubeInf | |
| apiserver.ObserveSendRetryAfterWhileNotReadyOnce, | ||
| apiserver.ObserveGoawayChance, | ||
| apiserver.ObserveAdmissionPlugins, | ||
| apiserver.NewObserveEventTTL(featureGateAccessor), | ||
| libgoapiserver.ObserveTLSSecurityProfile, | ||
| auth.ObserveAuthMetadata, | ||
| auth.ObserveServiceAccountIssuer, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The 'errs' variable is unused.