Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

bug(pkg/*) : Fixing access to backend services #1944

Merged
merged 5 commits into from
Oct 28, 2020
Merged

bug(pkg/*) : Fixing access to backend services #1944

merged 5 commits into from
Oct 28, 2020

Conversation

snehachhabria
Copy link
Contributor

Description:
Fixes : #1798 and #1316

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [X]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [X]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution? no

@snehachhabria snehachhabria linked an issue Oct 28, 2020 that may be closed by this pull request
Update RDS to handle a list of service hostnames while aggregating routes per host basis #1316
Closed
17 tasks
@codecov-io
Copy link

Codecov Report

Merging #1944 into main will decrease coverage by 0.04%.
The diff coverage is 70.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1944      +/-   ##
==========================================
- Coverage   58.51%   58.46%   -0.05%     
==========================================
  Files         131      131              
  Lines        5272     5285      +13     
==========================================
+ Hits         3085     3090       +5     
- Misses       2184     2192       +8     
  Partials        3        3
Impacted Files Coverage Δ
pkg/catalog/mock_catalog.go 0.00% <0.00%> (ø)
pkg/catalog/routes.go 79.67% <50.00%> (+1.89%) ⬆️
pkg/envoy/rds/response.go 28.57% <50.00%> (+3.57%) ⬆️
pkg/envoy/route/config.go 96.06% <100.00%> (+0.22%) ⬆️
pkg/kubernetes/util.go 95.23% <100.00%> (+0.23%) ⬆️
...ertificate/providers/tresor/certificate_manager.go 64.04% <0.00%> (-12.36%) ⬇️

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 97a0720...470fc2a. Read the comment docs.

shashankram
shashankram reviewed Oct 28, 2020
View reviewed changes
pkg/envoy/route/config.go Outdated Show resolved Hide resolved
pkg/kubernetes/util.go
Comment on lines +54 to +58
service := strings.Split(host, ".")[0]

// For services that are not namespaced the service name contains the port as well
// Ex. service:port
return strings.Split(service, ":")[0]
Copy link
Member

Choose a reason for hiding this comment

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

Good find.

pkg/kubernetes/util_test.go Show resolved Hide resolved
pkg/trafficpolicy/types.go Outdated Show resolved Hide resolved
pkg/catalog/types.go Outdated Show resolved Hide resolved
@snehachhabria snehachhabria marked this pull request as ready for review October 28, 2020 17:15
@snehachhabria snehachhabria requested a review from a team as a code owner October 28, 2020 17:15
shashankram
shashankram previously approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

And e2e test for this would be nice to have. Would you mind filing an issue for this.

@snehachhabria snehachhabria mentioned this pull request Oct 28, 2020
Closed
@snehachhabria snehachhabria requested a review from a team October 28, 2020 17:25
shashankram
shashankram approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. We should note somewhere (either code or commit message) that this is a short term fix and that issue will rework this.

@snehachhabria
Copy link
Contributor Author

Thanks for the fix. We should note somewhere (either code or commit message) that this is a short term fix and that issue will rework this.

I will include it in the commit message

eduser25
eduser25 approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@eduser25 eduser25 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

have the test for e2e for this changes, passing just fine. lgtm

@snehachhabria snehachhabria merged commit de55d86 into openservicemesh:main Oct 28, 2020
@snehachhabria snehachhabria deleted the fixbackends branch January 15, 2021 00:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shashankram shashankram shashankram approved these changes

@eduser25 eduser25 eduser25 approved these changes

@ksubrmnn ksubrmnn Awaiting requested review from ksubrmnn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@snehachhabria @codecov-io @eduser25 @shashankram