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

Convert to use V1 Ingress API #1416

Merged
merged 4 commits into from
May 4, 2021

Conversation

swetharepakula
Copy link
Member

@swetharepakula swetharepakula commented Apr 23, 2021

This is a large PR and touches many parts of the code. Currently it is based on #1415 , which I will update soon.

While reviewing please keep the following in mind:
1. E2E tests

  • Tests need to pass for Clusters 1.15 - 1.18 if applicable (where v1 API is unavailable)
  • Tests need to pass for Clusters 1.22+ (where v1beta1 API is removed)

2. Ingress Resource Changes
Service.Port (on the Ingress resource) used to be of type IntOrString, but now it is:

type ServiceBackendPort struct with {
    Number int32
    Name string
}
  • Internally the ServiceBackendPort will be used throughout the controller
  • Watch out for the .String() on that port as before it would return the value in string format but now will return the entire struct

3. Any accidental change in behavior due to the conversion or the Service.Port change that was made

4. Backend services in the Ingress spec can now be nil

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 23, 2021
@k8s-ci-robot k8s-ci-robot requested review from bowei and freehan April 23, 2021 22:52
@swetharepakula
Copy link
Member Author

/cc @prameshj @skmatti @spencerhance
/assign @freehan

@k8s-ci-robot
Copy link
Contributor

@swetharepakula: GitHub didn't allow me to request PR reviews from the following users: skmatti, spencerhance.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @prameshj @skmatti @spencerhance
/assign @freehan

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liggitt
Copy link
Member

liggitt commented Apr 26, 2021

in main.go, I'd suggest replacing this:

	ingClassEnabled := flags.F.EnableIngressGAFields && app.IngressClassEnabled(kubeClient)

with a check that requires v1 Ingress and IngressClass and exits with an error if those APIs aren't available, rather than acquiring leader election

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2021
@swetharepakula swetharepakula force-pushed the use-ingress-v1 branch 2 times, most recently from c176fcc to 1d04584 Compare April 27, 2021 19:21
@swetharepakula swetharepakula marked this pull request as ready for review April 27, 2021 19:21
@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 Apr 27, 2021
@liggitt
Copy link
Member

liggitt commented Apr 29, 2021

is there a check early in main.go now to ensure that v1 Ingresses are supported? would that make sense to add to ensure we don't get in a state where we're running with v1 informers that won't sync (xref kubernetes/kubernetes#99840 (comment))

also, this appears to be blocking a successful run of the kubernetes/kubernetes PR that tests without any beta APIs enabled (the gce cluster won't start with this controller crashlooping on extensions/v1beta1 being missing)

@swetharepakula
Copy link
Member Author

There was one more call to ExtensionsV1beta1() that I missed: https://github.com/kubernetes/ingress-gce/blob/master/cmd/glbc/app/namer.go#L164 which was causing the failure in #99840.

I have fixed it in this PR. Once this is merged and a release is cut I will update the image being used in the glbc addon.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021
@swetharepakula swetharepakula force-pushed the use-ingress-v1 branch 2 times, most recently from e0968f1 to 1dbe4ba Compare April 30, 2021 17:02
@swetharepakula swetharepakula force-pushed the use-ingress-v1 branch 2 times, most recently from 99e2c71 to 8846efa Compare April 30, 2021 18:21
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2021
@swetharepakula
Copy link
Member Author

swetharepakula commented May 3, 2021

Only unaddressed comment is about the startup checks for IngressClass and the IngressGAFields. Since this is already a large PR with a lot of subtle changes, I will address the remaining comment in a followup PR.

Otherwise I think this PR is ready for a final set of reviews and is almost ready to be merged.
Thanks everyone for all the reviews and feedback!

@freehan
Copy link
Contributor

freehan commented May 4, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 4, 2021
 - falls back to the v1beta1 API if V1 is not available
 - cmd/fuzzer was converted to use the V1 API solely
  - skip processing backend when service is nil
  - return error when a non-service backend is specified on a GCE
  Ingress
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2021
@freehan
Copy link
Contributor

freehan commented May 4, 2021

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, swetharepakula

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 b1a7452 into kubernetes:master May 4, 2021
@swetharepakula swetharepakula deleted the use-ingress-v1 branch May 5, 2021 00:21
@liggitt
Copy link
Member

liggitt commented May 6, 2021

happy to see the land. is the next step a v1.11.2 and a bump in kubernetes/kubernetes?

@swetharepakula
Copy link
Member Author

We will be cutting a 1.12 release soon. Just finishing up some testing before we cut the tag so should be ready in about a day.

@swetharepakula
Copy link
Member Author

kubernetes/kubernetes#101772 updates the glbc image which should unblock kubernetes/kubernetes#99840

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.

6 participants