Skip to content

Commit

Permalink
Improve feature gate handling in config and runtime (#162)
Browse files Browse the repository at this point in the history
Improved the handling of feature gates in the `config` and `runtime`
packages by updating the validation logic to properly handle errors
when overriding feature gates.

This patch also renams the `ReadOnly` featuregate to
`ReadOnlyResources` to better describe its purpose.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
a-hilaly authored Oct 9, 2024
1 parent 844d1e2 commit b5e7fbe
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 13 deletions.
5 changes: 4 additions & 1 deletion pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,10 @@ func (cfg *Config) Validate(options ...Option) error {
if err != nil {
return fmt.Errorf("invalid value for flag '%s': %v", flagFeatureGates, err)
}
cfg.FeatureGates = featuregate.GetFeatureGatesWithOverrides(featureGatesMap)
cfg.FeatureGates, err = featuregate.GetFeatureGatesWithOverrides(featureGatesMap)
if err != nil {
return fmt.Errorf("error overriding feature gates: %v", err)
}

return nil
}
Expand Down
20 changes: 12 additions & 8 deletions pkg/featuregate/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
// optionally overridden.
package featuregate

import "fmt"

const (
ReadOnly = "ReadOnly"

// ReadOnlyResources is a feature gate for enabling ReadOnly resources annotation.
ReadOnlyResources = "ReadOnlyResources"

// TeamLevelCARM is a feature gate for enabling CARM for team-level resources.
TeamLevelCARM = "TeamLevelCARM"

Expand All @@ -29,10 +32,9 @@ const (
// defaultACKFeatureGates is a map of feature names to Feature structs
// representing the default feature gates for ACK controllers.
var defaultACKFeatureGates = FeatureGates{
// Set feature gates here
ReadOnly: {Stage: Alpha, Enabled: false},
TeamLevelCARM: {Stage: Alpha, Enabled: false},
ServiceLevelCARM: {Stage: Alpha, Enabled: false},
ReadOnlyResources: {Stage: Alpha, Enabled: false},
TeamLevelCARM: {Stage: Alpha, Enabled: false},
ServiceLevelCARM: {Stage: Alpha, Enabled: false},
}

// FeatureStage represents the development stage of a feature.
Expand Down Expand Up @@ -102,13 +104,15 @@ func GetDefaultFeatureGates() FeatureGates {

// GetFeatureGatesWithOverrides returns a new FeatureGates instance with the default features,
// but with the provided overrides applied. This allows for runtime configuration of feature gates.
func GetFeatureGatesWithOverrides(featureGateOverrides map[string]bool) FeatureGates {
func GetFeatureGatesWithOverrides(featureGateOverrides map[string]bool) (FeatureGates, error) {
gates := GetDefaultFeatureGates()
for name, enabled := range featureGateOverrides {
if feature, ok := gates[name]; ok {
feature.Enabled = enabled
gates[name] = feature
} else {
return nil, fmt.Errorf("unknown feature gate: %v", name)
}
}
return gates
return gates, nil
}
6 changes: 4 additions & 2 deletions pkg/featuregate/features_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ package featuregate

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestIsEnabled(t *testing.T) {
Expand Down Expand Up @@ -108,10 +110,10 @@ func TestGetFeatureGatesWithOverrides(t *testing.T) {
overrides := map[string]bool{
"feature1": true,
"feature2": false,
"feature4": true, // This should be ignored as it's not in defaultFeatureGates
}

gates := GetFeatureGatesWithOverrides(overrides)
gates, err := GetFeatureGatesWithOverrides(overrides)
require.Nil(t, err)

tests := []struct {
name string
Expand Down
4 changes: 2 additions & 2 deletions pkg/runtime/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (r *resourceReconciler) reconcile(
if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete &&
// If the ReadOnly feature gate is enabled, and the resource is read-only,
// we don't delete the resource.
!(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnly) && IsReadOnly(res)) {
!(r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) && IsReadOnly(res)) {
// Resolve references before deleting the resource.
// Ignore any errors while resolving the references
resolved, _, _ := rm.ResolveReferences(ctx, r.apiReader, res)
Expand Down Expand Up @@ -360,7 +360,7 @@ func (r *resourceReconciler) Sync(
isAdopted := IsAdopted(desired)
rlog.WithValues("is_adopted", isAdopted)

if r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnly) {
if r.cfg.FeatureGates.IsEnabled(featuregate.ReadOnlyResources) {
isReadOnly := IsReadOnly(desired)
rlog.WithValues("is_read_only", isReadOnly)

Expand Down

0 comments on commit b5e7fbe

Please sign in to comment.