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

feat(kuma-cp): move protocol information to mesh context #8479

Merged
merged 24 commits into from
Dec 11, 2023

Conversation

lukidzi
Copy link
Contributor

@lukidzi lukidzi commented Nov 28, 2023

Checklist prior to review

We called the method InferProtocol in some plugins with Endpoints already filtered by old policies. That caused some of the services not to be available. In this change, I am creating a Map with service information about protocol based on EndpointMap. This allows us to make the calculation once in a mesh context and share it between plugins. Also, we don't need proxy.Routing which we want to avoid using in new policies.

  • Link to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
    • Don't forget ci/ labels to run additional/fewer tests
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label) --

pkg/insights/resyncer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

i don't see any of the backcompat code here. What happens when we upgrade and serviceInsight is not populated? Also what happens when a new service is created and it's not in serviceInsight yet? I'd expect we need to artificially backfill for at least back compat no?

pkg/plugins/runtime/gateway/route/util.go Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/context.go Show resolved Hide resolved
pkg/xds/generator/outbound_proxy_generator.go Outdated Show resolved Hide resolved
Signed-off-by: Lukasz Dziedziak <[email protected]>
Signed-off-by: Lukasz Dziedziak <[email protected]>
Signed-off-by: Lukasz Dziedziak <[email protected]>
Signed-off-by: Lukasz Dziedziak <[email protected]>
@lahabana
Copy link
Contributor

Jakub pointed that service insights are copied from Global to Zone. We should check and understand this.

@lukidzi
Copy link
Contributor Author

lukidzi commented Nov 28, 2023

Jakub pointed that service insights are copied from Global to Zone. We should check and understand this.

Yes, it seems like ServiceInsights calculations are running on Global/Standalone but not Zone. So if there is no connection we might have old data. The code of resyncer is responsible for serviceInsight generation.

We have some options:

  • relay on ServiceInsight - problem when Global is down
  • enable ServiceInsight on zone - we would need to sync to global/create global serviceInsight?
  • use EndpointMap - instead of calling for each policy we can just build this in mesh context once per each service and provide protocol in ServiceInsight only for UI purpose?

Copy link
Contributor

@lahabana lahabana 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 you are right we're trying to do too much at once. Let's remove the deps of plugins on proxy.Routing we can later look not use all the dataplanes to figure the protocol of a service.

pkg/util/protocol/protocol.go Outdated Show resolved Hide resolved
pkg/xds/context/context.go Show resolved Hide resolved
@lukidzi lukidzi changed the title feat(kuma-cp): move protocol information to ServiceInsights feat(kuma-cp): move protocol information to mesh context Nov 29, 2023
Signed-off-by: Lukasz Dziedziak <[email protected]>
@lukidzi lukidzi marked this pull request as ready for review November 29, 2023 16:29
@lukidzi lukidzi requested a review from a team as a code owner November 29, 2023 16:29
@lukidzi lukidzi requested review from jakubdyszkiewicz and lobkovilya and removed request for a team November 29, 2023 16:29
pkg/plugins/policies/core/xds/meshroute/listeners.go Outdated Show resolved Hide resolved
pkg/xds/context/context.go Outdated Show resolved Hide resolved
Signed-off-by: Lukasz Dziedziak <[email protected]>
@bartsmykla bartsmykla self-requested a review December 1, 2023 07:29
Copy link
Contributor

@bartsmykla bartsmykla left a comment

Choose a reason for hiding this comment

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

Some nits

pkg/util/protocol/protocol.go Outdated Show resolved Hide resolved
pkg/util/protocol/protocol.go Outdated Show resolved Hide resolved
pkg/xds/context/context.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
pkg/xds/context/mesh_context_builder.go Outdated Show resolved Hide resolved
Signed-off-by: Lukasz Dziedziak <[email protected]>
@lukidzi lukidzi requested a review from lahabana December 4, 2023 16:35
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry I wanted to sit down to properly look at this change

@lukidzi lukidzi merged commit 4dc57ae into kumahq:master Dec 11, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor InferProtocol
3 participants