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

Add 'class' label and argument, add default email/provider arguments #39

Merged
merged 7 commits into from
Mar 6, 2017

Conversation

whereisaaron
Copy link
Contributor

The -class label allows multiple kcm's in the same namespace(s) for different DNS accounts. If the new -class argument is not specified, then kcm remains backwards compatible. If -class is specified, then the 'enabled: true` annotation is deprecated. See #30 for more details on the behaviour.

Added -tag-prefix and -cert-namespace to allow backwards compatibility after the project moves to a new repository. See #33 for more details.

I've be running multiple instances of this version in pre-production for a bit, working well with both DNS and HTTP challenges.

  • Implement optional 'class' label as a filter within the selected namespace(s), only resources labelled with the configured 'class' label value are managed
  • Implement default provider and default email options, that are used except when overridden by the corresponding Certficate fields or annotations on Ingresses
  • Add log messages when creating or updating a Secret
  • Make prefix for labels and annotations able to be overridden with -tagPrefix
  • Make namespace for Certificate resource able to be overridden with -certNamespace
  • Correct syntax for setting Lego ACME HTTP and HTTPS challenge ports

Insert correct challenge provider into error message
…space(s), only resources labelled with the configured 'class' label value are managed

Implement default provider and default email options, that are used except when overridden by the corresponding Certficate fields or annotations on Ingresses
Add log messages when creating or updating a Secret
Make prefix for labels and annotations able to be overridden with -tagPrefix
Make namespace for Certificate resource able to be overridden with -certNamespace
Correct syntax for setting Lego ACME HTTP and HTTPS challenge ports
@luna-duclos
Copy link

Sorry for the delay on this, I will make sure to review this this weekend

@luna-duclos
Copy link

luna-duclos commented Feb 6, 2017

Everything looks good! One question: can we come up with a better name than "class" ? it doesn't feel super fit for purpose for what it does.

I'd also like to see a documentation update before we release this, but as everything keeps working as before without any arguments, this can potentially wait, though I rather it wouldn't.

@whereisaaron
Copy link
Contributor Author

Thank @luna-duclos!

I considered the label choice pretty carefully and went back and forth on several options before settling on "class" because:

  1. The label needed to be generic enough to cover different use cases. The "class" could be used for e.g. different "environments", "accounts", "clients", "projects", "apps", etc. Using any one, more specific and more prescriptive label neglects the other use cases.
  2. I considered other suitably generic labels like "group", "scope", "category", or "type", but for the almost identical requirement for k8s Ingress Controllers, to run multiple Ingress Controllers and target different Ingress resources, the ingress team settled on kubernetes.io/ingress.class: foo. Given ingress controllers and certificate managers usually go hand-in-hand this strongly influenced choosing "class". This means a project or app can target both its Ingresses and Certificates to the same project or app or environment "class" for both the Ingress Controller and the Certificate Manager, rather using than different terminology for each.

I was careful to keep everything backwards compatible with the old version and existing documentation. I considered also updating the documentation with this, but if I did, I'd want to simultaneously remove or annex the deprecated stuff (like 'enabled') and only document that new way of doing things. If you don't mind that, I can look at the documentation over the next week or so. I'd edit it in Github, so there probably be a huge stack of commits generated. Though you can squash the PR.

@luna-duclos
Copy link

Thanks for this! The class argument sounds good to me when put this way, it makes sense to use the same as ingress.
I would very much appreciate if documentation could be updated, a flurry of commits over the week is totally fine, let's just put everything in this PR, and I'll squash things when you're done

@luna-duclos
Copy link

When would you have time to look at docs ? I was hoping to get this in before dropping a 1.5 version

@whereisaaron
Copy link
Contributor Author

@luna-duclos I have this post-it-note on my monitor, so it is all in hand 😃
Hoping to get to it in the morning. Sorry for the delay.

…fault email and provider arguments

Add documentation of the deprecated Ingress 'enabled' annotation
Add complete documentation of all deployment arguments
Update the documentation and sample 'deployment.yaml' in preparation for a 0.5.0 release
Change the default `-class` argument value to 'default', enabling the class label behavior be default (can set to blank for backwards compatibility)
Add special upgrade note to README.md for people using the old `enabled` annotation behavior
@whereisaaron
Copy link
Contributor Author

Ok @luna-duclos I have updated all the documentation and examples for the 'class' label and the new default email/provider arguments. I updated everything ready for 0.5.0 and made the class label the default approach. I also documented how to enable the deprecated enabled annotation behavior, if upgrading from a previous version. I added a new documentation page for all available deployment arguments.

@luna-duclos
Copy link

Read over it, looks good! Will try it out sunday and merge

@luna-duclos luna-duclos merged commit aeaa115 into PalmStoneGames:master Mar 6, 2017
rporres pushed a commit to rporres/kube-cert-manager that referenced this pull request Nov 6, 2017
As of PalmStoneGames#39, resources need labels so that different kcm instances handle
them.
luna-duclos pushed a commit that referenced this pull request Nov 6, 2017
As of #39, resources need labels so that different kcm instances handle
them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants