Port activation flags with dynamic registration #29237

biazmoreira · 2024-12-19T14:20:14Z

Description

This PR adds activation flag logic, paths, and tests.

Activation flags are a one-off type of flags used to activate certain features in Vault.

Features cannot be deactivate and new paths cannot be registered by users.

TODO only if you're a HashiCorp employee

Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
- LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-12-19T14:38:08Z

CI Results:
All Go tests succeeded! ✅

banks

@biazmoreira this is looking great!

I think most comments inline are around parts of the test code that seem like they should ideally remain in Ent-only code.

I'm not sure if you already discussed these and there's a reason they need to be CE for now. Some of them seem from this PR like they'd fail because only the test code is here and not the actual implementation which I guess is ent-only.

I could be missing something though from previous ent review so let me know!

vault/logical_system_activation_flags.go

vault/logical_system_activation_flags_test.go

github-actions · 2025-01-06T19:55:17Z

Build Results:
All builds succeeded! ✅

banks

LGTM! Good to go. if you want to take the optional simpler syntax suggestion I can re-approve!

vault/logical_system_helpers.go

helper/activationflags/activation_flags.go

banks

re-approve. The delta is just my suggestion. There are some build things going on but don't seem related. If those can be figured out I think this is good!

mpalmi

Approving with my super late nits from before the break. Feel free to disregard unless they spark some new thinking.

mpalmi · 2024-12-19T15:07:55Z

vault/logical_system_activation_flags.go

+	return writeActivationFlag(ctx, b, req, data, true)
+}
+
+func (b *SystemBackend) readActivationFlag(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {


We can probably just drop the FieldData arg here, right (same with the other handler internals)?

mpalmi · 2024-12-19T15:10:26Z

vault/logical_system_activation_flags.go

+	return b.activationFlagsToResponse(activationFlags), nil
+}
+
+func (b *SystemBackend) writeActivationFlagWrite(ctx context.Context, req *logical.Request, _ *framework.FieldData, isActivate bool) (*logical.Response, error) {


Not a big deal, but we've got two "writes" in this method name. Maybe writeActivationFlag to keep it consistent with the readActivationFlag?

mpalmi · 2024-12-19T15:13:14Z

helper/activationflags/activation_flags.go

+	if storage == nil {
+		return fmt.Errorf("unable to access storage")
+	}


🤔 I wonder if we need to be so defensive... Wouldn't this mean that the systemBarrier is nil? Presumably we would have already bailed long before this point if so.

Same argument for the rest of the f.storage nil checks. I think it's safe to assume they're all non-nil at this point, unless you have a specific flow in mind that would break this assumption.

banks · 2025-01-09T11:19:14Z

changelog/29237.txt

@@ -0,0 +1,3 @@
+```release-note:feature
+**Activation Flag (core)**: A simple, one-time flag that lets users turn on specific Vault functionalities when they need them.


🤔 this already existed in previous versions right (just Enterprise only)? Was there no changelog entry at the time? It might be confusing for folks in the future if they see this in 1.19 but then see that older versions documented the API and had instructions for using with secret sync? We do have public docs for the endpoint already for example.

There's no Changelog for that... I can mention this was an enterprise only feature and now it's being ported to CE. Would that sound better?

Yeah, I'd also maybe not put it in as a "feature" as it's more of a mechanism for enabling actual features. (it's not use to users unless we wire it to something they care about). Maybe enhancement is better?

Maybe we could hint at the history without details like:

release-note:enhancement Activation Flags (core): A mechanism for users to opt-in to new functionality at a convenient time. Previously used only in Enterprise for SecretSync, activation flags are now available in CE for future features to use.

Or something?

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Dec 19, 2024

banks reviewed Jan 6, 2025

View reviewed changes

biazmoreira requested a review from banks January 6, 2025 19:35

biazmoreira added this to the 1.19.0-rc milestone Jan 6, 2025

biazmoreira marked this pull request as ready for review January 6, 2025 19:37

biazmoreira requested a review from a team as a code owner January 6, 2025 19:37

biazmoreira force-pushed the biazmoreira/activation-flag/1 branch from 48485ad to 115f7fe Compare January 6, 2025 21:58

vercel bot deployed to Preview January 8, 2025 12:02 View deployment

banks previously approved these changes Jan 8, 2025

View reviewed changes

vault/logical_system_helpers.go Outdated Show resolved Hide resolved

helper/activationflags/activation_flags.go Show resolved Hide resolved

biazmoreira requested a review from banks January 8, 2025 16:40

biazmoreira added 3 commits January 8, 2025 15:28

Port activation flags with dynamic registration

bbc63cb

Activation flag tests

b5d697f

PR feedback

114c46b

biazmoreira dismissed banks’s stale review via 114c46b January 8, 2025 18:31

biazmoreira force-pushed the biazmoreira/activation-flag/1 branch from dd4ad0a to 114c46b Compare January 8, 2025 18:31

banks previously approved these changes Jan 8, 2025

View reviewed changes

biazmoreira dismissed banks’s stale review via 91e29e8 January 8, 2025 18:56

Add CHANGELOG

7c6b409

biazmoreira force-pushed the biazmoreira/activation-flag/1 branch from 91e29e8 to 7c6b409 Compare January 8, 2025 19:06

biazmoreira requested review from mpalmi and banks January 8, 2025 19:10

mpalmi previously approved these changes Jan 8, 2025

View reviewed changes

Format proto (failing pipeline)

43999de

biazmoreira dismissed mpalmi’s stale review via 43999de January 8, 2025 19:24

biazmoreira requested a review from mpalmi January 8, 2025 19:26

mpalmi previously approved these changes Jan 8, 2025

View reviewed changes

Merge branch 'main' into biazmoreira/activation-flag/1

59dc51a

vercel bot deployed to Preview January 8, 2025 19:40 View deployment

banks reviewed Jan 9, 2025

View reviewed changes

Merge branch 'main' into biazmoreira/activation-flag/1

6d761e8

biazmoreira dismissed mpalmi’s stale review via d3af049 January 9, 2025 11:55

Update changelog

5539863

biazmoreira force-pushed the biazmoreira/activation-flag/1 branch from d3af049 to 5539863 Compare January 9, 2025 12:03

mpalmi approved these changes Jan 9, 2025

View reviewed changes

biazmoreira merged commit ab4e8da into main Jan 9, 2025
91 of 92 checks passed

biazmoreira deleted the biazmoreira/activation-flag/1 branch January 9, 2025 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port activation flags with dynamic registration #29237

Port activation flags with dynamic registration #29237

biazmoreira commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

banks left a comment

github-actions bot commented Jan 6, 2025 •

edited

Loading

banks left a comment

banks left a comment

mpalmi left a comment

mpalmi Dec 19, 2024

mpalmi Dec 19, 2024

mpalmi Dec 19, 2024

banks Jan 9, 2025

biazmoreira Jan 9, 2025

banks Jan 9, 2025

		@@ -0,0 +1,3 @@
		```release-note:feature
		Activation Flag (core): A simple, one-time flag that lets users turn on specific Vault functionalities when they need them.

Port activation flags with dynamic registration #29237

Port activation flags with dynamic registration #29237

Conversation

biazmoreira commented Dec 19, 2024 • edited Loading

Description

TODO only if you're a HashiCorp employee

github-actions bot commented Dec 19, 2024 • edited Loading

banks left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2025 • edited Loading

banks left a comment

Choose a reason for hiding this comment

banks left a comment

Choose a reason for hiding this comment

mpalmi left a comment

Choose a reason for hiding this comment

mpalmi Dec 19, 2024

Choose a reason for hiding this comment

mpalmi Dec 19, 2024

Choose a reason for hiding this comment

mpalmi Dec 19, 2024

Choose a reason for hiding this comment

banks Jan 9, 2025

Choose a reason for hiding this comment

biazmoreira Jan 9, 2025

Choose a reason for hiding this comment

banks Jan 9, 2025

Choose a reason for hiding this comment

biazmoreira commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Jan 6, 2025 •

edited

Loading