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(gateway): cp annotations from gateway to svc #4327

Merged
merged 2 commits into from
May 24, 2022

Conversation

johnharris85
Copy link
Contributor

Summary

Many cloud providers use annotations on service objects to control behavior with their auto-provisioned load balancers. This PR adds a feature that will copy all annotations specified on the MeshGatewayInstance object to the generated service.

In #4075 there was some discussion about how to implement this, and a suggestion that maybe we go down the route of creating a CRD that would allow full customization of the generated service. This makes sense in future, however the 90% use case is solved by just enabling a user to specify annotations. This PR's implementation covers that use case for users who are currently experiencing issues while not adding / altering any CRDs / APIs, and is easy to deprecate when we decide on a 'better' solution.

Will add docs if folks are fine with this implementation.

Issues resolved

Partially fix #4075

Documentation

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

@johnharris85 johnharris85 requested a review from a team as a code owner May 21, 2022 18:14
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2022

Codecov Report

Merging #4327 (360f683) into master (bb4e42b) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4327      +/-   ##
==========================================
- Coverage   55.75%   55.70%   -0.05%     
==========================================
  Files         935      935              
  Lines       56434    56437       +3     
==========================================
- Hits        31462    31439      -23     
- Misses      22498    22527      +29     
+ Partials     2474     2471       -3     
Impacted Files Coverage Δ
...ime/k8s/controllers/gateway_instance_controller.go 0.00% <0.00%> (ø)
pkg/insights/components.go 58.33% <0.00%> (-25.01%) ⬇️
pkg/plugins/common/postgres/listener.go 53.84% <0.00%> (-15.39%) ⬇️
pkg/plugins/resources/postgres/events/listener.go 16.66% <0.00%> (-14.59%) ⬇️
pkg/core/tokens/default_signing_key.go 77.77% <0.00%> (-8.34%) ⬇️
pkg/events/eventbus.go 85.18% <0.00%> (-7.41%) ⬇️
pkg/dp-server/server/server.go 83.63% <0.00%> (-7.28%) ⬇️
pkg/core/secrets/manager/global_manager.go 42.42% <0.00%> (-3.04%) ⬇️
pkg/insights/resyncer.go 71.60% <0.00%> (-2.47%) ⬇️
pkg/plugins/resources/postgres/store.go 76.37% <0.00%> (-0.43%) ⬇️
... and 2 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 bb4e42b...360f683. Read the comment docs.

@johnharris85
Copy link
Contributor Author

Test failure looks unrelated?

@michaelbeaumont
Copy link
Contributor

@johnharris85 What do you think about just adding a single spec.serviceTemplate.metadata.annotations field?

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.

LGTM maybe @lahabana take a look too 🤷

@lahabana
Copy link
Contributor

/golden_files

@lahabana
Copy link
Contributor

Arg obviously my tool is broken with forks :(

Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

Once the conflict is resolved

johnharris85 and others added 2 commits May 24, 2022 09:52
Signed-off-by: John Harris <john@johnharris.io>
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont enabled auto-merge (squash) May 24, 2022 07:54
@michaelbeaumont michaelbeaumont merged commit 5a1704d into kumahq:master May 24, 2022
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.

Mesh gateway svc - support for additional annotations and loadBalancerIp
4 participants