diff --git a/CHANGELOG.md b/CHANGELOG.md index e1956153..547d6ae0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed + +- Address issue when Grafana feature flags which were enabled via the `feature_flags.enabled` were only properly being + parsed, when they were space-delimited. This fix allows them to be _either_ space or comma-delimited. + by @joeyorlando ([#2623](https://github.com/grafana/oncall/pull/2623)) + ### Added - Added banner on the ChatOps screen for OSS to let the user know if no chatops integration is enabled diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index 861cba89..2641821b 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -270,6 +270,11 @@ class GcomAPIClient(APIClient): data, _ = self.api_get(url) return data + def _feature_is_enabled_via_enable_key( + self, instance_feature_toggles: GCOMInstanceInfoConfigFeatureToggles, feature_name: str, delimiter: str + ): + return feature_name in instance_feature_toggles.get("enable", "").split(delimiter) + def _feature_toggle_is_enabled(self, instance_info: GCOMInstanceInfo, feature_name: str) -> bool: """ there are two ways that feature toggles can be enabled, this method takes into account both @@ -284,12 +289,22 @@ class GcomAPIClient(APIClient): if not instance_feature_toggles: return False - # features enabled via enable key are comma separated (https://github.com/grafana/grafana/issues/36511) - features_enabled_via_enable_key = instance_feature_toggles.get("enable", "").split(",") - feature_enabled_via_enable_key = feature_name in features_enabled_via_enable_key + # features enabled via enable key can be either space or comma delimited + # https://raintank-corp.slack.com/archives/C036J5B39/p1690183217162019 + + feature_enabled_via_enable_key_space_delimited = self._feature_is_enabled_via_enable_key( + instance_feature_toggles, feature_name, " " + ) + feature_enabled_via_enable_key_comma_delimited = self._feature_is_enabled_via_enable_key( + instance_feature_toggles, feature_name, "," + ) feature_enabled_via_direct_key = instance_feature_toggles.get(feature_name, "false") == "true" - return feature_enabled_via_enable_key or feature_enabled_via_direct_key + return ( + feature_enabled_via_direct_key + or feature_enabled_via_enable_key_space_delimited + or feature_enabled_via_enable_key_comma_delimited + ) def is_rbac_enabled_for_stack(self, stack_id: str) -> bool: """ diff --git a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py index 395d9b88..249fbe46 100644 --- a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py @@ -30,6 +30,26 @@ class TestIsRbacEnabledForStack: assert api_client.is_rbac_enabled_for_stack(stack_id) == expected assert mocked_gcom_api_client_api_get.called_once_with(f"instances/{stack_id}?config=true") + @pytest.mark.parametrize( + "instance_info_feature_toggles,delimiter,expected", + [ + ({}, " ", False), + ({"enable": "foo,bar,baz"}, " ", False), + ({"enable": "foo,bar,baz"}, ",", False), + ({"enable": f"foo,bar,baz{TEST_FEATURE_TOGGLE}"}, " ", False), + ({"enable": f"foo,bar,baz{TEST_FEATURE_TOGGLE}"}, ",", False), + ({"enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}abc"}, ",", False), + ({"enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}"}, ",", True), + ], + ) + def test_feature_is_enabled_via_enable_key(self, instance_info_feature_toggles, delimiter, expected) -> None: + assert ( + GcomAPIClient("someFakeApiToken")._feature_is_enabled_via_enable_key( + instance_info_feature_toggles, self.TEST_FEATURE_TOGGLE, delimiter + ) + == expected + ) + @pytest.mark.parametrize( "instance_info,expected", [ @@ -38,9 +58,12 @@ class TestIsRbacEnabledForStack: ({"config": {"feature_toggles": {}}}, False), ({"config": {"feature_toggles": {"enable": "foo,bar,baz"}}}, False), ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "false"}}}, False), - # must be comma separated ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE}baz"}}}, False), - # these cases will probably never happen, but lets account for them anyways + ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE},baz"}}}, True), + ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, True), + ({"config": {"feature_toggles": {"enable": f"foo bar baz", TEST_FEATURE_TOGGLE: "true"}}}, True), + ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "true"}}}, True), + # this case will probably never happen, but lets account for it anyways ( { "config": { @@ -52,11 +75,6 @@ class TestIsRbacEnabledForStack: }, True, ), - ({"config": {"feature_toggles": {"enable": f"foo bar baz", TEST_FEATURE_TOGGLE: "true"}}}, True), - ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "true"}}}, True), - # features enabled via feature_toggles.enable should be comma separated, not space separated - ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE},baz"}}}, True), - ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, False), ], ) def test_feature_toggle_is_enabled(self, instance_info, expected) -> None: