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

Replace external issuers page with list of ALL issuers #1324

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

inteon
Copy link
Member

@inteon inteon commented Oct 12, 2023

This PR aims to achieve the following:

  1. give a full overview of a all issuers (have the same visibility for in-tree and external issuers)
  2. add a tier system that indicates the quality of issuers
  3. list the criteria that are used to rank issuers & make sure these criteria are achievable for anyone

Preview: https://deploy-preview-1324--cert-manager-website.netlify.app/docs/configuration/issuers/

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 12, 2023
@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 7527bbf
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/655f692c1287630008b94ef2
😎 Deploy Preview https://deploy-preview-1324--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@inteon inteon force-pushed the cleanup_issuers branch 2 times, most recently from 2dfc7dd to 87bb6fc Compare October 13, 2023 17:00
@SgtCoDFish
Copy link
Member

As discussed in standup: I find the current layout pretty unreadable. Firefox 118, macOS 14 Sonoma.

Screenshot 2023-10-16 at 11 53 16

@inteon
Copy link
Member Author

inteon commented Oct 16, 2023

I think the current PR is ready for feedback.
For the authors of some of these issuers:

  • Do you think it would be possible to reach the 🥇-tier soonish?
  • Do you think the tier-system fair?
  • Is there something missing from the table/ tier system?

We would like to get some more issuers into the 🥇-tier before merging the PR.
@m8rmclaren @bmsiegel @amibhi @dopey

@hawksight
Copy link
Member

My notes form Slack:

I think it looks good.
I quite like that the cert-manager version in the tutorial is linked and forces maintainers to keep the tutorial up to date by re checking at least every 2 releases 🙂
I can't think of any more criteria, so I'd say it'd be good to have this as a start and refine later as needed.

@m8rmclaren
Copy link
Contributor

Hey everyone, in general I support the move to a tiered system for cert-manager issuers/clusterissuers; it's a mature move and could result in higher adoption in general since all issuers are compiled to a single list.

I have a few comments/suggestions to add some clarity to the new system.

Release cadence required for 🥈-tier support

External issuers are well-defined extensions to cert-manager and don’t typically require new features after the initial release. In general, there are three overarching reasons for a new release of an external issuer:

  1. Cert-manager API changes – the cert-manager CertificateRequest API changes, requiring an adjustment to the CertificateRequest controller included with each external issuer.
  2. External issuer API changes – The target APIs that the external issuers use for certificate issuance change, requiring a code change to accommodate the new API version.
  3. Code bugs – Bugs exist in the code that require code changes.

The cert-manager CertificateRequest API is not likely to change, and in most cases target APIs won’t make breaking changes requiring a new release, even less likely within a given three-month release cycle.

Further, following cert-manager’s release cycle, even in-tree issuers would have been marked stale (tier-3) between cert-manager 1.12-1.13, since more than three months elapsed between releases.

I would propose a relaxed release cycle for external issuers targeting tier-2 status or remove the 3-month criterion and consider having only two tiers.

Lacking clear definition of “end-to-end”

In general, I agree with @hawksight about forcing maintainers to keep their documentation up-to-date, especially because there's nothing worse than following misleading docs. However, in general, documentation written against a supported version of software for production use typically implies active maintenance anyway.

My comment is that if tier-1 issuers will require an end-to-end tutorial, I think that there needs to be a clear definition of "end-to-end."

Ambiguity with result of tier-1 issuer failing tier-2 issuer requirements

In many cases, the requirements for tier-2 seem more stringent than those for tier-1. For example, since cert-manager releases are typically supported for more than 3 months, a given tier-1 issuer would usually fail the tier-2 requirement for a recent release BEFORE failing the tier-1 requirement of providing documentation with a supported version of cert-manager.

So, the question then becomes if a tier-1 issuer with end-to-end documentation on configuration for production environments misses the three-month release cycle, do they get marked as a tier-3 issuer?

Cheers!
-H

@inteon
Copy link
Member Author

inteon commented Oct 26, 2023

Thanks @m8rmclaren!

I agree that the tier-2 requirements might have been too strict. I applied the simplest fix: extend the 3 months requirement to 12 months.

Also, thank you for explaining the lack of clarity wrt. an "e2e tutorial" I added a list of things that must be included in your tutorial for it to be an e2e tutorial.

You can see the changes here: 103eac7
PTAL & let me know what you think.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2023
…s to indicate the quality of issuers and their documentation

Signed-off-by: Tim Ramlot <[email protected]>
…s to 12 months & add more info about what the e2e docs should look like

Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023
Signed-off-by: Tim Ramlot <[email protected]>
@inteon
Copy link
Member Author

inteon commented Nov 23, 2023

Looks like all remaining feedback has been resolved.
This PR is ready to be merged.
Additional improvements can still be made in subsequent PRs.

@maelvls
Copy link
Member

maelvls commented Nov 23, 2023

I don't think closed-source issuers belong to this page. I'm OK having venafi-enhanced-issuer shown at the bottom of the page in a "Commercial issuers".

@inteon
Copy link
Member Author

inteon commented Nov 24, 2023

I don't think closed-source issuers belong to this page. I'm OK having venafi-enhanced-issuer shown at the bottom of the page in a "Commercial issuers".

I think all issuers are commercial. The advantage of an open-source issuer integration will often not be significant compared to a closed-source integration, because none of these integrations are actually usable if you are not a commercial customer of the corresponding CA. The advantage of an open-source integration is that you can more easily contribute and audit the integration (note that this is often still possible for closed-source integrations too).
Note that the advantages are not comparable to cert-manager being open-source, because there a lot of the value comes from being CA-agnostic and offering an open interface that can integrate with multiple different CAs. The cert-manager community is very powerful because it is a combination of users with all kinds of use cases, using multiple different CAs.

@maelvls
Copy link
Member

maelvls commented Nov 24, 2023

I think I lack objectivity in this discussion, and no one from the end-users has voiced concerns or agreement. We have already asked in #cert-manager but no one chimed in... 😅

@maelvls
Copy link
Member

maelvls commented Nov 24, 2023

After asking some folks in the community, I think it is OK to self-promote closed-source products as long as other commercial actors are also able to do that too. Since the criteria for being promoted as "top tier" have been explicitly laid out, I am in favor of approving your PR.

/lgtm

I'd like to also state that other commercial offerings will want to have their logo to show in the landing page as Venafi does. We should also make it clear what the rules to appear on the landing page are.

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2023
@inteon
Copy link
Member Author

inteon commented Nov 24, 2023

/approve

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2023
@jetstack-bot jetstack-bot merged commit 2c72ba0 into cert-manager:master Nov 24, 2023
4 checks passed
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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants