From bf48480707bb3b31ed182b21754b1d50748cbc35 Mon Sep 17 00:00:00 2001 From: Michi Mutsuzaki Date: Thu, 25 May 2023 23:05:32 +0000 Subject: [PATCH 1/5] Add a CFP for https://github.com/cilium/cilium/issues/25694 Signed-off-by: Michi Mutsuzaki --- ...FP-25694-move-cilium-cli-to-cilium-repo.md | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md diff --git a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md new file mode 100644 index 0000000..d0fa56f --- /dev/null +++ b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md @@ -0,0 +1,108 @@ +# CFP-25694: Move cilium/cilium-cli code into cilium/cilium repository + +**Code Name: Mega Ketchup** + +**SIG: [sig-cilium-cli]** + +**Begin Design Discussion:** 2023-05-25 + +**Cilium Release:** 1.15 + +**Authors:** Michi Mutsuzaki `` + +## Summary + +Move [cilium/cilium-cli] code into [cilium/cilium] repository. + +## Motivation + +The main motivation for moving [cilium/cilium-cli] code into [cilium/cilium] +repository is to minimize the feature development overhead for: + +- adding new tests to `cilium connectivity test`. +- adding new tasks to `cilium sysdump`. + +## Goals + +* Move [cilium/cilium-cli] code into [cilium/cilium] main branch under `cli/` directory. +* Establish a backporting process for changes inside `cli/` directory. +* Don't disrupt the cilium-cli development and release processes. + +## Non-Goals + +* Rename the name of the `cilium-cli` binary from `cilium` to something else. +* Release `cilium-cli` as a part of Cilium releases. + +## Proposal + +This proposal consists of three steps to move [cilium/cilium-cli] code into +[cilium/cilium]: + +### 1. Move [cilium/cilium-cli] code to [cilium/cilium] under `cli/` directory + +I'm assuming that you want to preserve the Git history of [cilium/cilium-cli]. See +[michi-covalent/cilium] on how the code migration might look like. + +### 2. Make [cilium/cilium-cli] code a part of `github.com/cilium/cilium` Go module + +Merge [cilium/cilium-cli] code into `github.com/cilium/cilium` Go module by +removing `cli/go.mod`. This allows [cilium/cilium-cli] code to depend on +[cilium/cilium] code in the same tree. + +This is **not** to suggest `cilium-cli` to be tied to a specific `cilium` +version. The goal of this proposal is to make it easy for contributors to add +tests against the latest Cilium code. We'll continue to maintain the backward +compatibility of `cilium-cli` with respect to `cilium` versions. + +### 3. Release `cilium-cli` from [cilium/cilium-cli] repository + +We do not need to change the release process / cadence to accomplish the goals +defined in this CFP. Continue releasing `cilium-cli` from [cilium/cilium-cli] at +its own cadence using its own versioning that is independent of [cilium/cilium] +versioning as you have been doing in [cilium/cilium-cli] repository. + +See [michi-covalent/cilium-cli] as an example of how [cilium/cilium-cli] repo +might look like after the code migration. Basically it contains: + +- [`go.mod`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/go.mod) +- A simple [`main.go`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/main.go) + that calls [`NewCiliumCommand`](https://github.com/cilium/cilium-cli/blob/44ae1874fae4544c0db34dac89c11e37365b76ef/cli/cmd.go#L27) +- Release [tags](https://github.com/cilium/cilium-cli/tags) and [artifacts](https://github.com/cilium/cilium-cli/releases) + +## Impacts / Key Questions + +### Impact: Backporting to stable branches + +Starting in `v1.15`, you need to backport bug fixes related to +`cilium connectivity test` command from `main` branch to affected stable +branches since these stable branches use their own copy of `cli/` to run +`cilium connectivity test`. Apply the same [backporting process] as the rest of +the Cilium codebase. + +### Impact: Additional overhead for cilium-cli development + +### Impact: Module name change + +This proposal impacts users who depend on [cilium/cilium-cli] as a library. + +- The module path needs to be updated from `github.com/cilium/cilium-cli` to + `github.com/cilium/cilium/cli`. +- Instead of depending on a version of `github.com/cilium/cilium-cli` (`v0.14.5` + for example), you need to inspect `go.mod` of a specific `github.com/cilium/cilium-cli` + version, and transitively find the `github.com/cilium/cilium/cli` version to + depend on. + +### Key Question: Do we lose any test coverage? + +### Key Question: How do we run CI for CLI? More specifically, how do we ensure that changes to Cilium CLI remain compatible with older released versions of Cilium? + +### Key Question: Where do we store the release artifacts? Will the cilium/cilium repository release page contain both CNI and CLI releases? Or do we keep cilium-cli for CLI releases? + +## Alternatives to Consider + +[sig-cilium-cli]: https://github.com/orgs/cilium/teams/sig-cilium-cli +[cilium/cilium]: https://github.com/cilium/cilium +[cilium/cilium-cli]: https://github.com/cilium/cilium-cli +[michi-covalent/cilium]: https://github.com/michi-covalent/cilium/pull/169 +[michi-covalent/cilium-cli]: https://github.com/michi-covalent/cilium-cli/tree/cli-test +[backporting process]: https://docs.cilium.io/en/stable/contributing/release/backports/ From 64b82c01302583ccbc2b2059baaac430076d03d9 Mon Sep 17 00:00:00 2001 From: Michi Mutsuzaki Date: Wed, 31 May 2023 22:37:08 +0000 Subject: [PATCH 2/5] Update the proposal Signed-off-by: Michi Mutsuzaki --- ...FP-25694-move-cilium-cli-to-cilium-repo.md | 85 +++++++++++-------- 1 file changed, 50 insertions(+), 35 deletions(-) diff --git a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md index d0fa56f..e188eec 100644 --- a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md +++ b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md @@ -17,16 +17,33 @@ Move [cilium/cilium-cli] code into [cilium/cilium] repository. ## Motivation The main motivation for moving [cilium/cilium-cli] code into [cilium/cilium] -repository is to minimize the feature development overhead for: +repository is to minimize the feature development overhead for adding new tests +to `cilium connectivity test`. -- adding new tests to `cilium connectivity test`. -- adding new tasks to `cilium sysdump`. +Currently, adding a new test to `cilium connectivity test` requires the following +steps: + +- Open a pull request against [cilium/cilium-cli] repo to add a new test, and + wait for the CI build to finish. +- Modify GitHub workflows files to pick up the CI version of `cilium-cli` in + your pull request against [cilium/cilium] repo. +- Once the new tests passes, undo the change in the workflow files. +- Get these pull requests reveiewed and merged. + +Even after all these steps, the new test does not become a part of the regular +CI until the [sig-cilium-cli] team releases a new version of `cilium-cli` that +includes the test, and Renovate updates GitHub wokflow files to pick up the new +`cilium-cli` version. This has been a major source of frustration for some Cilium +contributors. ## Goals -* Move [cilium/cilium-cli] code into [cilium/cilium] main branch under `cli/` directory. +* Move [cilium/cilium-cli] code into [cilium/cilium] main branch under `cli/` + directory. This enables Cilium contributors to implement new features with + tests in a single pull request against [cilium/cilium] repository. * Establish a backporting process for changes inside `cli/` directory. -* Don't disrupt the cilium-cli development and release processes. +* Avoid disrupting the cilium-cli development and release processes as much as + possible. ## Non-Goals @@ -41,7 +58,8 @@ This proposal consists of three steps to move [cilium/cilium-cli] code into ### 1. Move [cilium/cilium-cli] code to [cilium/cilium] under `cli/` directory I'm assuming that you want to preserve the Git history of [cilium/cilium-cli]. See -[michi-covalent/cilium] on how the code migration might look like. +[michi-covalent/cilium] on how the code migration might look like. Squashing +these commits into a single commit is another option. ### 2. Make [cilium/cilium-cli] code a part of `github.com/cilium/cilium` Go module @@ -51,23 +69,19 @@ removing `cli/go.mod`. This allows [cilium/cilium-cli] code to depend on This is **not** to suggest `cilium-cli` to be tied to a specific `cilium` version. The goal of this proposal is to make it easy for contributors to add -tests against the latest Cilium code. We'll continue to maintain the backward +tests against the latest Cilium code. Continue to maintain the backward compatibility of `cilium-cli` with respect to `cilium` versions. ### 3. Release `cilium-cli` from [cilium/cilium-cli] repository -We do not need to change the release process / cadence to accomplish the goals -defined in this CFP. Continue releasing `cilium-cli` from [cilium/cilium-cli] at -its own cadence using its own versioning that is independent of [cilium/cilium] -versioning as you have been doing in [cilium/cilium-cli] repository. +Set up an automated process to periodically sync `cilium-cli` code under `cli/` +directory back to [cilium/cilium-cli] repo, similar to how Kubernetes syncs some +modules from [kubernetes/kubernetes staging directory] to module repos +([apimachinery] for example). Continue releasing `cilium-cli` from +[cilium/cilium-cli] at its own cadence using its own versioning. -See [michi-covalent/cilium-cli] as an example of how [cilium/cilium-cli] repo -might look like after the code migration. Basically it contains: - -- [`go.mod`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/go.mod) -- A simple [`main.go`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/main.go) - that calls [`NewCiliumCommand`](https://github.com/cilium/cilium-cli/blob/44ae1874fae4544c0db34dac89c11e37365b76ef/cli/cmd.go#L27) -- Release [tags](https://github.com/cilium/cilium-cli/tags) and [artifacts](https://github.com/cilium/cilium-cli/releases) +See [Tobias' comment](https://github.com/cilium/design-cfps/pull/9#discussion_r1211996796) +for more details. ## Impacts / Key Questions @@ -79,26 +93,25 @@ branches since these stable branches use their own copy of `cli/` to run `cilium connectivity test`. Apply the same [backporting process] as the rest of the Cilium codebase. -### Impact: Additional overhead for cilium-cli development - -### Impact: Module name change - -This proposal impacts users who depend on [cilium/cilium-cli] as a library. - -- The module path needs to be updated from `github.com/cilium/cilium-cli` to - `github.com/cilium/cilium/cli`. -- Instead of depending on a version of `github.com/cilium/cilium-cli` (`v0.14.5` - for example), you need to inspect `go.mod` of a specific `github.com/cilium/cilium-cli` - version, and transitively find the `github.com/cilium/cilium/cli` version to - depend on. - -### Key Question: Do we lose any test coverage? +### Key Question: How do you run CI for CLI? More specifically, how do you ensure that changes to Cilium CLI remain compatible with older released versions of Cilium? -### Key Question: How do we run CI for CLI? More specifically, how do we ensure that changes to Cilium CLI remain compatible with older released versions of Cilium? +Here is the list of GitHub workflows that currently run for each pull requests in [cilium/cilium-cli] +using the latest version of Cilium: -### Key Question: Where do we store the release artifacts? Will the cilium/cilium repository release page contain both CNI and CLI releases? Or do we keep cilium-cli for CLI releases? +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/aks-byocni.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/eks-tunnel.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/eks.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/externalworkloads.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/gke.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/kind.yaml +- https://github.com/cilium/cilium-cli/blob/main/.github/workflows/multicluster.yaml -## Alternatives to Consider +[cilium/cilium] has a similar coverage using CI images instead of released images, +so it's probably an overkill to port all of these workflows to [cilium/cilium]. My +proposal is to move https://github.com/cilium/cilium-cli/blob/main/.github/workflows/kind.yaml +to [cilium/cilium], and run this workflow for pull requests that modify files +under `cli/` directory. This ensures that any changes under `cli/` directory get +tested against a released version of Cilium. [sig-cilium-cli]: https://github.com/orgs/cilium/teams/sig-cilium-cli [cilium/cilium]: https://github.com/cilium/cilium @@ -106,3 +119,5 @@ This proposal impacts users who depend on [cilium/cilium-cli] as a library. [michi-covalent/cilium]: https://github.com/michi-covalent/cilium/pull/169 [michi-covalent/cilium-cli]: https://github.com/michi-covalent/cilium-cli/tree/cli-test [backporting process]: https://docs.cilium.io/en/stable/contributing/release/backports/ +[kubernetes/kubernetes staging directory]: https://github.com/kubernetes/kubernetes/tree/master/staging/ +[apimachinery]: https://github.com/kubernetes/apimachinery From d7b9338d590a79a810c9a0fb36d541d1f29a16da Mon Sep 17 00:00:00 2001 From: Michi Mutsuzaki Date: Mon, 5 Jun 2023 21:04:59 +0000 Subject: [PATCH 3/5] Back to the original proposal Signed-off-by: Michi Mutsuzaki --- ...FP-25694-move-cilium-cli-to-cilium-repo.md | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md index e188eec..5f5e69e 100644 --- a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md +++ b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md @@ -74,14 +74,18 @@ compatibility of `cilium-cli` with respect to `cilium` versions. ### 3. Release `cilium-cli` from [cilium/cilium-cli] repository -Set up an automated process to periodically sync `cilium-cli` code under `cli/` -directory back to [cilium/cilium-cli] repo, similar to how Kubernetes syncs some -modules from [kubernetes/kubernetes staging directory] to module repos -([apimachinery] for example). Continue releasing `cilium-cli` from -[cilium/cilium-cli] at its own cadence using its own versioning. +We do not need to change the release process / cadence to accomplish the goals +defined in this CFP. Continue releasing `cilium-cli` from [cilium/cilium-cli] at +its own cadence using its own versioning that is independent of [cilium/cilium] +versioning as you have been doing in [cilium/cilium-cli] repository. -See [Tobias' comment](https://github.com/cilium/design-cfps/pull/9#discussion_r1211996796) -for more details. +See [michi-covalent/cilium-cli] as an example of how [cilium/cilium-cli] repo +might look like after the code migration. Basically it contains: + +- [`go.mod`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/go.mod) +- A simple [`main.go`](https://github.com/michi-covalent/cilium-cli/blob/865cac4f148ce88cd04d99f8ecfe61a0ae4f645f/main.go) + that calls [`NewCiliumCommand`](https://github.com/cilium/cilium-cli/blob/44ae1874fae4544c0db34dac89c11e37365b76ef/cli/cmd.go#L27) +- Release [tags](https://github.com/cilium/cilium-cli/tags) and [artifacts](https://github.com/cilium/cilium-cli/releases) ## Impacts / Key Questions @@ -93,6 +97,24 @@ branches since these stable branches use their own copy of `cli/` to run `cilium connectivity test`. Apply the same [backporting process] as the rest of the Cilium codebase. +### Impact: Module name change + +This proposal impacts users who depend on [cilium/cilium-cli] as a library. + +- The module path needs to be updated from `github.com/cilium/cilium-cli` to + `github.com/cilium/cilium/cli`. +- Instead of depending on a version of `github.com/cilium/cilium-cli` (`v0.14.5` + for example), you need to inspect `go.mod` of a specific `github.com/cilium/cilium-cli` + version, and transitively find the `github.com/cilium/cilium/cli` version to + depend on. +- Alternatively, depend on a particular `github.com/cilium/cilium` version without + inspecting `go.mod` file in `github.com/cilium/cilium-cli`. + +If you must retain the Go module name, you need to copy the `cilium-cli` code +back to [cilium/cilium-cli] repo and modify all the import paths back to +`github.com/cilium/cilium-cli` for each release.. +See https://github.com/cilium/design-cfps/pull/9#discussion_r1211996796 for details. + ### Key Question: How do you run CI for CLI? More specifically, how do you ensure that changes to Cilium CLI remain compatible with older released versions of Cilium? Here is the list of GitHub workflows that currently run for each pull requests in [cilium/cilium-cli] From 3b5b719b9fac007d2ea683b18aafe4b3cb62e1eb Mon Sep 17 00:00:00 2001 From: Michi Mutsuzaki Date: Wed, 21 Jun 2023 22:31:37 +0000 Subject: [PATCH 4/5] Add another approach as an option Signed-off-by: Michi Mutsuzaki --- ...FP-25694-move-cilium-cli-to-cilium-repo.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md index 5f5e69e..07af9fe 100644 --- a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md +++ b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md @@ -135,6 +135,27 @@ to [cilium/cilium], and run this workflow for pull requests that modify files under `cli/` directory. This ensures that any changes under `cli/` directory get tested against a released version of Cilium. +## Alternative approach: Extend connectivity test from cilium/cilium repo + +Sebastian Wicki suggests another approach to extend `cilium connectivity test` +command from [cilium/cilium] repo using cilium-cli's [Hook interface]. This +approach involves two steps: + +### Step 1: Implement the Hook interface in cilium/cilium repo. + +See [this commit](https://github.com/cilium/cilium/commit/26526a1890de6e05ca58d37b5a2da5822a3f22f0) +as an example for how to implement the [Hook interface]. This enables you to +extend `cilium connectivity test` without having to migrate the entire [cilium/cilium-cli] +code base into [cilium/cilium] repo. Then, build a local version of cilium-cli +with the additional hook during the CI. + +### Step 2: Import the hook back in cilium/cilium-cli repo. + +Import the hook from the previous step in [cilium/cilium-cli] repo so that +additional tests in [cilium/cilium] repo get included in upstream cilium-cli +releases. See [this commit](https://github.com/cilium/cilium-cli/commit/7b474e61be358fb0b9f2cd6fd075ba843b5d78f5) +as an example. + [sig-cilium-cli]: https://github.com/orgs/cilium/teams/sig-cilium-cli [cilium/cilium]: https://github.com/cilium/cilium [cilium/cilium-cli]: https://github.com/cilium/cilium-cli @@ -143,3 +164,4 @@ tested against a released version of Cilium. [backporting process]: https://docs.cilium.io/en/stable/contributing/release/backports/ [kubernetes/kubernetes staging directory]: https://github.com/kubernetes/kubernetes/tree/master/staging/ [apimachinery]: https://github.com/kubernetes/apimachinery +[Hook interface]: https://github.com/cilium/cilium-cli/blob/4a6cb76243704f96dfc02ff312b57e4d0ced0d84/internal/cli/cmd/hooks.go#L13-L17 From 5bd1494530c2ae2c8bcc81af80a3370583ee9c85 Mon Sep 17 00:00:00 2001 From: Michi Mutsuzaki Date: Fri, 16 Aug 2024 21:10:56 +0900 Subject: [PATCH 5/5] Update cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md Co-authored-by: Bill Mulligan Signed-off-by: Michi Mutsuzaki --- cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md index 07af9fe..63f9c14 100644 --- a/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md +++ b/cilium/CFP-25694-move-cilium-cli-to-cilium-repo.md @@ -10,6 +10,8 @@ **Authors:** Michi Mutsuzaki `` +**Status:** Implementable + ## Summary Move [cilium/cilium-cli] code into [cilium/cilium] repository.