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

feat(*): enable Gateway with runtime flag #3736

Merged
merged 5 commits into from
Jan 25, 2022

Conversation

jakubdyszkiewicz
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz commented Jan 20, 2022

Summary

Enabled Gateway with runtime configuration instead of a flag during compilation.
This lets us ship gateway as an experimental opt-in feature.

Full changelog

  • Add Gateway support to kumactl get, kumactl delete and kumactl apply by default, even if it's not enabled in CP. Typing extra --experimental-gateway for all those commands would be annoying if anyone would like to use Gateway now.
  • Add --experimental-gateway to kumactl install gateway. Only in this case we install Gateway resources and RBAC.
  • HELM support is not yet ready as installing CRD conditionally is not a trivial task.
  • Previously install tests were compiled with gateway flag, that's why golden files had gateway CRDs. Since now gateway is optional, a bunch of lines is removed from golden files and gateway support is tested in one case only.
  • Gateway resources are registered conditionally in CP. For this to work, I converted Gateway Runtime plugin to Bootstrap plugin. Additionally, Gateway resources need to be registered before the Kubernetes Bootstrap plugin therefore I introduced an order of registration. I believe the order will be useful for other use cases in the future.

Issues resolved

Fix #3722

Documentation

  • No docs yet. New config is added to config reference.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

- [ ] Update UPGRADE.md with any steps users will need to take when upgrading.
- [ ] Add backport-to-stable label if the code follows our backporting policy

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #3736 (be99496) into master (45a6d5a) will increase coverage by 0.04%.
The diff coverage is 71.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3736      +/-   ##
==========================================
+ Coverage   50.89%   50.93%   +0.04%     
==========================================
  Files         924      923       -1     
  Lines       56960    56990      +30     
==========================================
+ Hits        28987    29026      +39     
+ Misses      25689    25679      -10     
- Partials     2284     2285       +1     
Impacted Files Coverage Δ
...d/install/context/install_control_plane_context.go 100.00% <ø> (ø)
...umactl/cmd/install/context/install_crds_context.go 84.61% <ø> (ø)
...kg/core/resources/apis/mesh/dataplane_validator.go 100.00% <ø> (+2.15%) ⬆️
pkg/plugins/runtime/gateway/enabled.go 100.00% <ø> (ø)
test/e2e/gateway/gateway_kubernetes.go 0.00% <0.00%> (ø)
test/e2e/gateway/gateway_universal.go 0.00% <0.00%> (ø)
pkg/core/resources/registry/global.go 63.63% <66.66%> (+3.63%) ⬆️
pkg/plugins/runtime/gateway/plugin.go 95.45% <88.23%> (-4.55%) ⬇️
app/kumactl/cmd/apply/apply.go 75.25% <100.00%> (+0.25%) ⬆️
app/kumactl/cmd/delete/delete.go 83.33% <100.00%> (+0.40%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45a6d5a...be99496. Read the comment docs.

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz marked this pull request as ready for review January 24, 2022 13:12
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner January 24, 2022 13:12
// to a Kubernetes zone, we would need to be able to transform Gateway
// resources from Universal -> Kubernetes and have to deal with namespace
// semantics and a lot of other unpleasantness.
// RegisterTypeIfAbsent is used because it's not deterministic in testing that RegisterGatewayTypes is called only once.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know why it's not deterministic? Could/should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example BuildRuntime() is a problem. It registers the plugin for every test case, but Type Registry is unfortunately global, so it tries to register multiple times.

Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

Looks good otherwise, I just fear the non determinism is due to some kind of broken initialization when we run the tests.

…e-flag

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz merged commit d259a13 into master Jan 25, 2022
@jakubdyszkiewicz jakubdyszkiewicz deleted the feat/gateway-runtime-flag branch January 25, 2022 10:51
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.

Switch gateway build flag to be a feature flag
4 participants