Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 #1039

rubenvp8510 · 2020-04-26T01:24:09Z

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

codecov · 2020-04-26T01:28:24Z

Codecov Report

Merging #1039 into master will increase coverage by 0.45%.
The diff coverage is 66.92%.

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   64.23%   64.69%   +0.45%     
==========================================
  Files          83       86       +3     
  Lines        6627     7013     +386     
==========================================
+ Hits         4257     4537     +280     
- Misses       2229     2313      +84     
- Partials      141      163      +22

Impacted Files	Coverage Δ
pkg/inventory/ingress.go	`100.00% <ø> (ø)`
pkg/strategy/strategy.go	`95.53% <ø> (-0.67%)`	⬇️
pkg/controller/jaeger/ingress.go	`78.37% <20.00%> (+0.60%)`	⬆️
pkg/autodetect/main.go	`82.35% <25.00%> (-4.36%)`	⬇️
pkg/ingress/ingress.go	`68.75% <68.75%> (ø)`
pkg/ingress/query.go	`100.00% <100.00%> (ø)`
pkg/strategy/all_in_one.go	`90.69% <0.00%> (-4.43%)`	⬇️
pkg/deployment/agent.go	`96.18% <0.00%> (-3.82%)`	⬇️
pkg/deployment/ingester.go	`96.24% <0.00%> (-3.76%)`	⬇️
pkg/deployment/collector.go	`96.77% <0.00%> (-3.23%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2eded8...2a5286f. Read the comment docs.

objectiser · 2020-04-27T08:16:29Z

pkg/ingress/query.go

@@ -3,7 +3,7 @@ package ingress
 import (
 	"fmt"

-	extv1beta1 "k8s.io/api/extensions/v1beta1"
+	extv1beta1 "k8s.io/api/networking/v1beta1"


nit: extv1beta1 -> netv1beta1?

objectiser · 2020-04-27T08:21:52Z

pkg/ingress/query.go

@@ -49,7 +49,7 @@ func (i *QueryIngress) Get() *extv1beta1.Ingress {
 	return &extv1beta1.Ingress{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Ingress",
-			APIVersion: "extensions/v1beta1",
+			APIVersion: "networking/v1beta1",


I think this should be networking.k8s.io/v1beta1, as shown in https://kubernetes.io/docs/concepts/services-networking/ingress/#the-ingress-resource.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

pavolloffay · 2020-04-28T08:45:45Z

Q: should be extensions.ingresses removed from roles? Could auto-detection use the newer/older API based on the platform version as opposed to just disabling the ingress if we it runs on the old platform?

rubenvp8510 · 2020-04-28T17:51:50Z

Could auto-detection use the newer/older API based on the platform version as opposed to just disabling the ingress if we it runs on the old platform?

About the role, I'm still not sure if we need to remove or may be have two versions, for your question about disabling the ingress, yes, the idea is to detect the platform and use the properly API. I added some code that does that, I still need to update the ingress code (and may be the tests).

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 · 2020-05-01T03:38:19Z

Not sure if this is a good solution for use both APIs

I'm using the new Ingress type for all ingress code, but changed to old API just at the time of consuming it. (on create, delete, update etc..)

objectiser · 2020-05-01T09:58:37Z

@rubenvp8510 sounds like a reasonable approach to me.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 · 2020-05-06T17:19:26Z

I added tests to this.

Not sure what should we do with roles, We can create the roles now because both APIs are present in the actual k8s versions, but once k8s removes the extension.ingresses, the role creation might fails.

Should we leave it as it is for now? or create two roles.yaml files one with extension and other with networking? @pavolloffay @objectiser

objectiser · 2020-05-07T09:04:38Z

@rubenvp8510 I think, as this is for supporting older versions of k8s, then having a separate role.yaml (and cluster_role.yaml) is probably fine - just need to make it clear in the install instructions.

pavolloffay · 2020-05-07T09:30:10Z

If the old API groups are noop in the new cluster versions (which I think they are) we can keep it in the single file. It's easier to maintain and easier to deploy for users.

objectiser · 2020-05-07T09:54:51Z

According to this post, k8s 1.22 will stop serving the deprecated API (ingress) - so we still have a bit of time until that happens. So it should be fine to include both in the same file for now as @pavolloffay suggests.

However we would need to check if it is a noop, once 1.22 is out. Or do a test by trying to include a non-existent api.

rubenvp8510 · 2020-05-07T17:03:14Z

If the old API groups are noop in the new cluster versions (which I think they are) we can keep it in the single file. It's easier to maintain and easier to deploy for users.

Definitely, will be easier to maintain, an is the option I would prefer, the only remain question is if it will work on 1.22.

objectiser · 2020-05-11T08:46:11Z

the only remain question is if it will work on 1.22.

@rubenvp8510 Agree, but we have plenty of time before 1.22 is available, so for now we can have the two versions included.

rubenvp8510 · 2020-05-11T14:49:46Z

the only remain question is if it will work on 1.22.

@rubenvp8510 Agree, but we have plenty of time before 1.22 is available, so for now we can have the two versions included.

Yes, just may be we would want to file a ticket to don't forget this?

Also I'm not very confident about tests I did here., those might could start failing when the old API won't be available (or does nothing (noop)). So Do you have any suggestions?

pavolloffay · 2020-05-12T07:26:31Z

+1 on a ticket to remove it in 1.22

We can have an e2e test that checks that the API is available.

Fixed

objectiser · 2020-05-14T13:33:52Z

@kevinearls Could you give this PR a test?

objectiser · 2020-05-14T13:47:57Z

pkg/ingress/ingress.go

+	if viper.Get("ingress-api") == ExtensionAPI {
+		extIngressList := c.fromNetToExt(*obj)
+		err := c.client.Update(ctx, &extIngressList, opts...)
+		if err != nil {


Not necessary, could just return c.client.Update...

objectiser · 2020-05-14T13:48:44Z

pkg/ingress/ingress.go

+	if viper.Get("ingress-api") == ExtensionAPI {
+		extIngressList := c.fromNetToExt(*obj)
+		err := c.client.Delete(ctx, &extIngressList, opts...)
+		if err != nil {


Same comment as above.

objectiser · 2020-05-14T13:48:54Z

pkg/ingress/ingress.go

+	if viper.Get("ingress-api") == ExtensionAPI {
+		extIngressList := c.fromNetToExt(*obj)
+		err := c.client.Create(ctx, &extIngressList, opts...)
+		if err != nil {


Same as above.

objectiser · 2020-05-14T13:55:13Z

pkg/ingress/ingress_test.go

+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(extIngressList.Items))
+
+	// Should return 0 (not using extension API)


Should this say "not using networking API"?

pavolloffay

LGTM, Just minor comments. It would be good to test this though in a real cluster.

pavolloffay · 2020-05-14T13:57:05Z

pkg/autodetect/main.go

@@ -6,6 +6,8 @@ import (
 	"sync"
 	"time"

+	"github.com/jaegertracing/jaeger-operator/pkg/ingress"


it should go to group import 3

pavolloffay · 2020-05-14T14:03:49Z

pkg/ingress/ingress.go

+
+// List is a wrap function that calls k8s client List with extend or networking API.
+func (c *Client) List(ctx context.Context, list *netv1beta.IngressList, opts ...client.ListOption) error {
+	if viper.Get("ingress-api") == ExtensionAPI {


This could be at client init time. I guess nobody will enable extensions API dynamically

objectiser · 2020-05-14T14:09:46Z

pkg/ingress/ingress.go

+		},
+		ObjectMeta: ingress.ObjectMeta,
+		Spec: netv1beta.IngressSpec{
+			Backend: &netv1beta.IngressBackend{


Whet if backend is not defined? In the fromNetToExt ingress.Spec.Backend is checked to see if nil.

kevinearls · 2020-05-14T15:07:30Z

@rubenvp8510 @objectiser Tests have passed on an OCP 4.4 cluster

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

kevinearls · 2020-05-14T15:39:11Z

@rubenvp8510 I ran into one small problem when running on OCP. I get the following log messages every 5 seconds

time="2020-05-14T15:29:25Z" level=info msg="Auto-detected ingress api" ingress-api=networking
time="2020-05-14T15:29:30Z" level=info msg="Auto-detected ingress api" ingress-api=networking

rubenvp8510 · 2020-05-14T15:40:52Z

@kevinearls I think this is due this logs:

https://github.com/jaegertracing/jaeger-operator/pull/1039/files#diff-c2bdb352258b22c51211b662ef5489fbR143

I can remove those log lines or change the detection to another place.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

pavolloffay · 2020-05-14T15:42:44Z

pkg/ingress/ingress.go

+func NewIngressClient(client client.Client, reader client.Reader) *Client {
+	usedAPI := NetworkingAPI
+	if viper.Get("ingress-api") != nil {
+		usedAPI = viper.Get("ingress-api").(string)


there is viper.GetString() so the type assertion is not needed

rubenvp8510 · 2020-05-14T15:43:02Z

@kevinearls @pavolloffay @objectiser I changed the detection place and address the comments.
Could you please review?

Thanks

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1115 branch from 437b3ca to 62157d9 Compare April 26, 2020 01:32

objectiser previously requested changes Apr 27, 2020

View reviewed changes

rubenvp8510 force-pushed the TRACING-1115 branch from 62157d9 to 86e4b5d Compare April 28, 2020 00:46

Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1

ad6a743

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1115 branch from 86e4b5d to ad6a743 Compare April 28, 2020 04:07

rubenvp8510 changed the title ~~Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1~~ WIP Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 Apr 28, 2020

rubenvp8510 force-pushed the TRACING-1115 branch from 7935411 to 14fd011 Compare April 28, 2020 04:48

Add network.k8s.io ingress api detection

f5bc0eb

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1115 branch from 14fd011 to f5bc0eb Compare April 28, 2020 04:49

Add client layer for decide which version of API ingress to use

861723d

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1115 branch from 3620705 to 861723d Compare May 1, 2020 03:17

Ingress new/old api tests

1c4b17a

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 changed the title ~~WIP Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1~~ Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 May 6, 2020

objectiser reviewed May 14, 2020

View reviewed changes

pavolloffay approved these changes May 14, 2020

View reviewed changes

objectiser reviewed May 14, 2020

View reviewed changes

rubenvp8510 added 2 commits May 14, 2020 10:33

Set used api when the client is created, minor format issues

706e57d

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Fix lint issues

61dac4a

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Change the API detection to run once at start

6c653f1

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

pavolloffay reviewed May 14, 2020

View reviewed changes

objectiser approved these changes May 14, 2020

View reviewed changes

pavolloffay mentioned this pull request May 14, 2020

Preparing release v1.18.0 #1062

Merged

rubenvp8510 force-pushed the TRACING-1115 branch 2 times, most recently from 92eaa82 to 50999d0 Compare May 14, 2020 16:18

Avoid string assertion on viper.get

2a5286f

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1115 branch from 50999d0 to 2a5286f Compare May 14, 2020 16:18

pavolloffay merged commit ee0ead3 into jaegertracing:master May 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 #1039

Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 #1039

rubenvp8510 commented Apr 26, 2020

codecov bot commented Apr 26, 2020 •

edited

Loading

objectiser Apr 27, 2020

objectiser Apr 27, 2020

pavolloffay commented Apr 28, 2020

rubenvp8510 commented Apr 28, 2020

rubenvp8510 commented May 1, 2020

objectiser commented May 1, 2020

rubenvp8510 commented May 6, 2020

objectiser commented May 7, 2020

pavolloffay commented May 7, 2020

objectiser commented May 7, 2020

rubenvp8510 commented May 7, 2020

objectiser commented May 11, 2020

rubenvp8510 commented May 11, 2020 •

edited

Loading

pavolloffay commented May 12, 2020

objectiser commented May 14, 2020

objectiser May 14, 2020

objectiser May 14, 2020

objectiser May 14, 2020

objectiser May 14, 2020

pavolloffay left a comment

pavolloffay May 14, 2020

pavolloffay May 14, 2020

objectiser May 14, 2020

kevinearls commented May 14, 2020

kevinearls commented May 14, 2020

rubenvp8510 commented May 14, 2020

pavolloffay May 14, 2020

rubenvp8510 commented May 14, 2020

Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 #1039

Migrate Ingress from API extensions/v1beta1 to networking.k8s.io/v1beta1 #1039

Conversation

rubenvp8510 commented Apr 26, 2020

codecov bot commented Apr 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Apr 28, 2020

rubenvp8510 commented Apr 28, 2020

rubenvp8510 commented May 1, 2020

objectiser commented May 1, 2020

rubenvp8510 commented May 6, 2020

objectiser commented May 7, 2020

pavolloffay commented May 7, 2020

objectiser commented May 7, 2020

rubenvp8510 commented May 7, 2020

objectiser commented May 11, 2020

rubenvp8510 commented May 11, 2020 • edited Loading

pavolloffay commented May 12, 2020

objectiser commented May 14, 2020

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

pavolloffay 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

kevinearls commented May 14, 2020

kevinearls commented May 14, 2020

rubenvp8510 commented May 14, 2020

Choose a reason for hiding this comment

rubenvp8510 commented May 14, 2020

codecov bot commented Apr 26, 2020 •

edited

Loading

rubenvp8510 commented May 11, 2020 •

edited

Loading