egctl: add `version` command #914

zirain · 2023-01-16T03:21:03Z

xref: #915

Xunzhuo · 2023-01-16T03:24:51Z

Can you sparate the design docs into issues in case we can better track it or make contributors/maintainers can help implement it? https://github.com/envoyproxy/gateway/blob/main/docs/latest/design/egctl.md @zirain

codecov-commenter · 2023-01-16T03:58:37Z

Codecov Report

Merging #914 (147568f) into main (4262398) will increase coverage by 0.03%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
+ Coverage   64.45%   64.48%   +0.03%     
==========================================
  Files          67       67              
  Lines        8738     8738              
==========================================
+ Hits         5632     5635       +3     
+ Misses       2733     2730       -3     
  Partials      373      373

Impacted Files	Coverage Δ
internal/provider/kubernetes/controller.go	`50.00% <0.00%> (+0.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

internal/ctl/app/version.go

cmd/egctl/main.go

internal/ctl/app/version.go

internal/utils/kube/client.go

internal/cmd/egctl/version.go

cmd/egctl/main.go

internal/cmd/egctl/version.go

danehans · 2023-02-02T20:36:36Z

internal/cmd/egctl/version.go

+	options.AddKubeConfigFlags(flags)
+
+	versionCommand.PersistentFlags().StringVarP(&output, "output", "o", yamlOutput, "One of 'yaml' or 'json'")
+	versionCommand.PersistentFlags().StringVar(&egContainerName, "eg-container-name", "envoy-gateway", "Name of the Envoy Gateway container")


I don't expect the EG container name to change so I'm not sure why the container name needs to be exposed to a user. However, I can see EG being run in a different ns than envoy-gateway-system and expose the ns to the user while defaulting to "envoy-gateway-system".

version will search all namespace, expect vendors may use EG multi-tenancy with custom container name？

I'm not following your point here about the EG container name. For multitenancy, a user would install EG in different namespaces with each EG using a different controllerName. A user would then create a GatewayClass per EG and one or more Gateways that reference the GatewayClass. By default, egctl should expect EG in the envoy-gateway-system namespace with the Deployment name of envoy-gateway. If egctl needs to communicate with an EG in a different namespace, a flag such as --namespace should be surfaced. For security purposes, we should avoid providing egctl access across all namespaces.

let me remove this part first.

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

zirain · 2023-02-03T01:25:33Z

output of running make build && bin/linux/amd64/egctl v

client: v0.2.0
server:
- Name: envoy-gateway-6cbd765bdc-ddqpm
  Namespace: envoy-gateway-system
  envoyGatewayVersion: v0.2.0
  envoyProxyVersion: latest
  gatewayAPIVersion: v0.6.0
  gitCommitID: 54fdd5bc185d8fa57e7afa7ec5f53bb1466757e0

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

danehans · 2023-02-10T17:14:12Z

I just tested egctl and it's showing the incorrect client version. Maybe this PR needs to be rebased?

$ ./bin/darwin/amd64/egctl version
client: v0.2.0
server:
- Name: envoy-gateway-7fd4c7b9cf-jpjrt
  Namespace: envoy-gateway-system
  envoyGatewayVersion: v0.3.0
  envoyProxyVersion: v1.25-latest
  gatewayAPIVersion: v0.6.1
  gitCommitID: f735041d6ce31e104bf00f9a96f47d327634a923

In the future, EG could be running multiple replicas. In this case, which pod name will be displayed? It might be better to surface the deployment name instead.

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

zirain · 2023-02-11T02:10:33Z

I just tested egctl and it's showing the incorrect client version. Maybe this PR needs to be rebased?
$ ./bin/darwin/amd64/egctl version
client: v0.2.0
server:
- Name: envoy-gateway-7fd4c7b9cf-jpjrt
  Namespace: envoy-gateway-system
  envoyGatewayVersion: v0.3.0
  envoyProxyVersion: v1.25-latest
  gatewayAPIVersion: v0.6.1
  gitCommitID: f735041d6ce31e104bf00f9a96f47d327634a923
In the future, EG could be running multiple replicas. In this case, which pod name will be displayed? It might be better to surface the deployment name instead.

after rebasing from main, it's correct now.

danehans · 2023-02-13T16:52:47Z

@zirain thanks for rebasing. I have confirmed that the client/server versions now match. As a follow-on to #914 (comment), EG can run multiple replicas in the future. In this case, which pod name will be displayed? I think it's better to surface the EG deployment name instead of the pod name. Thoughts?

zirain · 2023-02-14T00:39:02Z

In the future, EG could be running multiple replicas. In this case, which pod name will be displayed? It might be better to surface the deployment name instead.

for now, envoy pod will displayed.

danehans · 2023-02-14T05:24:57Z

Why display the Pod name instead of the Deployment name?

zirain · 2023-02-14T06:10:35Z

Why display the Pod name instead of the Deployment name?

if you look at the implement, egctl list pods by selector, then gather server information via PodExec.
BTW, I have no strong option about change to display deployment name.

danehans · 2023-02-14T18:27:16Z

Yes, I understand the implementation. I just don't understand the purpose of displaying the EG pod name instead of the deployment name. I'm fine with resolving this in the future when EG supports multiple replicas.

danehans · 2023-02-14T18:29:59Z

@zirain I can approve after you resolve the merge conflicts.

Xunzhuo · 2023-02-15T04:09:24Z

Yes, I understand the implementation. I just don't understand the purpose of displaying the EG pod name instead of the deployment name. I'm fine with resolving this in the future when EG supports multiple replicas.

I prefer deployment name too, but let us get this in first. We can make it later.

Xunzhuo

Thanks @zirain !

zirain requested a review from a team as a code owner January 16, 2023 03:21

zirain mentioned this pull request Jan 16, 2023

Implement egctl #915

Closed

10 tasks

zirain force-pushed the egctl-1 branch from 3ab25ba to 9d10f26 Compare January 18, 2023 01:11

zirain requested a review from danehans January 18, 2023 02:19

zirain force-pushed the egctl-1 branch from 9d10f26 to cb1ac40 Compare January 28, 2023 02:09

danehans suggested changes Jan 31, 2023

View reviewed changes

zirain mentioned this pull request Feb 1, 2023

rename envoyVersion to envoyProxyVersion #952

Closed

zirain added this to the 0.4.0-rc.1 milestone Feb 2, 2023

danehans suggested changes Feb 2, 2023

View reviewed changes

zirain added 17 commits February 3, 2023 09:19

egctl: initialization

56e2af5

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

add kube client utils

b0cc3bd

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

rebase

1f1617d

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

implement egctl version

154a281

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix lint

6121ec1

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix cmd

a955d4a

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix lint

d198873

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix ut

afc348e

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

reorg pkg ctl

a7e70bb

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix typo

e802d29

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

move to internal/kubernetes

21d6bc3

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

remove remote

6ce6f13

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

reuse GetScheme()

222691d

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

custom egContainerName

355ffe9

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fmt types.NamespacedName

366dd5d

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

fix lint

8e2d1da

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

improve server version

e8a787c

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

zirain force-pushed the egctl-1 branch from d2f542c to fa36e6d Compare February 3, 2023 01:25

fix typo

39a192c

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

zirain force-pushed the egctl-1 branch from fa36e6d to 39a192c Compare February 3, 2023 01:26

go mod tidy

9ad95df

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

zirain requested a review from danehans February 3, 2023 03:05

zirain added 2 commits February 11, 2023 10:06

Merge branch 'main' into egctl-1

5afee94

make eg container name const

73d100b

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>

Merge branch 'main' into egctl-1

147568f

zirain closed this Feb 15, 2023

zirain reopened this Feb 15, 2023

danehans approved these changes Feb 15, 2023

View reviewed changes

Xunzhuo approved these changes Feb 15, 2023

View reviewed changes

Xunzhuo merged commit 7419603 into envoyproxy:main Feb 15, 2023

zirain deleted the egctl-1 branch February 15, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

egctl: add `version` command #914

egctl: add `version` command #914

zirain commented Jan 16, 2023 •

edited

Xunzhuo commented Jan 16, 2023

codecov-commenter commented Jan 16, 2023 •

edited

danehans Feb 2, 2023

zirain Feb 3, 2023

danehans Feb 10, 2023

zirain Feb 11, 2023

zirain commented Feb 3, 2023

danehans commented Feb 10, 2023

zirain commented Feb 11, 2023

danehans commented Feb 13, 2023

zirain commented Feb 14, 2023

danehans commented Feb 14, 2023

zirain commented Feb 14, 2023

danehans commented Feb 14, 2023

danehans commented Feb 14, 2023

Xunzhuo commented Feb 15, 2023

Xunzhuo left a comment

egctl: add version command #914

egctl: add version command #914

Conversation

zirain commented Jan 16, 2023 • edited

Xunzhuo commented Jan 16, 2023

codecov-commenter commented Jan 16, 2023 • edited

Codecov Report

danehans Feb 2, 2023

Choose a reason for hiding this comment

zirain Feb 3, 2023

Choose a reason for hiding this comment

danehans Feb 10, 2023

Choose a reason for hiding this comment

zirain Feb 11, 2023

Choose a reason for hiding this comment

zirain commented Feb 3, 2023

danehans commented Feb 10, 2023

zirain commented Feb 11, 2023

danehans commented Feb 13, 2023

zirain commented Feb 14, 2023

danehans commented Feb 14, 2023

zirain commented Feb 14, 2023

danehans commented Feb 14, 2023

danehans commented Feb 14, 2023

Xunzhuo commented Feb 15, 2023

Xunzhuo left a comment

Choose a reason for hiding this comment

egctl: add `version` command #914

egctl: add `version` command #914

zirain commented Jan 16, 2023 •

edited

codecov-commenter commented Jan 16, 2023 •

edited