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

Revert Istio VirtualService support for 1.4 release. #3988

Merged
merged 4 commits into from
May 11, 2021
Merged

Revert Istio VirtualService support for 1.4 release. #3988

merged 4 commits into from
May 11, 2021

Conversation

jakexks
Copy link
Member

@jakexks jakexks commented May 11, 2021

What this PR does / why we need it:

Supporting solving HTTP-01 challenges for Istio VirtualService users is important, so merging @inteon's hard work on #3724 for v1.4 was a good idea to test in an alpha release. However after much discussion in our public dev meetings and standups we decided the technical debt incurred for adding a specific implementation to our core API / codebase isn't such a good idea after all. It only solves Istio when we have received feature requests for gateway-api, gloo, contour, etc. This PR reverts the Istio support changes.

However, there is now agreement amongst the cert-manager maintainers that we should have a well-supported method of using out-of-tree challenge solvers and we will design an API for this feature in in the upcoming 1.5 release (tracked here: #3924 (comment)). It's anticipated that much of the code reverted here will be ported to the out-of-tree solver, as such support for Istio VirtualServices should also land in v1.5

Revert "Merge pull request #3724 from inteon/istio-virtualservice-for-http01"

This reverts commit 80f27739b59884a229bc4a31e3f9867429972d00, reversing
changes made to 96604d02a315f8f976063ed3a0019ab485bcd07c.

Revert "Merge pull request #3939 from JoshVanL/istio-api-to-internal-apis"

This reverts commit f2a74ade5ee9ab28f7c780545c20f9d88887f26b, reversing
changes made to 7ff54e61e97cf622a7a1f4cee141f8773daafd8b.

Revert "Merge pull request #3946 from inteon/fix_kubectl_apply"

This reverts commit c7514d92624613395eb00da8594ffa5391a0b169, reversing
changes made to 49cbedf26285b4bc7b11347c73f6ea2664a8092e.

Special notes for your reviewer:
Apologies to @inteon 😢

Release note:

REMOVE ME WHEN RELEASING! This PR reverts a feature with a release-note,
so ensure the final generated release notes don't contain a reference to the removed feature.

jakexks added 4 commits May 11, 2021 14:50
@jakexks
Revert "Merge pull request cert-manager#3946 from inteon/fix_kubectl_…
8ca19b2 
…apply"

This reverts commit c7514d9, reversing
changes made to 49cbedf.

Signed-off-by: Jake Sanders <i@am.so-aweso.me>
@jakexks
Revert "Merge pull request cert-manager#3939 from JoshVanL/istio-api-…
423e82b 
…to-internal-apis"

This reverts commit f2a74ad, reversing
changes made to 7ff54e6.

Signed-off-by: Jake Sanders <i@am.so-aweso.me>
@jakexks
Revert "Merge pull request cert-manager#3724 from inteon/istio-virtua…
79d8d9c 
…lservice-for-http01"

This reverts commit 80f2773, reversing
changes made to 96604d0.

Signed-off-by: Jake Sanders <i@am.so-aweso.me>
@jakexks
./hack/update-bazel.sh
ef2a830 
Signed-off-by: Jake Sanders <i@am.so-aweso.me>
@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme Indicates a PR directly modifies the ACME Issuer code labels May 11, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakexks

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

@jetstack-bot jetstack-bot added area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code area/api Indicates a PR directly modifies the 'pkg/apis' directory approved Indicates a PR has been approved by an approver from all required OWNERS files. area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 11, 2021
@jakexks
Copy link
Member Author

jakexks commented May 11, 2021

/kind cleanup

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 11, 2021
@jakexks jakexks mentioned this pull request May 11, 2021
Closed
13 tasks
@irbekrm
Copy link
Contributor

irbekrm commented May 11, 2021

Looks good to me.

Sad that we have to put supporting Istio VirtualServices off for a couple of releases, but glad that we will still be able to re-use most of @inteon 's work for the out-of-tree solver.

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2021
@jetstack-bot jetstack-bot merged commit 2aa625e into cert-manager:master May 11, 2021
@jetstack-bot jetstack-bot added this to the v1.4 milestone May 11, 2021
@jakexks jakexks deleted the revert-istio-virtualservice branch May 11, 2021 14:53
@inteon
Copy link
Member

inteon commented May 11, 2021

@jakexks & @irbekrm & @JoshVanL & @wallrj
Indeed, very unfortunate that this cannot be part of cert-manager just yet. #3724 works well for adding Istio VirtualService support, but I understand that a more general approach is needed in cert-manager. I'm happy to hear that other developers will also spend time creating such a solution #3924. Hopefully, a lot of my PR can be repurposed, it will definitely serve as a baseline and provides a reference implementation for Istio.

NOTE: @maelvls, I think my changes to the conformance tests in #3724 can still be applied, maybe you can reuse them in your PR (#3836)

@maelvls maelvls mentioned this pull request May 12, 2021
Merged
@maelvls maelvls mentioned this pull request Jun 15, 2021
Merged
@inteon inteon mentioned this pull request Jun 16, 2021
Closed
@WojciechKarpiel
Copy link

Hi!
What is the timeline for 1.5.0 release?
I very much liked the VirtualService support. Thanks @inteon !

@inteon
Copy link
Member

inteon commented Aug 4, 2021

@WojciechKarpiel see: #4120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@irbekrm irbekrm

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
5 participants
@jakexks @jetstack-bot @irbekrm @inteon @WojciechKarpiel