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

NET-5824 Exported services api #20015

Merged
merged 16 commits into from
Jan 23, 2024
Merged

Conversation

tauhid621
Copy link
Contributor

@tauhid621 tauhid621 commented Dec 21, 2023

Description

This PR adds support for an API to list exported services and their consumers. The sameness groups and wildcards are resolved to peers and services.

URL: /v1/exported-services
Response:

[
    {
        "Service": "frontend",
        "Consumers": {
            "Peers": [ "east", "third" ]
        }
    },
    {
        "Service": "backend",
        "Consumers": {
            "Peers": [ "east", "west"]
        }
    }
]

Testing & Reproduction steps

  • Unit tests added
  • Local testing

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@tauhid621 tauhid621 added the consul-india PRs/Issues assigned to Consul India team label Dec 26, 2023
@tauhid621 tauhid621 added the backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. label Dec 27, 2023
@tauhid621 tauhid621 marked this pull request as ready for review December 27, 2023 07:42
@tauhid621 tauhid621 requested a review from a team as a code owner December 27, 2023 07:42
@tauhid621 tauhid621 changed the title Exported services api implemented NET-5824 Exported services api Dec 27, 2023
@tauhid621 tauhid621 requested a review from dhiaayachi December 27, 2023 12:26
agent/http_register.go Outdated Show resolved Hide resolved
agent/config_endpoint.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

Great work @tauhid621! 👏

I think you need an enterprise PR for this, so I will wait to see the enterprise PR before approving this, but nothing blocking so far!


type ResolvedExportedService struct {
// Service is the name of the service which is exported.
Service string
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this missing the json tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this field will always be present, I did not add the json:",omitempty" tag.

Copy link
Contributor

@Ganeshrockz Ganeshrockz left a comment

Choose a reason for hiding this comment

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

Looks good. Just one minor comment


// If this isn't a wildcard, we can simply add it to the list of services to watch and move to the next entry.
if svc.Name != structs.WildcardSpecifier {
insertEntry(exportedServices, svcName, svc.Consumers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of converting to an intermediate map and then to another response why not add the list of services encountered here to a slice and pass it on to the prepareExportedServicesResponse function? There you could just walkover the list of services and compute the response as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main purpose of the map is to avoid duplicate entries for service and consumers. Are you suggesting to create the map in the prepareExportedServicesResponse function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. The map is still needed but it just looks cleaner if there a function that accepts a list of exported services and returns back the expected proto response. The function's signature doesn't look that readable as of now. But I would leave it upto your best judgement here and let you take a call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the duplicate removal logic to a new function.

@tauhid621 tauhid621 merged commit 5d294b2 into main Jan 23, 2024
91 checks passed
@tauhid621 tauhid621 deleted the tauhid621/exported_services_api_grpc branch January 23, 2024 04:37
tauhid621 added a commit that referenced this pull request Jan 23, 2024
* Exported services api implemented

* Tests added, refactored code

* Adding server tests

* changelog added

* Proto gen added

* Adding codegen changes

* changing url, response object

* Fixing lint error by having namespace and partition directly

* Tests changes

* refactoring tests

* Simplified uniqueness logic for exported services, sorted the response in order of service name

* Fix lint errors, refactored code
tauhid621 added a commit that referenced this pull request Jan 23, 2024
* Exported services api implemented

* Tests added, refactored code

* Adding server tests

* changelog added

* Proto gen added

* Adding codegen changes

* changing url, response object

* Fixing lint error by having namespace and partition directly

* Tests changes

* refactoring tests

* Simplified uniqueness logic for exported services, sorted the response in order of service name

* Fix lint errors, refactored code
@hc-github-team-consul-core
Copy link
Collaborator

@tauhid621, a backport is missing for this PR [20015] for versions [1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

tauhid621 added a commit that referenced this pull request Jan 23, 2024
NET-5824 Exported services api (#20015)

* Exported services api implemented

* Tests added, refactored code

* Adding server tests

* changelog added

* Proto gen added

* Adding codegen changes

* changing url, response object

* Fixing lint error by having namespace and partition directly

* Tests changes

* refactoring tests

* Simplified uniqueness logic for exported services, sorted the response in order of service name

* Fix lint errors, refactored code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. consul-india PRs/Issues assigned to Consul India team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants