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

update gateway-services table with endpoints #13217

Merged
merged 26 commits into from
May 31, 2022

Conversation

dhiaayachi
Copy link
Collaborator

@dhiaayachi dhiaayachi commented May 25, 2022

Description

This part of the work needed to add destinations (formerly endpoints) to the terminating gateway. Destinations are defined using a service-default config entry and represent an external entity (a set of addresses and a port) which can be reachable through a given terminating gateway.

In this PR we add the association between a given destination and a terminating gateway. This include the use case of a terminating gateway that define a wildcard in his LinkedServices.

We also add a filed that define the kind of the mapping (Unknown, Service or Destination) so that could be used in the proxycfg part to define the right envoy config.

The Kind is updated based on Service and Destination CRUD and if a Service and a Destination with the same name exists priority is given to the Service.

PR Checklist

  • updated test coverage
  • not a security concern

@dhiaayachi dhiaayachi added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test labels May 25, 2022
@dhiaayachi dhiaayachi requested review from a team and rboyer and removed request for a team May 25, 2022 17:35
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

I think there's a missing part of a memdb lookup.

@dhiaayachi dhiaayachi requested a review from rboyer May 26, 2022 22:26
@dhiaayachi dhiaayachi requested a review from DanStough May 31, 2022 13:09
Copy link
Contributor

@DanStough DanStough left a comment

Choose a reason for hiding this comment

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

A couple non-blocking things


}

func TestStore_ServiceDefaults_Kind_Destination(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I should probably do an actual diff, but just looking at TestStore_ServiceDefaults_isDestination above and this function, I didn't see the difference.


}

func TestStore_ServiceDefaults_Kind_Destination_Wildcard(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about redundancy with TestStore_ServiceDefaults_isDestination_Wildcard

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right all the isDestination tests are duplicate. I will remove them

}
if sd.Destination != nil {
return structs.GatewayServiceKindDestination, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}
return structs.GatewayServiceKindService, nil

Would this be appropriate? Or are you elsewhere going to interpret "unknown" as "service"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be dealt with in the watch receiving part #13196
Once this PR got merged we will update #13196 to have the right logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I added the unknown kind is to differentiate between existing services/destinations and references in the tgtw to non existing, it will help for debugging purpose.

Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM given that more changes are coming on later PRs.

@dhiaayachi dhiaayachi merged commit 1b77924 into main May 31, 2022
@dhiaayachi dhiaayachi deleted the egress-gtw/tgtw-gateway-services branch May 31, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants