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

feat(multicluster): Multicluster working demo #3915

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

snehachhabria
Copy link
Contributor

@snehachhabria snehachhabria commented Aug 3, 2021

Description:
This commit contains the following changes :

  1. Changes across all vericals of XDS to ensure that the osm-gateway and envoy proxy for
    client apps are configured correctly to support multicluster traffic
  2. Updates to the multicluster demo script
  3. Remove redundant code

Part of #3444 and #3440

Output :
Tailing the bookbuyer logs on the alpha cluster will give 200 OK responses to bookstore-v1 on both alpha and beta cluster as shown below

image

Signed-off-by: Sneha Chhabria snchh@microsoft.com

Affected area:

Functional Area
New Functionality [ ]
Documentation [ ]
Install [ ]
CLI Tool [ ]
Certificate Management [ ]
Control Plane [ ]
Ingress [ ]
Egress [ ]
Networking [ ]
Observability [ ]
SMI Policy [ ]
Sidecar Injection [ ]
Security [ ]
Upgrade [ ]
Tests [ ]
CI System [ ]
Demo [ ]
Performance [ ]
Other [ ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project? no

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change? no

@snehachhabria snehachhabria force-pushed the multiClusterPOC branch 3 times, most recently from 4045f36 to 6afd2b5 Compare August 3, 2021 20:19
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2021

Codecov Report

Merging #3915 (d2975b8) into main (a4fab9f) will increase coverage by 0.11%.
The diff coverage is 78.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3915      +/-   ##
==========================================
+ Coverage   67.42%   67.53%   +0.11%     
==========================================
  Files         203      202       -1     
  Lines       11485    11491       +6     
==========================================
+ Hits         7744     7761      +17     
+ Misses       3690     3680      -10     
+ Partials       51       50       -1     
Flag Coverage Δ
unittests 67.53% <78.70%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/osm-controller/osm-controller.go 13.66% <0.00%> (ø)
pkg/apis/config/v1alpha1/zz_generated.deepcopy.go 0.00% <ø> (ø)
pkg/catalog/mock_catalog_generated.go 0.00% <0.00%> (ø)
pkg/configurator/methods.go 71.81% <ø> (+1.28%) ⬆️
pkg/configurator/mock_client_generated.go 0.00% <ø> (ø)
pkg/catalog/outbound_traffic_policies.go 84.07% <33.33%> (-0.93%) ⬇️
pkg/providers/kube/client.go 64.19% <50.00%> (-0.81%) ⬇️
pkg/providers/kube/multiclusterservice.go 77.50% <66.66%> (ø)
pkg/envoy/lds/response.go 75.47% <71.42%> (+0.47%) ⬆️
pkg/envoy/cds/response.go 79.16% <77.77%> (ø)
... and 4 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 a4fab9f...d2975b8. Read the comment docs.

@snehachhabria snehachhabria marked this pull request as ready for review August 3, 2021 20:30
@snehachhabria snehachhabria requested a review from a team as a code owner August 3, 2021 20:30
@snehachhabria snehachhabria force-pushed the multiClusterPOC branch 2 times, most recently from 0f48539 to 2eb85e0 Compare August 3, 2021 21:23
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.

This is a good start. I had a few concerns regarding certificates and admin port exposure. Other comments are regarding correctness.

charts/osm/templates/osm-gateway-deployment.yaml Outdated Show resolved Hide resolved
charts/osm/templates/osm-gateway-deployment.yaml Outdated Show resolved Hide resolved
charts/osm/templates/osm-gateway-service.yaml Outdated Show resolved Hide resolved
charts/osm/templates/osm-gateway-service.yaml Outdated Show resolved Hide resolved
charts/osm/values.yaml Show resolved Hide resolved
pkg/constants/constants.go Outdated Show resolved Hide resolved
pkg/envoy/cds/cluster.go Outdated Show resolved Hide resolved
}

// getGatewayUpstreamServiceCluster returns an Envoy Cluster corresponding to the given upstream service for the multicluster gateway
func getGatewayUpstreamServiceCluster(upstreamSvc service.MeshService, cfg configurator.Configurator, ports []corev1.ServicePort) (*xds_cluster.Cluster, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not reuse getUpstreamServiceCluster and use ClusterOption to override for multicluster? It would avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this, reusing getUpstreamServiceCluster will not be very effective and will add another condition. The name of the remote clusters for the gateway will be the entire server name of the service which makes it different from getUpstreamServiceCluster. Further the cluster discovery mechanism here will be strict DNS and we add the endpoints making it very different from the condition that exists in getUpstreamServiceCluster. Adding another method in my opinion is cleaner for this scenario.

pkg/envoy/cds/response.go Outdated Show resolved Hide resolved
pkg/envoy/lds/gateway.go Outdated Show resolved Hide resolved
@snehachhabria snehachhabria force-pushed the multiClusterPOC branch 2 times, most recently from 725e6f9 to 2e21c11 Compare August 4, 2021 01:05
demo/deploy-bookstore.sh Show resolved Hide resolved
demo/run-osm-multicluster-demo.sh Outdated Show resolved Hide resolved
pkg/catalog/outbound_traffic_policies.go Show resolved Hide resolved
pkg/envoy/lds/gateway.go Outdated Show resolved Hide resolved
demo/copy-osm-ca-bundle.sh Outdated Show resolved Hide resolved
.env.example Show resolved Hide resolved
charts/osm/values.schema.json Show resolved Hide resolved
This commit contains the following changes :
1. Changes across all vericals of XDS to ensure that the osm-gateway and
client are configured correctly to support multicluter traffic
2. Updates to the multicluster demo script
3. Remove redundant code

Part of openservicemesh#3444 and  openservicemesh#3440

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
charts/osm/README.md Show resolved Hide resolved
pkg/providers/kube/client.go Show resolved Hide resolved
pkg/envoy/lds/response.go Show resolved Hide resolved
pkg/envoy/cds/response.go Show resolved Hide resolved
Comment on lines +66 to +67
marshalledUpstreamTLSContext, err := ptypes.MarshalAny(
envoy.GetUpstreamTLSContext(downstreamIdentity, upstreamSvc))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
marshalledUpstreamTLSContext, err := ptypes.MarshalAny(
envoy.GetUpstreamTLSContext(downstreamIdentity, upstreamSvc))
marshalledUpstreamTLSContext, err := ptypes.MarshalAny(envoy.GetUpstreamTLSContext(downstreamIdentity, upstreamSvc))

pkg/envoy/cds/cluster.go Show resolved Hide resolved
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

🎆👏🏻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants