Add option to use networking.k8s.io/v1beta1 ingresses #945

rinx · 2021-01-21T09:38:43Z

Description:

Add usev1beta1 option to use networking.k8s.io/v1beta1 version of ingresses.
Remove useless values yaml
Fix agent-standalone yaml

Related Issue:

Nothing

How Has This Been Tested?:

Nothing

Environment:

Go Version: 1.15.7
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.13.1

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

vdaas-ci · 2021-01-21T09:39:00Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master
🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

rinx · 2021-01-21T09:39:46Z

charts/vald/values.yaml

@@ -995,6 +995,9 @@ gateway:
      # @schema {"name": "gateway.vald.ingress.servicePort", "type": "string"}
      # gateway.vald.ingress.servicePort -- service port to be exposed by ingress
      servicePort: grpc
+      # @schema {"name": "gateway.vald.ingress.usev1beta1", "type": "boolean"}
+      # gateway.vald.ingress.usev1beta1 -- use networking.k8s.io/v1beta1 instead of v1
+      usev1beta1: false


Currently the default value is false. what do you think about it?

I think it's okay, and we should add comment if k8s 1.22 released it would be better to remove this option

kevindiu · 2021-01-21T09:41:30Z

I am thinking the same issue may happen in later k8s also, maybe we can let user to choose which version to use in other k8s resources also.

charts/vald/templates/gateway/backup/ing.yaml

kpango · 2021-01-21T10:24:22Z

how about define vald wide api version definition list in the charts's defaults section like

defaults:
    k8s:
        apiVersion:
            k8s: v1
            apps: apps/v1
            apiextensions-k8s-io: apiextensions.k8s.io/v1
            batch: batch/v1
            policy: policy/v1beta1
            scheduling-k8s-io: scheduling.k8s.io/v1
            autoscaling: v2beta2
            networking-k8s-io: networking.k8s.io/v1
            rbac-authorization-k8s-io: rbac.authorization.k8s.io/v1

and use it

{{- $defaults := .Values.defaults -}}
{{- $gateway := .Values.gateway.lb -}}
{{- if and $gateway.enabled $gateway.ingress.enabled }}
apiVersion: {{ $defaults.k8s.apiVersion.networking-k8s-io }}
kind: Ingress

and users can easy to overwrite specification by helm's helm install --set defaults.k8s.apiVersion.networking-k8s-io=networking.k8s.io/v1beta1

but if some api version breaks backward compatibility we have to add logical section.

kevindiu · 2021-01-21T10:37:24Z

if some api version breaks backward compatibility we have to add logical section.

I starting thinking we should manage k8s manifest by version

kpango · 2021-01-21T10:39:44Z

I starting thinking we should manage k8s manifest by version

yes we should care about it, cuz we provide helm charts for user usually charts provider maintains k8s apiVersion.

rinx · 2021-01-22T01:13:34Z

I think it's not a good idea to allow users to edit api-versions through values.yaml.
It takes too much cost to maintain. Keeping backward compatibility would be really hard.

If the users want to edit them, they should use helm template command to render manifests and edit them manually.

rinx · 2021-01-22T01:15:03Z

Maybe we should define required version of K8s for each version of the charts.

kpango · 2021-01-22T02:06:01Z

I think that specifying apiVersion from values.yaml and implementing conditional branching with usev1beta1 are not so different in maintenance cost, is there any other cost that I am not considering?

kpango · 2021-01-22T02:07:39Z

But, as you mentioned, we shouldn't support backward compatibility about api version. in this document which said helm automatically resolves api version which suitable for current k8s context cluster version.
https://helm.sh/docs/topics/kubernetes_apis/

rinx · 2021-01-22T02:25:59Z

usev1beta1 are not so different in maintenance cost

that's right.

I thought this PR is just for temporary workaround for the problem that is happened in our dev cluster.
After resolved the problem, I'd like to remove the conditional branches from our chart.

kpango · 2021-01-22T02:42:28Z

After resolved the problem, I'd like to remove the conditional branches from our chart.

I understand that this pull request will be used as a temporary workaround.
Also, I think there is no use case for apiVersion to be different for each component, I don't think it is necessary to have a setting for each component, but only for defaults.

kevindiu · 2021-01-22T02:42:37Z

How about this approach?
we will have different version of yaml stored and user can use different version of it.
e.g. we store multiple version of ing.yaml such as ing-v1.yaml and manage them one by one?

kpango · 2021-01-22T02:43:30Z

@kevindiu we can't do that, helm operator will apply everything that he made.

rinx · 2021-01-22T03:04:23Z

In the private chat, Kpango said that it is enough to prepare usev1beta1 only in the defaults section. I agree with him.
So I'm going to revise this PR to have a usev1beta1 in the defaults section and add some comments on that it'll be removed after becoming useless.

…ach gateway

rinx · 2021-01-22T03:17:23Z

I've just updated. Could you please check the changes? @kevindiu @kpango

kpango · 2021-01-22T03:18:25Z

okay, and I opened new issue #946 about this pr's problems.

kpango

LGTM

rinx added 2 commits January 21, 2021 09:33

🔥 Remove useless values-gateways.yaml, Revise agent-standalone yaml

186f360

✨ Add usev1beta1 options to ingresses

b96909f

github-actions bot added area/helm size/S type/refactoring priority/low labels Jan 21, 2021

rinx commented Jan 21, 2021

View reviewed changes

rinx requested a review from kpango January 21, 2021 09:40

kevindiu reviewed Jan 21, 2021

View reviewed changes

charts/vald/templates/gateway/backup/ing.yaml Show resolved Hide resolved

kpango mentioned this pull request Jan 22, 2021

About k8s apiVersion backward compatibility policy in Vald project. #946

Closed

🔧 Add .defaults.ingress.usev1beta1 field and removed that option in e…

93fe045

…ach gateway

kpango approved these changes Jan 22, 2021

View reviewed changes

kpango merged commit 2bfb542 into master Jan 22, 2021

kpango deleted the refactor/helm/add-usev1beta1-option-for-ingresses branch January 22, 2021 03:27

vdaas-ci mentioned this pull request Jan 29, 2021

Bug fix for gateway #963

Merged

18 tasks

kpango mentioned this pull request Feb 2, 2021

[major] v1.0.0 Release #974

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use networking.k8s.io/v1beta1 ingresses #945

Add option to use networking.k8s.io/v1beta1 ingresses #945

rinx commented Jan 21, 2021

vdaas-ci commented Jan 21, 2021

rinx Jan 21, 2021

kpango Jan 21, 2021

kevindiu commented Jan 21, 2021

kpango commented Jan 21, 2021 •

edited

Loading

kevindiu commented Jan 21, 2021 •

edited

Loading

kpango commented Jan 21, 2021

rinx commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kpango commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kevindiu commented Jan 22, 2021

kpango commented Jan 22, 2021

rinx commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kpango left a comment

Add option to use networking.k8s.io/v1beta1 ingresses #945

Add option to use networking.k8s.io/v1beta1 ingresses #945

Conversation

rinx commented Jan 21, 2021

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Jan 21, 2021

rinx Jan 21, 2021

Choose a reason for hiding this comment

kpango Jan 21, 2021

Choose a reason for hiding this comment

kevindiu commented Jan 21, 2021

kpango commented Jan 21, 2021 • edited Loading

kevindiu commented Jan 21, 2021 • edited Loading

kpango commented Jan 21, 2021

rinx commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kpango commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kevindiu commented Jan 22, 2021

kpango commented Jan 22, 2021

rinx commented Jan 22, 2021

rinx commented Jan 22, 2021

kpango commented Jan 22, 2021

kpango left a comment

Choose a reason for hiding this comment

kpango commented Jan 21, 2021 •

edited

Loading

kevindiu commented Jan 21, 2021 •

edited

Loading