Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to new networking.k8s.io/v1beta1 package #4127

Merged
merged 2 commits into from
Jun 13, 2019

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented May 27, 2019

What this PR does / why we need it:

Migrates from the extensions package to the new one networking.k8s.io/v1beta1.
There are some restrictions:

  • We cannot break existing installations in clusters < 1.14
  • This is the reason why e2e tests keep the old package
  • Internally we detect the version and convert the object to the new one
  • After k8s 1.21 (three releases after 1.18 we can change the tests and remove the extensions package)

Migrates from the "k8s.io/api/networking/v1beta1" package to "k8s.io/api/networking/v1beta1" for Kubernetes versions >= 1.14.0 and keeps the extension package for older versions.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

replaces #4081

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 27, 2019
@k8s-ci-robot k8s-ci-robot requested review from bowei and ElvinEfendi May 27, 2019 11:51
@aledbf aledbf force-pushed the migration branch 3 times, most recently from 36912e3 to 90aab26 Compare May 27, 2019 12:30
@cmluciano
Copy link
Contributor

@aledbf Will this replace #4081 ?

@aledbf
Copy link
Member Author

aledbf commented Jun 5, 2019

@cmluciano not sure yet

@codecov-io
Copy link

codecov-io commented Jun 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@aa131b0). Click here to learn what that means.
The diff coverage is 62.69%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4127   +/-   ##
=========================================
  Coverage          ?   57.48%           
=========================================
  Files             ?       87           
  Lines             ?     6534           
  Branches          ?        0           
=========================================
  Hits              ?     3756           
  Misses            ?     2345           
  Partials          ?      433
Impacted Files Coverage Δ
internal/ingress/types.go 0% <ø> (ø)
internal/ingress/controller/store/ingress.go 0% <0%> (ø)
cmd/nginx/main.go 6.57% <0%> (ø)
internal/k8s/main.go 60.37% <0%> (ø)
internal/ingress/annotations/ratelimit/main.go 61.68% <100%> (ø)
internal/ingress/annotations/influxdb/main.go 60% <100%> (ø)
internal/ingress/annotations/cors/main.go 57.14% <100%> (ø)
internal/ingress/annotations/ipwhitelist/main.go 55.17% <100%> (ø)
internal/ingress/annotations/snippet/main.go 100% <100%> (ø)
internal/ingress/annotations/luarestywaf/main.go 60% <100%> (ø)
... and 37 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 aa131b0...5f8deb2. Read the comment docs.

@aledbf
Copy link
Member Author

aledbf commented Jun 5, 2019

@cmluciano ok, pull-ingress-nginx-test job is now passing and only two e2e tests are failing in pull-ingress-nginx-e2e-1-14.

@cmluciano
Copy link
Contributor

Cool, do you think those leader elect failures are related to the api change moving between different versions?

@aledbf
Copy link
Member Author

aledbf commented Jun 9, 2019

Cool, do you think those leader elect failures are related to the api change moving between different versions?

Yes. This error is not present in other passing e2e tests

No. This was related to missing permission to change the configmap when the ingress class is testclass

@aledbf aledbf force-pushed the migration branch 4 times, most recently from 24b69c0 to f56fb5f Compare June 10, 2019 12:31
@aledbf
Copy link
Member Author

aledbf commented Jun 10, 2019

/test pull-ingress-nginx-test

@aledbf aledbf changed the title WIP: Migrate to new networking.k8s.io/v1beta1 package Migrate to new networking.k8s.io/v1beta1 package Jun 10, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2019
@aledbf
Copy link
Member Author

aledbf commented Jun 11, 2019

/test all

@aledbf
Copy link
Member Author

aledbf commented Jun 11, 2019

/retest

@aledbf
Copy link
Member Author

aledbf commented Jun 11, 2019

/test pull-ingress-nginx-e2e-1-12

@aledbf aledbf force-pushed the migration branch 2 times, most recently from 2d80850 to 97ec90e Compare June 12, 2019 22:24
@aledbf
Copy link
Member Author

aledbf commented Jun 12, 2019

/retest

@aledbf aledbf force-pushed the migration branch 2 times, most recently from 8c55ea3 to 8bee39e Compare June 13, 2019 14:57
@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot merged commit 6f12610 into kubernetes:master Jun 13, 2019
@aledbf aledbf deleted the migration branch June 13, 2019 16:29
@aledbf aledbf mentioned this pull request Jun 27, 2019
@nealAR
Copy link

nealAR commented Oct 9, 2019

We have non-RBAC enabled cluster running 1.14.6 on AKS. After upgrading NGINX to 0.25.0 our ingress will no longer resolve to anything but the default route (all other routes currently return a 404). The release notes for 0.25.0 state "please make sure to update the ClusterRole required for the new networking.k8s.io/v1beta1 Ingress API", so I'm thinking the issue may have something do to with this PR? A couple question: 1) does that assumption sound correction given the issues we're having with our ingress? 2) is an update to the ClusterRole required to go along with this PR? 3) if either 1 or 2 are correct how would we resolve the issue for a non-RBAC enabled cluster?

@nealAR
Copy link

nealAR commented Oct 9, 2019

Also are we required to change the apiVersion of the ingress-controller from networking.k8s.io/v1beta1 to networking/v1beta1?

@davidkarlsen
Copy link
Member

davidkarlsen commented Jan 15, 2020

@aledbf
Should the release still run on on k8s 1.13.9 ? I experience that the ingress resources do not get assigned LB address with the upgraded nginx-ingress. Using metallb for providing LoadBalancer IPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants