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

Contour HTTPProxy support #1628

Merged

Conversation

josephglanville
Copy link
Contributor

@josephglanville josephglanville commented Jun 12, 2020

This PR implements support of the Contour HTTPProxy CRD in external-dns using the new LoadBalancer field in Status to propagate endpoints to external-dns.

This requires Contour v1.5.0

Fixes #1366

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 12, 2020
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 15, 2020
@josephglanville josephglanville changed the title WIP: Contour HTTPProxy support Contour HTTPProxy support Jun 16, 2020
@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 16, 2020
@josephglanville
Copy link
Contributor Author

josephglanville commented Jun 16, 2020

/cc @stevesloka if you get a chance could you have a look at this? For the most part I tried to re-use what I could from the IngressRoute based source including the same tests. The main difference being instead of specifying which LB service to pull endpoints from we use the LB status on the HTTPProxy objects.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2020
@josephglanville
Copy link
Contributor Author

/assign @linki

@stevesloka
Copy link
Contributor

Hey @josephglanville, I'll poke around with this next week and have a run in my cluster.

@josephglanville
Copy link
Contributor Author

Awesome. I will try extend the existing Contour docs during the week too with the additional/alternate RBAC rules.

@josephglanville
Copy link
Contributor Author

@stevesloka I pushed an update that adds the RBAC rules to the docs and HTTPProxy example.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020
@josephglanville
Copy link
Contributor Author

Now that #1627 is merged this is ready for review.

@nikhita
Copy link
Member

nikhita commented Jul 14, 2020

@linki PTAL :)

Copy link
Contributor

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm Nice work @josephglanville!! 🎉

Additionally, we should make a plan to remove the IngressRoute support as it's fully deprecated now in Contour, but can wait for the next release to do that work.

source/httpproxy.go Outdated Show resolved Hide resolved
@voor
Copy link
Member

voor commented Aug 19, 2020

Really excited for this PR, will help simplify annotations on the Contour Load Balancer Service!

@seanmalloy
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 19, 2020
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
source/httpproxy.go Outdated Show resolved Hide resolved
@tariq1890
Copy link
Contributor

@josephglanville Sorry I couldn't get back to this review sooner. Provided comments around error handling as it's better to use one package (going with github.com/pkg/errors since it's already imported).

Can we also have a barebones doc? It doesn't have to be all fleshed out. What I am requesting for is some manifests to help get the user started. A lot of external-dns users face issues from forgetting to specify the right RBAC, for e.g:- We would need to specify get, list as well as watch perms for contour httpproxies since we are using shared informers.

@josephglanville
Copy link
Contributor Author

I did have docs but I think they got lost in a rebase rejiggering or moving between my machines. I will add those and attend to your review comments shortly. Thanks @tariq1890.

@tariq1890
Copy link
Contributor

/lgtm

Cc @njuettner @Raffo for final approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2020
@seanmalloy
Copy link
Member

/assign @Raffo @njuettner
/unassign @linki

@k8s-ci-robot k8s-ci-robot assigned njuettner and Raffo and unassigned linki Aug 27, 2020
@Raffo
Copy link
Contributor

Raffo commented Sep 7, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: josephglanville, Raffo

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0947994 into kubernetes-sigs:master Sep 7, 2020
@josephglanville josephglanville deleted the jpg/contour-httpproxy branch September 9, 2020 13:22
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. kind/feature Categorizes issue or PR as related to a new feature. 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.

Project Contour HTTPProxy support?