feat: refactor i2gw codes and add new features #19

Xunzhuo · 2023-03-24T04:49:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

refactor code base to have a better extensible translator code structure.
init basic testing abilities:
- unit test
- coverage test
- e2e test ( TODO )
add support for version output in cmd
add support for different resource mode: local and remote, to support translating Ingress from files besides from the clusters.
add support for filtering different GWAPI Resources in output
add support for different output type: json and yaml.

Sub of #9

Which issue(s) this PR fixes:

xref: #21

Does this PR introduce a user-facing change?:

Signed-off-by: bitliu <[email protected]>

k8s-ci-robot · 2023-03-24T04:49:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Xunzhuo
Once this PR has been reviewed and has the lgtm label, please assign shaneutt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Xunzhuo · 2023-03-24T05:21:23Z

xref: #21

robscott

Thanks for all the work on this @Xunzhuo! Some great improvements in here.

pkg/cmd/root.go

robscott · 2023-03-24T22:50:14Z

pkg/cmd/testdata/out/from-ingress-to-canary-gwapi.all.json

@@ -0,0 +1,119 @@
+{


Is there an easy way to generate the output files when developing these test cases?

robscott · 2023-03-24T22:52:00Z

pkg/cmd/translate.go

+		Use:   "translate",
+		Short: "Translates Ingress resources into Gateway API resources.",
+		Example: `  # Translate Ingress Resources into Gateway API Resources Locally.
+  i2gw translate --file <input file> --mode local


Is this limited to a single file? If so, it seems like it would be helpful to have an option to find all Ingress manifests in a directory and covert them.

It could be added later I think : )

I think we should lean more into Unix style, and have the tool convert its arguments, with the argument list either being filenames (like i2gw translate ingress1.yaml ingress2.yaml etc) or have i2gw translate able to take from stdin somehow ( we could use the special filename - like kubectl does as well).

If the output can be in a kubectl apply-able format, then you can also make a pipe like ls ingress/*.yaml | xargs i2gw translate | kubectl apply -f - if you want to directly apply in a more Unix-y way.

The trick here is to think about how, if you had 1000 Ingress YAMLs to convert, how would you want to do it?

robscott · 2023-03-24T22:53:00Z

pkg/cmd/testdata/out/from-ingress-to-canary-gwapi.all.yaml

@@ -0,0 +1,62 @@
+Gateways:


I think if I were using this tool I'd prefer to have output that I could translate directly to kubectl apply, this additional structure/nesting would prevent that.

Actually I think this is in more generic since it can be converted into a yaml or json. People can use something like jq to parse it. Or maybe we can have an additional flag like --format which can make the output in the kubecl apply format(Which I think we can add them later). One reason to have a struct to store/nest these resources is that we can filter these GWAPI resources with different types like Gateway, HTTPRoute, etc.

I think that having the objects be separate YAML documents within a single file ( that is, separated with ---) should work just as well though, I think most of the tools will support that, and then it's also directly kubectl apply-able.

robscott · 2023-03-24T22:54:12Z

pkg/cmd/testdata/in/from-ingress-to-multi-gateways-gwapi.yaml

@@ -0,0 +1,53 @@
+apiVersion: networking.k8s.io/v1


It's very common to see Ingress resources mixed in a manifest file with other resources like Services or Deployments. It seems like we should have test coverage to show what happens when that's the input.

robscott · 2023-03-24T23:04:53Z

pkg/translator/annotations.go

+type IngressProvider string
+
+const (
+	IngressNginxIngressProvider IngressProvider = "ingress-nginx"


I think it's reasonable to start with this since we only have one provider right now, but I'd be tempted to split this out into different files or maybe packages for each provider going forward so it was easier for individual implementations to update their own translation code without breaking others.

robscott · 2023-03-24T23:08:00Z

pkg/translator/translator.go

+	return nil
+}
+
+func (t Translator) ConstructIngresses(mode, file string) ([]networkingv1.Ingress, error) {


It would be helpful to have comments describing what this and other exported functions are doing. In this case I'd be tempted to call this ReadIngresses because Construct makes me think we're building new things, not just reading existing things.

robscott · 2023-03-24T23:08:57Z

pkg/translator/util.go

+}
+
+func getInputBytes(inFile string) ([]byte, error) {
+


Suggested change

robscott · 2023-03-24T23:10:08Z

pkg/translator/util.go

+}
+
+// kubernetesYAMLToResources converts a Kubernetes YAML string into Ingresses
+func kubernetesYAMLToResources(str string) ([]networkingv1.Ingress, error) {


Suggested change

func kubernetesYAMLToResources(str string) ([]networkingv1.Ingress, error) {

func readIngressesFromYAML(str string) ([]networkingv1.Ingress, error) {

robscott · 2023-03-24T23:14:10Z

pkg/translator/util.go

+		if err != nil {
+			return nil, err
+		}
+		un := unstructured.Unstructured{Object: obj}


I thought there was an easier way to do this? Especially since we really only care about Ingress resources and can likely ignore anything else. Would something like this work? kubernetes/client-go#193 (comment)

Co-authored-by: Rob Scott <[email protected]>

Signed-off-by: bitliu <[email protected]>

robscott · 2023-06-22T23:38:45Z

@Xunzhuo let us know if you want to follow up on these, for now I'm going to close out in favor of more recent PRs.

refactor: i2gw codes and add new features

3f915a9

Signed-off-by: bitliu <[email protected]>

k8s-ci-robot requested review from robscott and youngnick March 24, 2023 04:49

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 24, 2023

robscott reviewed Mar 24, 2023

View reviewed changes

Xunzhuo mentioned this pull request Mar 27, 2023

The Road to v0.1.0 #21

Closed

45 tasks

Xunzhuo and others added 2 commits March 27, 2023 18:58

Update pkg/cmd/root.go

4245a03

Co-authored-by: Rob Scott <[email protected]>

resolve feedbacks

7735bd1

Signed-off-by: bitliu <[email protected]>

robscott closed this Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor i2gw codes and add new features #19

feat: refactor i2gw codes and add new features #19

Xunzhuo commented Mar 24, 2023 •

edited

Loading

k8s-ci-robot commented Mar 24, 2023

Xunzhuo commented Mar 24, 2023

robscott left a comment

robscott Mar 24, 2023

robscott Mar 24, 2023

Xunzhuo Mar 27, 2023

youngnick Mar 29, 2023

youngnick Mar 29, 2023

robscott Mar 24, 2023

Xunzhuo Mar 27, 2023 •

edited

Loading

youngnick Mar 29, 2023

robscott Mar 24, 2023

robscott Mar 24, 2023

robscott Mar 24, 2023

robscott Mar 24, 2023

robscott Mar 24, 2023

robscott Mar 24, 2023

robscott commented Jun 22, 2023

	func kubernetesYAMLToResources(str string) ([]networkingv1.Ingress, error) {
	func readIngressesFromYAML(str string) ([]networkingv1.Ingress, error) {

feat: refactor i2gw codes and add new features #19

feat: refactor i2gw codes and add new features #19

Conversation

Xunzhuo commented Mar 24, 2023 • edited Loading

k8s-ci-robot commented Mar 24, 2023

Xunzhuo commented Mar 24, 2023

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xunzhuo Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Jun 22, 2023

Xunzhuo commented Mar 24, 2023 •

edited

Loading

Xunzhuo Mar 27, 2023 •

edited

Loading