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.

Sign up for GitHub

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

Jump to bottom

feat: add gloo proxy source #1693

Merged
merged 1 commit into from
Mar 18, 2021
Merged

feat: add gloo proxy source #1693

merged 1 commit into from
Mar 18, 2021

Conversation

Hugome
Copy link
Contributor

@Hugome Hugome commented Jul 29, 2020 •
edited

Gloo source integration

Just to justify the use of a dynamic client with some custom redefined type for CRD instead of using the solo-io/gloo client :
If you try to use the solo-io/gloo package, you will have a lot of sub-dependencies issues, they need a huge lot of replace into the go.mod.
Unfortunately even if we want to use only the go types definitions, they use some custom generation tool from the protobuf definitions solo-kit and this generation make use of many sub-sub dependencies.
(If you want a example on how many dependencies : https://docs.solo.io/gloo/latest/guides/dev/example-proxy-controller/ and open the "Click to see the full go.mod file that should be used for this project")

If you can take a rapid look and confirm that your OK with this integration and then I will write tests & documentations.

Fix : #1435 / solo-io/gloo#2467

Missing

  • Tests
  • Documentation

@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 Jul 29, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2020
@Hugome Hugome marked this pull request as ready for review August 4, 2020 23:32
@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 Aug 4, 2020
@Hugome
Copy link
Contributor Author

Hugome commented Aug 4, 2020

/assign @linki

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2020
@seanmalloy
Copy link
Member

/kind feature

@Hugome can you fix the merge conflict with the CHANGELOG.md file?

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2020
@Hugome
Copy link
Contributor Author

Hugome commented Aug 19, 2020

@seanmalloy Yes no problems, done

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 20, 2020
@Raffo
Copy link
Contributor

Raffo commented Aug 26, 2020

@Hugome the linter is complaining, can you take a look?

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2020
@Hugome
Copy link
Contributor Author

Hugome commented Aug 26, 2020

@Raffo Oh sorry didn't see this one, done and rebase done :)

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2020
@jgrumboe
Copy link
Contributor

@Hugome Could you also replace the image registry location from registry.opensource.zalan.do/teapot/external-dns:latest to k8s.gcr.io/external-dns/external-dns:v0.7.3, please?
Within #1741 all other tutorials were already updated.
Thanks

@Hugome
Copy link
Contributor Author

Hugome commented Aug 28, 2020

@jgrumboe Done

@Hugome
Copy link
Contributor Author

Hugome commented Sep 29, 2020

@jgrumboe absolutely right :) Little mistake on a rebase

@graphex-sumo
Copy link

aww just missed 0.7.4.. maybe for 0.7.5?

@Hugome
Copy link
Contributor Author

Hugome commented Oct 26, 2020

@jgrumboe Does this need more changes ?

jgrumboe
jgrumboe approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@jgrumboe jgrumboe left a comment

Choose a reason for hiding this comment

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

From my side it's ok.
But needs lgtm labels from maintainers.

@Hugome
Copy link
Contributor Author

Hugome commented Nov 1, 2020

@Raffo @hjacobs @njuettner do you need some updates/changes on this PR ?
Thx

njuettner
njuettner requested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

some nits, if it works for you I'm happy to merge it

CHANGELOG.md Outdated Show resolved Hide resolved
docs/tutorials/gloo-proxy.md Outdated Show resolved Hide resolved
pkg/apis/externaldns/types.go Outdated Show resolved Hide resolved
source/gloo.go Show resolved Hide resolved
@Hugome
Copy link
Contributor Author

Hugome commented Nov 8, 2020

@njuettner Changes/comments done, let me know if you need more changes.

@Hugome Hugome requested a review from njuettner November 8, 2020 09:44
@ZF-fredericvanlinthoudt
Copy link

@njuettner Any idea when this PR is getting merged and released?
We're currently using Gloo and missing out the DNS automation part...
@Hugome thanks for this PR! 👍

@shaunofneuron
Copy link

@njuettner Any idea when this PR is getting merged and released?
We're currently using Gloo and missing out the DNS automation part...
@Hugome thanks for this PR! 👍

Maybe @Raffo could 👀 ^ ? We understand this is a big project w/ two maintainers and appreciate all your help!

Raffo
Raffo reviewed Dec 23, 2020
View reviewed changes
Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

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

I added a couple of comments, I'm happy to approve this once they are resolved.

docs/tutorials/gloo-proxy.md Outdated Show resolved Hide resolved
docs/tutorials/gloo-proxy.md Show resolved Hide resolved
docs/tutorials/gloo-proxy.md Outdated Show resolved Hide resolved
@Hugome
Copy link
Contributor Author

Hugome commented Dec 23, 2020

Thanks for the test @tsunamishaun
Changes has been done @Raffo
Let me know if you need any more changes :)

@RichiCoder1
Copy link

Not to be that guy, but bump! Would love to see this integrated

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 25, 2021
@Hugome
Copy link
Contributor Author

Hugome commented Jan 27, 2021

@k8s-ci-robot Beep boop, rebase done little 🤖

@jon-walton jon-walton mentioned this pull request Feb 19, 2021
Merged
3 tasks
@tsunamishaun
Copy link

We've been running this feature in production for well over a month now, really surprised this hasn't been merged. Is there anything I can do to help it across the 🏁 ? Thanks to @jon-walton for front-loading the upstream chart with permissions for this (it will be nice to remove our aggregate role!).

@Raffo
Copy link
Contributor

Raffo commented Mar 3, 2021

@Hugome can you rebase this PR? Happy to review it again.

@Hugome
Copy link
Contributor Author

Hugome commented Mar 3, 2021

@Raffo Yes with pleasure, rebase done

@tsunamishaun
Copy link

@njuettner items addressed in review appear to be fixed, can you resolve? Currently no conflicts, can this be re-reviewed @Raffo ? cc @Hugome

@Raffo
Copy link
Contributor

Raffo commented Mar 18, 2021

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Hugome, jgrumboe, 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 Mar 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 030d86c into kubernetes-sigs:master Mar 18, 2021
@Hugome Hugome deleted the gloo-proxy branch March 18, 2021 14:55
@esalter esalter mentioned this pull request Jan 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Raffo Raffo Raffo left review comments

@tsunamishaun tsunamishaun tsunamishaun left review comments

@jgrumboe jgrumboe jgrumboe approved these changes

@hjacobs hjacobs Awaiting requested review from hjacobs

@njuettner njuettner Awaiting requested review from njuettner

Assignees

@linki linki

@Raffo Raffo

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Gloo VirtualService as an Endpoint Source