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

feat(envoy): Return list of services in GetServiceFromEnvoyCertificate #1566

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

ksubrmnn
Copy link
Contributor

Please describe the motivation for this PR and provide enough
information so that others can review it.
Return a list of services for an envoy certificate instead of just the first one
Please mark with X for applicable areas.

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [X]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests / CI System [ ]
  • 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

@ksubrmnn ksubrmnn force-pushed the services_from_cert branch from 3d24216 to b154f7f Compare August 18, 2020 02:01
@ksubrmnn ksubrmnn marked this pull request as ready for review August 18, 2020 02:11
@ksubrmnn ksubrmnn requested a review from a team as a code owner August 18, 2020 02:11
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.

@ksubrmnn thank you so much for making all these changes!

Would you mind changing the []*MeshService to []MeshService ?

DESIGN.md Outdated Show resolved Hide resolved
pkg/catalog/types.go Outdated Show resolved Hide resolved
pkg/catalog/xds_certificates.go Outdated Show resolved Hide resolved
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.

Change looks good. Would you mind addressing the following 2 comments at a high level:

  1. Adding unit tests for GetServiceFromEnvoyCertificate() to return multiple services
  2. Tagging assumptions with Github issue ID using comments inline in the code

Thanks

pkg/catalog/xds_certificates_test.go Show resolved Hide resolved
pkg/envoy/ads/stream.go Show resolved Hide resolved
pkg/envoy/cds/response.go Outdated Show resolved Hide resolved
pkg/envoy/eds/response.go Outdated Show resolved Hide resolved
pkg/envoy/lds/response.go Outdated Show resolved Hide resolved
@ksubrmnn ksubrmnn force-pushed the services_from_cert branch 2 times, most recently from bcf6ec3 to dc81a3d Compare August 18, 2020 18:14
shashankram
shashankram previously approved these changes Aug 18, 2020
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.

Thank you @ksubrmnn !

Comment on lines +45 to +48
meshService := service.MeshService{
Namespace: cnMeta.Namespace,
Name: svc.Name,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought, it might be a good idea to abstract the conversion of a service to meshService in a utils function and they use it everywhere. WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From kubernetes service to meshService? I think that's a good idea. Can you make an issue and tag it as "good first issue"?

Copy link
Member

Choose a reason for hiding this comment

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

There is a helper for converting K8s service to MeshService in catalog/routes.go: k8sSvcToMeshSvc

Copy link
Contributor

Choose a reason for hiding this comment

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

@ksubrmnn please could we use k8sSvcToMeshSvc wherever possible

@ksubrmnn ksubrmnn merged commit b52d4c3 into openservicemesh:main Aug 18, 2020
@ksubrmnn ksubrmnn deleted the services_from_cert branch August 18, 2020 19:26
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.

GetServiceFromEnvoyCertificate should list all services associated with Envoy
4 participants