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

Fork: Remove 'Enterprise' and related references from user-visible codebase #142

Merged

Conversation

Gabrielopesantos
Copy link
Contributor

@Gabrielopesantos Gabrielopesantos commented Feb 14, 2024

Resolves #121

Description

Removes references to upstream Enterprise features;

  • User visible references in CLI, API;
  • Transit and PKI mentions to managed keys;
  • Enterprise features such as licensing, entropy configuration;

Target Release

Alpha

@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch 3 times, most recently from b5a0a5d to 83c6ebd Compare February 17, 2024 16:10
vault/core.go Outdated Show resolved Hide resolved
@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch 2 times, most recently from bb19748 to f82fe38 Compare February 21, 2024 23:27
@Gabrielopesantos Gabrielopesantos marked this pull request as ready for review February 21, 2024 23:27
@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch from a7bd97f to 357ab27 Compare February 22, 2024 00:39
@Gabrielopesantos
Copy link
Contributor Author

Gabrielopesantos commented Feb 22, 2024

There seem to be a few tests broken, under builtin/logical/database and /vault/external_tests/raft but I don't think they are related to these changes.

Also, as my previous contributions to vault were only crypto related there might be additional places where references to enterprise are made but I'm not aware of, if so, please let me know.

Copy link
Member

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Broadly I think this looks fairly good and am happy to see this merged, with or without the constants.IsEnterprise removal!

Long term, I think we should think about removing all the stubs from files that are marked //go:build !enterprise: we'll need to remove a lot of call sites, but these should be unnecessary to keep given we don't have Vault Enterprise code.

@Gabrielopesantos
Copy link
Contributor Author

Broadly I think this looks fairly good and am happy to see this merged, with or without the constants.IsEnterprise removal!

Long term, I think we should think about removing all the stubs from files that are marked //go:build !enterprise: we'll need to remove a lot of call sites, but these should be unnecessary to keep given we don't have Vault Enterprise code.

Just committed the removal of all "enterprise" stubs in the files. With your previous comment did you mean just that or the files that had that stub and all code references/calls to that code?

@cipherboy
Copy link
Member

With your previous comment did you mean just that or the files that had that stub and all code references/calls to that code?

Ah, I was thinking that you'd remove the functions themselves, and maybe the files too, to that where you can... E.g., builtin/logical/pki/managed_key_util.go only has no-op functions, so if you want to make the more invasive change, you can remove that file, and then remove all the function calls throughout the code base to those functions.

Its more work, but reduces our code size a fair bit :-)

That's why I thought that perhaps a separate PR (maybe logically grouped -- one for managed keys, one for clustering hooks, &c) could be better. :-)

@Gabrielopesantos
Copy link
Contributor Author

Gabrielopesantos commented Feb 24, 2024

With your previous comment did you mean just that or the files that had that stub and all code references/calls to that code?

Ah, I was thinking that you'd remove the functions themselves, and maybe the files too, to that where you can... E.g., builtin/logical/pki/managed_key_util.go only has no-op functions, so if you want to make the more invasive change, you can remove that file, and then remove all the function calls throughout the code base to those functions.

Its more work, but reduces our code size a fair bit :-)

That's why I thought that perhaps a separate PR (maybe logically grouped -- one for managed keys, one for clustering hooks, &c) could be better. :-)

I see, maybe having separate PRs for that, as you are suggesting, is a better idea. Do I need to revert the commit the removed the "enterprise" stubs?

@cipherboy
Copy link
Member

@Gabrielopesantos No, I think this is fine as-is, just need to remember which files you need to trim up later.

Copy link
Member

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Looks very nice @Gabrielopesantos!

@naphelps naphelps self-requested a review March 1, 2024 15:33
@naphelps naphelps self-requested a review March 1, 2024 15:35
Copy link
Member

@naphelps naphelps left a comment

Choose a reason for hiding this comment

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

@Gabrielopesantos Your commits need to be signed before I can merge your pull request.

@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch from 3a7b895 to b8319cd Compare March 1, 2024 21:06
@Gabrielopesantos
Copy link
Contributor Author

Gabrielopesantos commented Mar 1, 2024

Hey @naphelps, do you mean GPG-signed? If it's just the "signed-off-by" message they are already signed and the DCO check is passing.

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
build is breaking

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch from b8319cd to bb0b542 Compare March 4, 2024 17:01
Copy link
Member

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Hmm, wonder if something got messed up in a rebase:

# github.com/openbao/openbao/command [github.com/openbao/openbao/command.test]
command/operator_diagnose.go:29:2: could not import github.com/openbao/openbao/helper/constants (open : no such file or directory)
command/operator_diagnose.go:598:15: coreConfig.LicensePath undefined (type "github.com/openbao/openbao/vault".CoreConfig has no field or method LicensePath)
command/operator_diagnose.go:601:15: coreConfig.License undefined (type "github.com/openbao/openbao/vault".CoreConfig has no field or method License)
command/server.go:436:29: config.Entropy undefined (type *server.Config has no field or method Entropy)
command/server.go:438:10: config.Entropy undefined (type *server.Config has no field or method Entropy)
command/server.go:1138:10: config.LicensePath undefined (type *server.Config has no field or method LicensePath)
command/server.go:1141:10: config.License undefined (type *server.Config has no field or method License)
command/server.go:1656:19: undefined: vault.LicenseReload
command/server.go:2751:3: unknown field License in struct literal of type "github.com/openbao/openbao/vault".CoreConfig
command/server.go:2751:42: config.License undefined (type *server.Config has no field or method License)
command/server.go:2751:3: too many errors
?   	github.com/openbao/openbao/builtin/plugin/v5	[no test files]
FAIL	github.com/openbao/openbao/command [build failed]

--- FAIL: TestUnknownFieldValidation (0.00s)
    config_test_helpers.go:453: found unexpected error: unknown or unsupported field license_path found in configuration at ./test-fixtures/config.hcl:54:1
FAIL
FAIL	github.com/openbao/openbao/command/server	15.330s

@Gabrielopesantos
Copy link
Contributor Author

Gabrielopesantos commented Mar 4, 2024

Hmm, wonder if something got messed up in a rebase:

# github.com/openbao/openbao/command [github.com/openbao/openbao/command.test]
command/operator_diagnose.go:29:2: could not import github.com/openbao/openbao/helper/constants (open : no such file or directory)
command/operator_diagnose.go:598:15: coreConfig.LicensePath undefined (type "github.com/openbao/openbao/vault".CoreConfig has no field or method LicensePath)
command/operator_diagnose.go:601:15: coreConfig.License undefined (type "github.com/openbao/openbao/vault".CoreConfig has no field or method License)
command/server.go:436:29: config.Entropy undefined (type *server.Config has no field or method Entropy)
command/server.go:438:10: config.Entropy undefined (type *server.Config has no field or method Entropy)
command/server.go:1138:10: config.LicensePath undefined (type *server.Config has no field or method LicensePath)
command/server.go:1141:10: config.License undefined (type *server.Config has no field or method License)
command/server.go:1656:19: undefined: vault.LicenseReload
command/server.go:2751:3: unknown field License in struct literal of type "github.com/openbao/openbao/vault".CoreConfig
command/server.go:2751:42: config.License undefined (type *server.Config has no field or method License)
command/server.go:2751:3: too many errors
?   	github.com/openbao/openbao/builtin/plugin/v5	[no test files]
FAIL	github.com/openbao/openbao/command [build failed]

--- FAIL: TestUnknownFieldValidation (0.00s)
    config_test_helpers.go:453: found unexpected error: unknown or unsupported field license_path found in configuration at ./test-fixtures/config.hcl:54:1
FAIL
FAIL	github.com/openbao/openbao/command/server	15.330s

This error was fairly simple to fix but it seems I broke some additional tests. Having a look.
UPDATE: I was wrong, it was the tests were timing out due to the value I had set in the timeout flag.

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>
@Gabrielopesantos Gabrielopesantos force-pushed the gabrielopesantos/remove-enterprise-refs branch from 67fd5ca to f595e81 Compare March 4, 2024 23:04
Copy link
Member

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

This looks much better, thanks @Gabrielopesantos ! Tests pass again :-D

@cipherboy
Copy link
Member

@Gabrielopesantos said:

This error was fairly simple to fix but it seems I broke some additional tests. Having a look.
UPDATE: I was wrong, it was the tests were timing out due to the value I had set in the timeout flag.

Yeah, if you look at the Makefile, it sets a really long (45min?) timeout... Certain packages take nearly forever to run.

@naphelps naphelps merged commit f8e61b4 into openbao:main Mar 5, 2024
31 of 35 checks passed
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.

Fork - Remove references to Enterprise from user-visible code base
3 participants