fix: restore Gateway API generation (issue #1427). #1431

battlebyte · 2024-11-06T13:41:30Z

Fixes #1427
Separate integration tests in kong2kic_integration_test.go and must be explicitly invoked with -tags=integration.
Fix tests to evaluate all yaml objects.

Separate integration tests in kong2kic_integration_test.go and must be explicitly invoked with -tags=integration. Fix tests to evaluate all yaml objects.

codecov-commenter · 2024-11-06T13:45:40Z

Codecov Report

Attention: Patch coverage is 75.49669% with 74 lines in your changes missing coverage. Please review.

Project coverage is 29.43%. Comparing base (ba07071) to head (c95e91c).

Files with missing lines	Patch %	Lines
kong2kic/route.go	72.28%	63 Missing and 11 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1431      +/-   ##
==========================================
+ Coverage   27.58%   29.43%   +1.84%     
==========================================
  Files          61       61              
  Lines        6285     6537     +252     
==========================================
+ Hits         1734     1924     +190     
- Misses       4423     4474      +51     
- Partials      128      139      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Prashansa-K · 2024-11-07T06:47:56Z

Adding @randmonkey for reviews here as I don't have much context about KIC. Tao will be able to help out in that area.

mheap · 2024-11-18T17:30:19Z

@randmonkey Please review when you can

battlebyte · 2024-11-22T08:53:43Z

@randmonkey let me know if any changes needed.

battlebyte · 2024-12-02T08:19:34Z

Hi @randmonkey , sorry to ping you again. Let please know by when can I expect a review. Just to manage expectations with users.

mheap

Kubernetes changes LGTM. Please take a look at code and PR quality @Prashansa-K

Prashansa-K · 2024-12-10T13:25:07Z

@battlebyte Could you take a look at the failed CI pipelines please?

Prashansa-K · 2024-12-10T13:36:13Z

kong2kic/route.go

+	for _, service := range content.Services {
+		for _, route := range service.Routes {
+			httpRoute, err := createHTTPRoute(service, route)
+			if err != nil {


Just curious: why are we continuing here even with the err?

The input Kong configuration may have many routes that must be converted to HTTPRoute objects. There may be an error when converting one of the routes. If that is the case, we inform of the error and continue processing the next route object.

Prashansa-K · 2024-12-10T13:39:11Z

kong2kic/route.go

+		httpRoute.APIVersion = GatewayAPIVersionV1Beta1
+	}
+	if service.Name != nil && route.Name != nil {
+		httpRoute.ObjectMeta.Name = calculateSlug(*service.Name + "-" + *route.Name)


Are we sure that httpRoute.ObjectMeta is always non-nil?
I am unaware about the underlying struct but if it ever comes nil, our code can panic here.

ObjectMeta is a struct so it is always initialized.

Prashansa-K · 2024-12-10T13:41:50Z

kong2kic/route.go

+	return nil
+}
+
+func createHTTPRoute(service file.FService, route *file.FRoute) (k8sgwapiv1.HTTPRoute, error) {


We do a lot of dereferencing for service and route here. Can we do a nil check beforehand to avoid potential panics?

Provided comments on the concrete potential nil-checking.

Prashansa-K · 2024-12-10T13:46:06Z

kong2kic/route.go

+func addHosts(httpRoute *k8sgwapiv1.HTTPRoute, route *file.FRoute) {
+	if route.Hosts != nil {
+		for _, host := range route.Hosts {
+			httpRoute.Spec.Hostnames = append(httpRoute.Spec.Hostnames, k8sgwapiv1.Hostname(*host))


Same concern as on line 322

Spec is a struct, so it is initialized.

Prashansa-K · 2024-12-10T13:52:12Z

kong2kic/route.go

+		httpRoute.ObjectMeta.Annotations["konghq.com/tags"] = strings.Join(tags, ",")
+	}
+	if route.SNIs != nil {
+		var snis string


Performance wise, it would be better to use a string builder.
I will leave the choice to you. If this piece of code isn't accessed much, we can leave it as is.

Good point, refactored as a string builder.

Prashansa-K · 2024-12-10T13:55:32Z

kong2kic/route.go

+	})
+}
+
+func addBackendRefs(httpRoute *k8sgwapiv1.HTTPRoute, service file.FService, route *file.FRoute) {


Can we please add some comments in this function to ensure that future contributors can understand what needs to be done and why?

Comments added. Let me know if it is clear.

Prashansa-K · 2024-12-10T13:58:05Z

kong2kic/route.go

+
+func addAnnotations(httpRoute *k8sgwapiv1.HTTPRoute, route *file.FRoute) {
+	if route.PreserveHost != nil {
+		httpRoute.ObjectMeta.Annotations["konghq.com/preserve-host"] = strconv.FormatBool(*route.PreserveHost)


Shouldn't we treat the annotation names as constants?

There are two diff functions using the same sort of annotations. It would be better to make these top level constants in my opinion.

Refactored as constants.

Prashansa-K · 2024-12-10T13:58:38Z

kong2kic/route.go

+
+func createHTTPRoute(service file.FService, route *file.FRoute) (k8sgwapiv1.HTTPRoute, error) {
+	var httpRoute k8sgwapiv1.HTTPRoute
+	httpRoute.Kind = "HTTPRoute"


Should we treat this string as a constant?

Refactored as a constant.

Prashansa-K · 2024-12-10T14:00:37Z

kong2kic/route.go

+				ExtensionRef: &k8sgwapiv1.LocalObjectReference{
+					Name:  k8sgwapiv1.ObjectName(kongPlugin.ObjectMeta.Name),
+					Kind:  KongPluginKind,
+					Group: "configuration.konghq.com",


Const string

Refactored as constant.

Prashansa-K · 2024-12-11T06:29:20Z

@battlebyte Just one comment and some linting issues that need to be addressed.
Everything else looks good.

battlebyte · 2024-12-11T10:29:22Z

I don't see the linting issue when running the linter locally, so not sure why it is complaining here.
What is the comment that needs to be addressed?

Prashansa-K · 2024-12-12T06:34:33Z

I don't see the linting issue when running the linter locally, so not sure why it is complaining here.

That could either be due to difference in local linter version vs the one used here. Could you just resolve it based on the error it is showing in the pipeline? I think a comment regarding nolint needs to be removed and it should work fine.

What is the comment that needs to be addressed?
It is on utils.go file

…ressPaths

battlebyte · 2024-12-13T08:37:09Z

@Prashansa-K linter issue fixed.

Prashansa-K · 2024-12-11T06:28:18Z

kong2kic/utils.go

+	KongHQWriteTimeout            = "konghq.com/write-timeout"
+)
+
+const (


why aren't all constants in one block?

I separated the ones that belong to annotations from the rest.

* fix: restore Gateway API generation (issue #1427). Separate integration tests in kong2kic_integration_test.go and must be explicitly invoked with -tags=integration. Fix tests to evaluate all yaml objects. * refactor: replace hardcoded strings with constants in KIC components * fix: remove nolint directive for service port assignment in createIngressPaths --------- Co-authored-by: Prashansa Kulshrestha <[email protected]>

fix: restore Gateway API generation (issue #1427).

c3e02c0

Separate integration tests in kong2kic_integration_test.go and must be explicitly invoked with -tags=integration. Fix tests to evaluate all yaml objects.

Merge branch 'main' into fix/kong2kic-1427

3d71193

battlebyte requested a review from Prashansa-K November 6, 2024 13:54

Prashansa-K requested a review from randmonkey November 7, 2024 06:47

mheap self-requested a review December 9, 2024 13:55

mheap previously approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'main' into fix/kong2kic-1427

35e727c

Prashansa-K reviewed Dec 10, 2024

View reviewed changes

Prashansa-K requested changes Dec 10, 2024

View reviewed changes

refactor: replace hardcoded strings with constants in KIC components

8347e7a

battlebyte dismissed mheap’s stale review via 8347e7a December 10, 2024 16:19

Prashansa-K and others added 3 commits December 12, 2024 17:46

Merge branch 'main' into fix/kong2kic-1427

5f49ae2

Merge branch 'main' into fix/kong2kic-1427

f80409d

fix: remove nolint directive for service port assignment in createIng…

4255dc8

…ressPaths

Merge branch 'main' into fix/kong2kic-1427

c95e91c

Prashansa-K approved these changes Dec 13, 2024

View reviewed changes

Prashansa-K merged commit ca82e63 into main Dec 13, 2024
21 checks passed

Prashansa-K deleted the fix/kong2kic-1427 branch December 13, 2024 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: restore Gateway API generation (issue #1427). #1431

fix: restore Gateway API generation (issue #1427). #1431

battlebyte commented Nov 6, 2024 •

edited

Loading

codecov-commenter commented Nov 6, 2024 •

edited

Loading

Prashansa-K commented Nov 7, 2024

mheap commented Nov 18, 2024

battlebyte commented Nov 22, 2024

battlebyte commented Dec 2, 2024

mheap left a comment

Prashansa-K commented Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K Dec 10, 2024

battlebyte Dec 10, 2024

Prashansa-K commented Dec 11, 2024

battlebyte commented Dec 11, 2024

Prashansa-K commented Dec 12, 2024

battlebyte commented Dec 13, 2024

Prashansa-K Dec 11, 2024

battlebyte Dec 13, 2024

fix: restore Gateway API generation (issue #1427). #1431

fix: restore Gateway API generation (issue #1427). #1431

Conversation

battlebyte commented Nov 6, 2024 • edited Loading

codecov-commenter commented Nov 6, 2024 • edited Loading

Codecov Report

Prashansa-K commented Nov 7, 2024

mheap commented Nov 18, 2024

battlebyte commented Nov 22, 2024

battlebyte commented Dec 2, 2024

mheap left a comment

Choose a reason for hiding this comment

Prashansa-K commented Dec 10, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prashansa-K commented Dec 11, 2024

battlebyte commented Dec 11, 2024

Prashansa-K commented Dec 12, 2024

battlebyte commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

battlebyte commented Nov 6, 2024 •

edited

Loading

codecov-commenter commented Nov 6, 2024 •

edited

Loading