Skip to content
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.

Sign up for GitHub

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

Don't override default values when applying partial features.yaml configmap #7379

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Don't override default values when applying partial features.yaml configmap #7379

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb4e1ad
Applying partial features.yaml configmap overrides default values
pierDipi Oct 18, 2023
0f3e1de
Add all FF and test enabled by default feature
pierDipi Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions pkg/apis/feature/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,27 @@ const (
// Missing entry in the map means feature is equal to feature not enabled.
type Flags map[string]Flag

func newDefaults() Flags {
return map[string]Flag{
KReferenceGroup: Disabled,
DeliveryRetryAfter: Disabled,
DeliveryTimeout: Enabled,
KReferenceMapping: Disabled,
NewTriggerFilters: Enabled,
TransportEncryption: Disabled,
pierDipi marked this conversation as resolved.
Show resolved Hide resolved
}
}

// IsEnabled returns true if the feature is enabled
func (e Flags) IsEnabled(featureName string) bool {
return e != nil && e[featureName] == Enabled
}

// IsDisabled returns true if the feature is disabled
func (e Flags) IsDisabled(featureName string) bool {
return e != nil && e[featureName] == Disabled
}

// IsAllowed returns true if the feature is enabled or allowed
func (e Flags) IsAllowed(featureName string) bool {
return e.IsEnabled(featureName) || (e != nil && e[featureName] == Allowed)
Expand Down Expand Up @@ -86,7 +102,7 @@ func (e Flags) String() string {

// NewFlagsConfigFromMap creates a Flags from the supplied Map
func NewFlagsConfigFromMap(data map[string]string) (Flags, error) {
flags := Flags{}
flags := newDefaults()

for k, v := range data {
if strings.HasPrefix(k, "_") {
Expand All @@ -100,12 +116,12 @@ func NewFlagsConfigFromMap(data map[string]string) (Flags, error) {
flags[sanitizedKey] = Disabled
} else if strings.EqualFold(v, string(Enabled)) {
flags[sanitizedKey] = Enabled
} else if strings.EqualFold(v, string(Permissive)) {
} else if k == TransportEncryption && strings.EqualFold(v, string(Permissive)) {
flags[sanitizedKey] = Permissive
} else if strings.EqualFold(v, string(Strict)) {
} else if k == TransportEncryption && strings.EqualFold(v, string(Strict)) {
flags[sanitizedKey] = Strict
} else {
return Flags{}, fmt.Errorf("cannot parse the boolean flag '%s' = '%s'. Allowed values: [true, false]", k, v)
return flags, fmt.Errorf("cannot parse the feature flag '%s' = '%s'", k, v)
}
}

Expand Down
14 changes: 13 additions & 1 deletion pkg/apis/feature/features_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ import (
"github.com/stretchr/testify/require"
_ "knative.dev/pkg/system/testing"

. "knative.dev/eventing/pkg/apis/feature"
. "knative.dev/pkg/configmap/testing"

. "knative.dev/eventing/pkg/apis/feature"
)

func TestFlags_IsEnabled_NilMap(t *testing.T) {
Expand Down Expand Up @@ -56,3 +57,14 @@ func TestGetFlags(t *testing.T) {
require.True(t, flags.IsAllowed("my-allowed-flag"))
require.False(t, flags.IsAllowed("non-disabled-flag"))
}

func TestShouldNotOverrideDefaults(t *testing.T) {

f, err := NewFlagsConfigFromMap(map[string]string{})
require.Nil(t, err)
require.NotNil(t, f)

if !f.IsDisabled(KReferenceGroup) && !f.IsEnabled(KReferenceGroup) {
pierDipi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@Leo6Leo Leo6Leo Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pierDipi , I've reviewed the part where we call NewFlagsConfigFromMap with an empty string map. If I understand correctly, this will return an empty flag. Wouldn't this potentially cause an issue at line 65? (nvm, it only checks for nil. The flag is empty, and it doesn't mean it is nil)

Also, I noted that with use of KReferenceGroup as a feature flag name. Is this specifically for testing purposes? Why we are using KReferenceGroup? And the if statement here is used to validate that KReferenceGroup only allow values from "Disabled" or "Enabled" right? How can that be related to "TestShouldNotOverrideDefaults"?

Thanks for helping me clarify this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the part where we call NewFlagsConfigFromMap with an empty string map. If I understand correctly, this will return an empty flag. Wouldn't this potentially cause an issue at line 65? (nvm, it only checks for nil. The flag is empty, and it doesn't mean it is nil)

Exactly

Also, I noted that with use of KReferenceGroup as a feature flag name. Is this specifically for testing purposes? Why we are using KReferenceGroup? And the if statement here is used to validate that KReferenceGroup only allow values from "Disabled" or "Enabled" right? How can that be related to "TestShouldNotOverrideDefaults"?

This assertion is the "regression test" for the original issue and if you remove the solution this assertion will fail, there is no specific reason for using that feature flag KReferenceGroup, another one would have worked as well

t.Errorf("Expected default value for %s in flags %+v", KReferenceGroup, f)
}
}
Loading