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

fix(kuma-cp): protocol check should be case insensitive #4248

Merged
merged 6 commits into from
May 12, 2022

Conversation

lukidzi
Copy link
Contributor

@lukidzi lukidzi commented May 4, 2022

Summary

When protocol label is defined with uppercased value TCP deployment of gateway fails because it's different than TCP.
We can discuss in issue if we want this way or maybe make it different way.

Full changelog

  • [Implement case insensitive comparision]
  • [added to log information about provided protocol]

Issues resolved

Fix #4247

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

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lukidzi lukidzi requested a review from a team as a code owner May 4, 2022 17:11
@jakubdyszkiewicz
Copy link
Contributor

do we also treat kuma.io/protocol as case insensitive down the road when we actually use it? Somewhere around outbound_proxy_generator.go etc.

@lukidzi
Copy link
Contributor Author

lukidzi commented May 5, 2022

In some places, we check lower cases by using https://github.com/kumahq/kuma/blob/master/pkg/core/resources/apis/mesh/dataplane_helpers.go#L26. Maybe the best option will be to lowercase that at inbound_converter.

@jakubdyszkiewicz
Copy link
Contributor

I'm ok with lower-casing in inbound converter

@lahabana
Copy link
Contributor

lahabana commented May 9, 2022

Decision from triage:

  • Should not touch casing in validation (except fixing error message).
  • In inbound_converter.go should lower case appProtocol/annotation.

@codecov-commenter
Copy link

Codecov Report

Merging #4248 (a01b4a2) into master (cc164cd) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4248      +/-   ##
==========================================
- Coverage   55.65%   55.60%   -0.06%     
==========================================
  Files         935      935              
  Lines       56347    56347              
==========================================
- Hits        31361    31331      -30     
- Misses      22504    22523      +19     
- Partials     2482     2493      +11     
Impacted Files Coverage Δ
...kg/core/resources/apis/mesh/dataplane_validator.go 100.00% <100.00%> (ø)
...ugins/runtime/k8s/controllers/inbound_converter.go 89.38% <100.00%> (ø)
pkg/clusterid/creator.go 73.33% <0.00%> (-13.34%) ⬇️
pkg/metrics/store/counter.go 74.54% <0.00%> (-10.91%) ⬇️
pkg/core/runtime/component/component.go 82.45% <0.00%> (-10.53%) ⬇️
pkg/core/tokens/default_signing_key.go 66.66% <0.00%> (-5.56%) ⬇️
...s/authn/api-server/tokens/admin_token_bootstrap.go 82.00% <0.00%> (-4.00%) ⬇️
pkg/defaults/components.go 82.14% <0.00%> (-3.58%) ⬇️
pkg/core/secrets/manager/global_manager.go 39.39% <0.00%> (-3.04%) ⬇️
pkg/core/resources/manager/cache.go 85.71% <0.00%> (-2.60%) ⬇️
... and 3 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 cc164cd...a01b4a2. Read the comment docs.

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lukidzi lukidzi merged commit 5791baf into master May 12, 2022
@lukidzi lukidzi deleted the fix/protocol_case branch May 12, 2022 12:11
slonka pushed a commit that referenced this pull request May 13, 2022
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
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.

Deployment of gateway with kuma.io/protocol label with not lowercase value fails
4 participants