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.

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

Optionally block enabling certain feature flags via Management UI/API #10682

Closed
wants to merge 1 commit into from

Conversation

gomoripeti
Copy link
Contributor

@gomoripeti gomoripeti commented Mar 5, 2024

Proposed Changes

Useful to avoid accidentally enabling for example experimental feature flags from the Management UI on sensitive clusters,
even for users who have administrator access to the Mgmt UI/API, but still don't have server access. After testing an experimental feature flag in a test/staging env, the feature flag can be unblocked by operators on the sensitive production clusters.

I intentionally kept this configuration outside of the core feature flags framework, and only a management plugin feature.

I am open to suggestions if this should be configured or implemented differently.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

Useful to avoid accidentally enabling for example experiemental
feature flags from the Management UI on sensitive clusters.
@michaelklishin
Copy link
Member

michaelklishin commented Mar 5, 2024

@gomoripeti I'd highly recommend consulting with someone on the core team before you spend time on something that's not obviously a bug fix.

For example, this PR further complicates the matrix of feature flags, deprecated features, all that. Now we'd potentially have "blocked" feature flags. The risk of not being accepted for such semantically complex PRs is usually much higher than average :(

@gomoripeti
Copy link
Contributor Author

you are right with the consulting.

I'd like to mention that exactly because of the complexity of the feature flags subsystem, I did not touch that. This is rather part of the restrictions subsystem of the management plugin only, that I think fits well to the other restrictions. I am open to changing naming in any way - another alternative that matches more the naming of the other restrictions, feature_flag_enabling_disabled sounds a bit strange.


%% Block enabling certain feature flags over API

{mapping, "management.restrictions.feature_flag_blocked.$name", "rabbitmq_management.restrictions.feature_flag_blocked", [
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about renaming this key to management.restrictions.blocked_feature_flags.$name (in plural with the adjective coming first, as it would in a piece of prose in English)?

@michaelklishin
Copy link
Member

you are right with the consulting.

I'd like to mention that exactly because of the complexity of the feature flags subsystem, I did not touch that. This is rather part of the restrictions subsystem of the management plugin only, that I think fits well to the other restrictions. I am open to changing naming in any way - another alternative that matches more the naming of the other restrictions, feature_flag_enabling_disabled sounds a bit strange.

I did not mean the technical complexity of feature flags. If we accept a new concept, like "blocked feature flags" or "restricted feature flags", we must be able to explain it to the user with as little as possible confusion with related concepts.

Deprecated features and feature flags have very little in common technically but they seem like the same "on and off switch" idea for features, which is not correct. Adding blocked/restricted feature flags does not make that any easier, and as more concepts are added, the complexity grows exponentially.

In any case, I've asked the rest of the team if they'd accept such a feature in general.

@michaelklishin
Copy link
Member

@gomoripeti someone has suggested that a UI confirmation (of the yes/no type) for experimental feature flags would suffice, and would not introduce any new concepts to document and explain.

@michaelklishin
Copy link
Member

@gomoripeti another idea, sadly a more painful one to implement given the age of our JS codebase, but still: similarly to how you must type in the name of the repository you attempt to delete (or transfer) on GitHub, we can ask for the name of an experimental feature flag before enabling it.

This should be a lot more "fat-finger-proof" than a simple Ok/Cancel confirmation.

@gomoripeti
Copy link
Contributor Author

it seems to be important to have a naming that clarifies that this is not a new concept

There are already management.restrictions.operator_policy_changes.disabled and management.restrictions.quorum_queue_replica_operations.disabled which disallow certain operations and block certain API endpoints.
I also had some more ideas to remove specifying a particular FF (so that it does not look like that FF is blocked, as a new concept)

  • management.restrictions.feature_flag_changes.disabled could just disallow enabling any FF
  • management.restrictions.experimental_feature_flag_changes.disabled could disallow enabling only experimental FFs

But I realise these proposals (including the PR in the current form) don't have any added value. Just blocking certain API endpoints can be implemented in a firewall or proxy and doesn't need any more rabbitmq-specific information.

It would have added value if the UI would be changed as you described with some dialogue, but I would like to skip that more painful part.

But it was a good discussion and I learnt about management restrictions while implementing it, so not fully wasted effort :)

@gomoripeti gomoripeti closed this Mar 6, 2024
@michaelklishin
Copy link
Member

michaelklishin commented Mar 6, 2024

With a solution like this, we'd have to explain this "concept" or "feature set" of feature flags on top of everything else. Whether that's technically a new concept or not does not change that fact.

But I think that any confirmation solution for experimental FFs would be very welcome, I can see how clicking a button and enabling Khepri by accident is fairly easy.

@michaelklishin
Copy link
Member

In RabbitMQ-as-a-service scenarios, blocking an endpoint might work. We can always wait and see how often people would enable Khepri (or a future experimental feature flag) by accident and complain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants