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.

Sign up for GitHub

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

Jump to bottom

feat(kuma-dp): Kuma DP + Envoy version in Dataplane Insights (#1112) #1192

Merged
merged 18 commits into from
Dec 8, 2020
Merged

feat(kuma-dp): Kuma DP + Envoy version in Dataplane Insights (#1112) #1192

merged 18 commits into from
Dec 8, 2020

Conversation

jewertow
Copy link
Contributor

@jewertow jewertow commented Nov 22, 2020 •
edited
Loading

Signed-off-by: Jacek Ewertowski [email protected]

Summary

This PR resolves issue #1112.

Full changelog

  • Implement reading envoy version on Kuma DP startup.
  • Send Kuma DP and Envoy version in bootstrap request.
  • Generate bootstrap with KumaDP and Envoy version.
  • Store version of Envoy and Kuma sent in DiscoveryRequest.Node.Metadata

@jewertow jewertow changed the title [WIP] feat(kuma-dp): Send Kuma DP and Envoy version in bootstrap request (#1112) [WIP] feat(kuma-dp): Kuma DP + Envoy version in Dataplane Insights (#1112) Nov 22, 2020
jakubdyszkiewicz
jakubdyszkiewicz reviewed Nov 24, 2020
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

Fantastic work! 👏

I have a couple of nits and questions but overall really nice.

The next step is now to use this info and put it into DataplaneInsights

app/kuma-dp/pkg/dataplane/envoy/envoy.go Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/envoy.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/envoy.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/remote_bootstrap_test.go Outdated Show resolved Hide resolved
@jakubdyszkiewicz
Copy link
Contributor

run make check to cleanup the code.

Don't worry about failing E2E or minikube tests for now. It's on our side

@jewertow
Copy link
Contributor Author

Thank you very much for the code review. I fixed imports and the step make check passes again.
I will continue work on this PR this week.

@jewertow
Copy link
Contributor Author

jewertow commented Nov 26, 2020 •
edited
Loading

I need your help @jakubdyszkiewicz.
I have read the documentation of DiscoveryRequest and I am not sure which property of DiscoveryRequest is a proper place for version. I consider node.metadata or node. user_agent_build_version but I am not sure if a version fits their semantics.

@jakubdyszkiewicz
Copy link
Contributor

Hey @jewertow let's do node.metadata. I looked into Envoy code and don't really see any references for user_agent_build_version in the code so I don't see the benefit of fighting with semver parsing etc.

@jewertow jewertow force-pushed the feat/kumadp-and-envoy-version-in-dataplane-insight branch from f3be97e to cc0c6d3 Compare November 28, 2020 22:17
@jewertow
Copy link
Contributor Author

@jakubdyszkiewicz how command kumactl inspect dataplanes should work?
Should it always return 6 additional columns? Maybe it would be better to add -version/--version flag to this command and return columns with versions when the flag is passed?

@jakubdyszkiewicz
Copy link
Contributor

jakubdyszkiewicz commented Dec 1, 2020 •
edited
Loading

@jewertow you brought a good question about CLI. Let's do this a separate PR and let me create an issue to discuss.

#1244

@jakubdyszkiewicz
Copy link
Contributor

please merge origin/master. CI should be green then

@jewertow jewertow marked this pull request as ready for review December 1, 2020 23:48
@jewertow jewertow requested a review from a team as a code owner December 1, 2020 23:48
@nickolaev nickolaev changed the title [WIP] feat(kuma-dp): Kuma DP + Envoy version in Dataplane Insights (#1112) feat(kuma-dp): Kuma DP + Envoy version in Dataplane Insights (#1112) Dec 2, 2020
@nickolaev
Copy link
Contributor

@jewertow would you mind merging master please? We will consider including this one after we get 1.0.2 released today.

Also, please do to put WIP in the name of the PR, being a draft is already good enough.

@jewertow
Copy link
Contributor Author

jewertow commented Dec 3, 2020

@nickolaev I merged master into this branch.

@jakubdyszkiewicz jakubdyszkiewicz merged commit 8efb495 into kumahq:master Dec 8, 2020
@jewertow jewertow mentioned this pull request Dec 9, 2020
Closed
mergify bot pushed a commit that referenced this pull request Dec 11, 2020
@jewertow
feat(kuma-dp) Kuma DP + Envoy version in Dataplane Insights (#1192)
d98e3b7 
Signed-off-by: Jacek Ewertowski <[email protected]>
(cherry picked from commit 8efb495)

# Conflicts:
#	api/mesh/v1alpha1/dataplane_insight.pb.go
@mergify mergify bot mentioned this pull request Dec 11, 2020
Merged
jakubdyszkiewicz pushed a commit that referenced this pull request Dec 11, 2020
@jewertow @jakubdyszkiewicz
feat(kuma-dp) Kuma DP + Envoy version in Dataplane Insights (#1192)
4d04467 
Signed-off-by: Jacek Ewertowski <[email protected]>
jakubdyszkiewicz pushed a commit that referenced this pull request Dec 11, 2020
@mergify
feat(kuma-dp) Kuma DP + Envoy version in Dataplane Insights (#1192)
625f00a 
Signed-off-by: Jacek Ewertowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubdyszkiewicz jakubdyszkiewicz jakubdyszkiewicz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jewertow @jakubdyszkiewicz @nickolaev