From 04846d983cfb8ca71f90f7b408d8adf4ce0aa032 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Tue, 22 Aug 2023 08:34:17 -0500 Subject: [PATCH] Revert "Merge pull request #1536 from benluddy/runtime-config" This reverts commit 7864ef22791d34787956e160160654054832864d, reversing changes made to 7d5e2e0213035bfab1d1c2c9633bcd276422c118. --- bindata/assets/config/defaultconfig.yaml | 1 - .../config/bootstrap-config-overrides.yaml | 2 - pkg/cmd/render/render.go | 21 +-- pkg/cmd/render/render_test.go | 44 ++--- .../apienablement/observe_runtime_config.go | 86 --------- .../observe_runtime_config_test.go | 171 ------------------ .../observe_config_controller.go | 5 +- 7 files changed, 14 insertions(+), 316 deletions(-) delete mode 100644 pkg/operator/configobservation/apienablement/observe_runtime_config.go delete mode 100644 pkg/operator/configobservation/apienablement/observe_runtime_config_test.go diff --git a/bindata/assets/config/defaultconfig.yaml b/bindata/assets/config/defaultconfig.yaml index 13e02d8d24..27bf2da5a1 100644 --- a/bindata/assets/config/defaultconfig.yaml +++ b/bindata/assets/config/defaultconfig.yaml @@ -75,7 +75,6 @@ apiServerArguments: - StorageObjectInUseProtection - TaintNodesByCondition - ValidatingAdmissionWebhook - - ValidatingAdmissionPolicy - authorization.openshift.io/RestrictSubjectBindings - authorization.openshift.io/ValidateRoleBindingRestriction - config.openshift.io/DenyDeleteClusterConfiguration diff --git a/bindata/bootkube/config/bootstrap-config-overrides.yaml b/bindata/bootkube/config/bootstrap-config-overrides.yaml index 75e3a3cb63..76a70e2a45 100644 --- a/bindata/bootkube/config/bootstrap-config-overrides.yaml +++ b/bindata/bootkube/config/bootstrap-config-overrides.yaml @@ -63,8 +63,6 @@ apiServerArguments: - /etc/kubernetes/secrets/apiserver-proxy.key requestheader-client-ca-file: - /etc/kubernetes/secrets/aggregator-signer.crt - runtime-config: {{range .RuntimeConfig}} - - {{.}}{{end}} service-account-key-file: - /etc/kubernetes/secrets/service-account.pub - /etc/kubernetes/secrets/bound-service-account-signing-key.pub diff --git a/pkg/cmd/render/render.go b/pkg/cmd/render/render.go index 38cfb265ae..b0c974757f 100644 --- a/pkg/cmd/render/render.go +++ b/pkg/cmd/render/render.go @@ -9,6 +9,7 @@ import ( "encoding/pem" "errors" "fmt" + "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" "io/ioutil" "net" "os" @@ -18,17 +19,14 @@ import ( configv1 "github.com/openshift/api/config/v1" kubecontrolplanev1 "github.com/openshift/api/kubecontrolplane/v1" "github.com/openshift/cluster-kube-apiserver-operator/bindata" - "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/apienablement" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/auth" libgoaudit "github.com/openshift/library-go/pkg/operator/apiserver/audit" - "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" genericrender "github.com/openshift/library-go/pkg/operator/render" genericrenderoptions "github.com/openshift/library-go/pkg/operator/render/options" "github.com/spf13/cobra" "github.com/spf13/pflag" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" kyaml "k8s.io/apimachinery/pkg/util/yaml" auditv1 "k8s.io/apiserver/pkg/apis/audit/v1" "k8s.io/klog/v2" @@ -45,16 +43,10 @@ type renderOpts struct { clusterConfigFile string clusterAuthFile string infraConfigFile string - - groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion } // NewRenderCommand creates a render command. func NewRenderCommand() *cobra.Command { - return newRenderCommand() -} - -func newRenderCommand(testOverrides ...func(*renderOpts)) *cobra.Command { renderOpts := renderOpts{ generic: *genericrenderoptions.NewGenericOptions(), manifest: *genericrenderoptions.NewManifestOptions("kube-apiserver", "openshift/origin-hyperkube:latest"), @@ -63,9 +55,6 @@ func newRenderCommand(testOverrides ...func(*renderOpts)) *cobra.Command { etcdServerURLs: []string{"https://127.0.0.1:2379"}, etcdServingCA: "root-ca.crt", } - for _, f := range testOverrides { - f(&renderOpts) - } cmd := &cobra.Command{ Use: "render", Short: "Render kubernetes API server bootstrap manifests, secrets and configMaps", @@ -137,9 +126,6 @@ func (r *renderOpts) Complete() error { if err := r.generic.Complete(); err != nil { return err } - if r.groupVersionsByFeatureGate == nil { - r.groupVersionsByFeatureGate = apienablement.DefaultGroupVersionsByFeatureGate - } return nil } @@ -162,9 +148,6 @@ type TemplateData struct { // FeatureGates is list of featuregates to apply FeatureGates []string - // RuntimeConfig is a list of API group-versions to enable or disable. - RuntimeConfig []string - // ServiceClusterIPRange is the IP range for service IPs. ServiceCIDR []string @@ -208,8 +191,6 @@ func (r *renderOpts) Run() error { return err } - renderConfig.RuntimeConfig = apienablement.RuntimeConfigFromFeatureGates(featureGates, r.groupVersionsByFeatureGate) - if len(r.clusterConfigFile) > 0 { clusterConfigFileData, err := ioutil.ReadFile(r.clusterConfigFile) if err != nil { diff --git a/pkg/cmd/render/render_test.go b/pkg/cmd/render/render_test.go index 2040d72055..6ca8a127ba 100644 --- a/pkg/cmd/render/render_test.go +++ b/pkg/cmd/render/render_test.go @@ -13,6 +13,7 @@ import ( configv1 "github.com/openshift/api/config/v1" kubecontrolplanev1 "github.com/openshift/api/kubecontrolplane/v1" + "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/configobservercontroller" libgoaudit "github.com/openshift/library-go/pkg/operator/apiserver/audit" "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" genericrenderoptions "github.com/openshift/library-go/pkg/operator/render/options" @@ -20,7 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" kyaml "k8s.io/apimachinery/pkg/util/yaml" ) @@ -228,13 +229,17 @@ func TestRenderCommand(t *testing.T) { } templateDir := filepath.Join("..", "..", "..", "bindata", "bootkube") + tempDisabledFeatureGates := configobservercontroller.FeatureBlacklist + if tempDisabledFeatureGates == nil { + tempDisabledFeatureGates = sets.New[configv1.FeatureGateName]() + } + defaultFGDir := filepath.Join("testdata", "rendered", "default-fg") tests := []struct { // note the name is used as a name for a temporary directory name string args []string - overrides []func(*renderOpts) setupFunction func() error testFunction func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error podTestFunction func(cfg *corev1.Pod) error @@ -249,13 +254,6 @@ func TestRenderCommand(t *testing.T) { "--payload-version=test", "--rendered-manifest-files=" + defaultFGDir, }, - overrides: []func(*renderOpts){ - func(opts *renderOpts) { - opts.groupVersionsByFeatureGate = map[configv1.FeatureGateName][]schema.GroupVersion{ - "Foo": {{Group: "foos.example.com", Version: "v4alpha7"}}, - } - }, - }, testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error { actualGates, ok := cfg.APIServerArguments["feature-gates"] if !ok { @@ -278,21 +276,6 @@ func TestRenderCommand(t *testing.T) { return fmt.Errorf("%q not found on the list of expected feature gates %v", actualGate, expectedGates) } } - - actualRuntimeConfig, ok := cfg.APIServerArguments["runtime-config"] - if !ok { - return fmt.Errorf(`missing expected "runtime-config" entry in APIServerArguments`) - } - expectedRuntimeConfig := []string{"foos.example.com/v4alpha7=true"} - if len(expectedRuntimeConfig) != len(actualRuntimeConfig) { - return fmt.Errorf("expected runtime-config of len %d, got: %v (len %d)", len(expectedRuntimeConfig), actualRuntimeConfig, len(actualRuntimeConfig)) - } - for i := 0; i < len(expectedRuntimeConfig); i++ { - if expectedRuntimeConfig[i] != actualRuntimeConfig[i] { - return fmt.Errorf("expected %dth runtime-config entry %q, got %q", i+1, expectedRuntimeConfig[i], actualRuntimeConfig[i]) - } - } - return nil }, }, @@ -612,7 +595,7 @@ spec: } test.args = setOutputFlags(test.args, outputDir) - err = runRender(test.args, test.overrides) + err = runRender(test.args...) if err != nil { t.Fatalf("%s: got unexpected error %v", test.name, err) } @@ -715,14 +698,9 @@ func setOutputFlags(args []string, dir string) []string { return newArgs } -func runRender(args []string, overrides []func(*renderOpts)) error { - defaultTestOverrides := []func(*renderOpts){ - func(opts *renderOpts) { - opts.groupVersionsByFeatureGate = map[configv1.FeatureGateName][]schema.GroupVersion{} - }, - } - c := newRenderCommand(append(defaultTestOverrides, overrides...)...) - c.SetArgs(args) +func runRender(args ...string) error { + c := NewRenderCommand() + os.Args = append([]string{""}, args...) return c.Execute() } diff --git a/pkg/operator/configobservation/apienablement/observe_runtime_config.go b/pkg/operator/configobservation/apienablement/observe_runtime_config.go deleted file mode 100644 index 3069cc4a5b..0000000000 --- a/pkg/operator/configobservation/apienablement/observe_runtime_config.go +++ /dev/null @@ -1,86 +0,0 @@ -package apienablement - -import ( - "fmt" - "sort" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/sets" - - configv1 "github.com/openshift/api/config/v1" - "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" -) - -var DefaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]schema.GroupVersion{ - "ValidatingAdmissionPolicy": {{Group: "admissionregistration.k8s.io", Version: "v1alpha1"}}, -} - -var ( - featureGatesPath = []string{"apiServerArguments", "feature-gates"} - runtimeConfigPath = []string{"apiServerArguments", "runtime-config"} -) - -// NewFeatureGateObserverWithRuntimeConfig returns a config observation function that observes -// feature gates and sets the --feature-gates and --runtime-config options accordingly. Since a -// mismatch between these two options can result in an unstable config, the observed value for -// either will only be set if both can be successfully set. Otherwise, the existing config is -// returned pruned but otherwise unmodified. -func NewFeatureGateObserverWithRuntimeConfig(featureWhitelist sets.Set[configv1.FeatureGateName], featureBlacklist sets.Set[configv1.FeatureGateName], featureGateAccessor featuregates.FeatureGateAccess, groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion) configobserver.ObserveConfigFunc { - - featureGateObserver := featuregates.NewObserveFeatureFlagsFunc( - featureWhitelist, - featureBlacklist, - featureGatesPath, - featureGateAccessor, - ) - - return newFeatureGateObserverWithRuntimeConfig(featureGateObserver, featureGateAccessor, groupVersionsByFeatureGate) -} - -func newFeatureGateObserverWithRuntimeConfig(featureGateObserver configobserver.ObserveConfigFunc, featureGateAccessor featuregates.FeatureGateAccess, groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion) configobserver.ObserveConfigFunc { - return func(listers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (observedConfig map[string]interface{}, errs []error) { - defer func() { - observedConfig = configobserver.Pruned(observedConfig, featureGatesPath, runtimeConfigPath) - }() - - if !featureGateAccessor.AreInitialFeatureGatesObserved() { - return existingConfig, nil - } - - featureGates, err := featureGateAccessor.CurrentFeatureGates() - if err != nil { - return existingConfig, []error{err} - } - - observedConfig, errs = featureGateObserver(listers, recorder, existingConfig) - - runtimeConfig := RuntimeConfigFromFeatureGates(featureGates, groupVersionsByFeatureGate) - if len(runtimeConfig) == 0 { - return observedConfig, errs - } - - if err := unstructured.SetNestedStringSlice(observedConfig, runtimeConfig, runtimeConfigPath...); err != nil { - // The new feature gate config is broken without its required APIs. - return existingConfig, append(errs, err) - } - - return observedConfig, errs - } -} - -func RuntimeConfigFromFeatureGates(featureGates featuregates.FeatureGate, groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion) []string { - var entries []string - for name, gvs := range groupVersionsByFeatureGate { - if !featureGates.Enabled(name) { - continue - } - for _, gv := range gvs { - entries = append(entries, fmt.Sprintf("%s=true", gv.String())) - } - } - sort.Strings(entries) - return entries -} diff --git a/pkg/operator/configobservation/apienablement/observe_runtime_config_test.go b/pkg/operator/configobservation/apienablement/observe_runtime_config_test.go deleted file mode 100644 index 7f1241b07b..0000000000 --- a/pkg/operator/configobservation/apienablement/observe_runtime_config_test.go +++ /dev/null @@ -1,171 +0,0 @@ -package apienablement - -import ( - "errors" - "testing" - - "github.com/google/go-cmp/cmp" - configv1 "github.com/openshift/api/config/v1" - "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/runtime/schema" -) - -func staticObserver(cfg map[string]interface{}, errs []error) configobserver.ObserveConfigFunc { - return func(configobserver.Listers, events.Recorder, map[string]interface{}) (map[string]interface{}, []error) { - return cfg, errs - } -} - -func TestFeatureGateObserverWithRuntimeConfig(t *testing.T) { - for _, tc := range []struct { - name string - featureGates featuregates.FeatureGateAccess - groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion - delegatedObserver configobserver.ObserveConfigFunc - existingConfig map[string]interface{} - expectedConfig map[string]interface{} - expectedErrors bool - }{ - { - name: "return existing config if initial feature gates not observed", - featureGates: featuregates.NewHardcodedFeatureGateAccessForTesting(nil, nil, make(chan struct{}), nil), - existingConfig: map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedConfig: map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - }, - { - name: "return existing config if error getting current feature gates", - featureGates: featuregates.NewHardcodedFeatureGateAccessForTesting( - nil, - nil, - func() chan struct{} { - c := make(chan struct{}) - close(c) - return c - }(), - errors.New("test"), - ), - existingConfig: map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedConfig: map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedErrors: true, - }, - { - name: "return config directly from feature gate observer if no runtime config applies", - featureGates: featuregates.NewHardcodedFeatureGateAccess(nil, nil), - delegatedObserver: staticObserver( - map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"foo"}, - }, - }, - nil, - ), - existingConfig: map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedConfig: map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"foo"}, - }, - }, - }, - { - name: "return existing config on failure to apply runtime-config", - featureGates: featuregates.NewHardcodedFeatureGateAccess([]configv1.FeatureGateName{"TestFeature"}, nil), - groupVersionsByFeatureGate: map[configv1.FeatureGateName][]schema.GroupVersion{"TestFeature": {{Version: "v6"}}}, - delegatedObserver: staticObserver( - map[string]interface{}{ - "apiServerArguments": int64(42), - }, - nil, - ), - existingConfig: map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedConfig: map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedErrors: true, - }, - { - name: "return config with runtime-config applied", - featureGates: featuregates.NewHardcodedFeatureGateAccess( - []configv1.FeatureGateName{"TestEnabledFeature"}, - []configv1.FeatureGateName{"TestDisabledFeature"}, - ), - groupVersionsByFeatureGate: map[configv1.FeatureGateName][]schema.GroupVersion{ - "TestEnabledFeature": {{Version: "v6"}}, - "TestDisabledFeature": {{Version: "v7"}}, - }, - delegatedObserver: staticObserver( - map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"TestEnabledFeature=true"}, - }, - }, - nil, - ), - existingConfig: map[string]interface{}{ - "prune": "me", - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"keep"}, - "runtime-config": []interface{}{"keep"}, - }, - }, - expectedConfig: map[string]interface{}{ - "apiServerArguments": map[string]interface{}{ - "feature-gates": []interface{}{"TestEnabledFeature=true"}, - "runtime-config": []interface{}{"v6=true"}, - }, - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - actual, errs := newFeatureGateObserverWithRuntimeConfig(tc.delegatedObserver, tc.featureGates, tc.groupVersionsByFeatureGate)(nil, nil, tc.existingConfig) - if diff := cmp.Diff(tc.expectedConfig, actual); diff != "" { - t.Errorf("unexpected config:\n%s", diff) - } - if tc.expectedErrors && len(errs) == 0 { - t.Errorf("expected errors but got none") - } - if !tc.expectedErrors && len(errs) > 0 { - t.Errorf("unexpecteded errors: %v", errs) - } - }) - } -} diff --git a/pkg/operator/configobservation/configobservercontroller/observe_config_controller.go b/pkg/operator/configobservation/configobservercontroller/observe_config_controller.go index 9cddb59c49..e97c28648d 100644 --- a/pkg/operator/configobservation/configobservercontroller/observe_config_controller.go +++ b/pkg/operator/configobservation/configobservercontroller/observe_config_controller.go @@ -20,7 +20,6 @@ import ( "github.com/openshift/library-go/pkg/operator/v1helpers" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation" - "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/apienablement" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/apiserver" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/auth" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation/etcdendpoints" @@ -146,11 +145,11 @@ func NewConfigObserver( []string{"apiServerArguments", "cloud-config"}, featureGateAccessor, ), - apienablement.NewFeatureGateObserverWithRuntimeConfig( + featuregates.NewObserveFeatureFlagsFunc( nil, FeatureBlacklist, + []string{"apiServerArguments", "feature-gates"}, featureGateAccessor, - apienablement.DefaultGroupVersionsByFeatureGate, ), network.ObserveRestrictedCIDRs, network.ObserveServicesSubnet,