refactor assistance

[twhiston]

Hi,
Firstly thanks for the great project its really cool to see acme in Openshift! Secondly I see you are in the middle of a rewrite of the code, some of the features (retry limiting, global account etc.....) are things that would be really useful to us, so I'd like to offer my help in the refactoring process.-However I don't see a clear outline of how to help with the refactoring on here to just jump in, so 1) are you open to some help? 2) if so what can I help with? :)

[tnozicka]

Hi @twhiston, thanks for the offer to help, always appreciated :)

I think I am closing in and I want to merge the rewrite soon in a minimal form and build features on top of it. Global account is already present and part of the new design. Rate limiting is there from the queue and with pausing the route. Although if you would intentionally try to DOS it, it wouldn't be enough. Proper rate limiting will be one of the first features when this gets merged but I already have the design in my head based on kube cache.ttlstore (similar to kube expectations).

I am not sure if any development time would help before that gets merged and work can be started on features.

What would help most at this point is for you to try it and test if it works for you. Also trying the upgrade scenario would be very helpful. And when I make the PR with rewrite, at least a brief code review would help as well.

[twhiston]

Hi @tnozicka, very happy to help with those things, and contributing features once the rewrite is in main.....
If its ready for some testing right now i would be happy to do that before the start of next week, and always happy to code review, just let me know when you need it.

[tobru]

Same here, we're happy to help, test, contribute. Whatever needed to make it awesome!

[tnozicka]

thanks guys! I have published what I have right now in #48. I need to cleanup the code before code reviews but tests are passing and I would appreciate if you could try it out early and provide feedback.

[schemen]

Hi!

I've tested the clusterwide configuration so far and after some kicking it started working nicely.

• "oc adm policy add-cluster-role-to-user cluster-admin -z openshift-acme" is required, the cluster role does not seem to work. The problem seems to be that he is not allowed to create routes to acme-controller pod

• acme-controller loops if project is being removed during registration #42 seems to be resolved. I have not been able to replicate it now

• Because it is pre-alpha I suppose the output is verbose?

• Before getting it to work, I seemed to struggle with some secrets. I deleted them, recreated pods and at some point things started working

  • ERROR dbcertentry.go:79 [domain: <DOMAIN>, error: 403 urn:acme:error:unauthorized: No registration exists matching provided key]

• Usual operations work so far

Hope this helps as an initial feedback!

[tobru]

I gave the rewrite a test (using the image docker.io/tnozicka/openshift-acme:v0.8.0-pre-alpha) and as far as I can tell it works as expected.

From my point of view, the migration path from the previous version looks like this:

1. CLEANUP: Delete old secrets: acme-account and acme.$NAME from project if using a global acme controller
2. WORKAROUND: Remove all HTTPS configuration from route: spec.tls
3. Wait for controller to request a new certficate and reconfigure the route

When not doing point 2 from above, I get for routes which already have expired certificates:

E0114 16:49:16.869135      14 exposer.go:267] Failed to GET "http://example.com/.well-known/acme-challenge/gxiCyzkbobyP2HeLIwb0I_psuqSizb9zufJ5ssQy0js": Get https://example.com/.well-known/acme-challenge/gxiCyzkbobyP2HeLIwb0I_psuqSizb9zufJ5ssQy0js: x509: certificate has expired or is not yet valid

The only thing I'm currently missing is the feature already mentioned in README: "Creating associated Secrets for Routes containing the certificate".

[twhiston]

I just gave the rewrite a try also using the pre-alpha image and ran in to some issues

Like @tobru I performed the cleanup and workaround steps but then ran in to a different issue

E0116 15:17:30.975550       1 route.go:526] failed to accept ACME authorization: failed to create exposing Endpoints xxxx/kb-ssl-acme-forwarder: endpoints "kb-ssl-acme-forwarder" is forbidden: endpoint address xx.xxx.x.xxx is not allowed
--
  | I0116 15:17:30.975570       1 route.go:527] Dropping Route "xxxx/kb-ssl" out of the queue: failed to accept ACME authorization: failed to create exposing Endpoints xxxx/kb-ssl-acme-forwarder: endpoints "kb-ssl-acme-forwarder" is forbidden: endpoint address xxx.xxx.x.xxx is not allowed

@schemen had the right idea here, the cluster role does not work.... Once I ran "oc adm policy add-cluster-role-to-user cluster-admin -z openshift-acme" it got past the above issue, for sure the error message above does not exactly make this issue clear though ;)

After that it was all good :)

[tnozicka]

@schemen @tobru @twhiston thaks everyone for the feedback!

I am low on time now, but just wanted to let you know I read it and I plan to address your comments fully when I get the time. But at least quickly for now:

@schemen

"oc adm policy add-cluster-role-to-user cluster-admin -z openshift-acme" is required, the cluster role does not seem to work. The problem seems to be that he is not allowed to create routes to acme-controller pod

What openshift version do you use? Have you created the new role from https://github.com/tnozicka/openshift-acme/blob/rewrite/deploy/letsencrypt-staging/cluster-wide/clusterrole.yaml#L14
We test the full setup in CI for v3.6.1 and it works. (I'll try to add 3.7.1 now that's out there, 3.7.0 was broken on travis.)

Because it is pre-alpha I suppose the output is verbose?

You can specify whatever logging level you want (0-9) here https://github.com/tnozicka/openshift-acme/blob/rewrite/deploy/letsencrypt-staging/cluster-wide/deployment.yaml#L33 . I chose higher one to be the default so people deploying it for the first time know what's going on and the more advanced user can always adjust it (lower it).

Before getting it to work, I seemed to struggle with some secrets. I deleted them, recreated pods and at some point things started working
ERROR dbcertentry.go:79 [domain: , error: 403 urn:acme:error:unauthorized: No registration exists matching provided key]

That's an error from the old controller when you mix staging key with live or vice versa

Usual operations work so far

Great!

@tobru

When not doing point 2 from above, I get for routes which already have expired certificates:

Fixed it already - the controller was checking whether the route is exposed and it didn't disable certificate check (there was the old cert for a while).

The only thing I'm currently missing is the feature already mentioned in README: "Creating associated Secrets for Routes containing the certificate".

great; I'll add it once I fix the CI; (easy feature)

@twhiston

endpoint address xx.xxx.x.xxx is not allowed

Hmm, that's weird and shouldn't happen with this new version. The old version used to set IP from service IP range to endpoints (which should be from pod IP range) - that's why it didn't work in Online. This new version sets acme controller's pod's IP into the endpoints so it should work I think. Interested to see the details on this. I didn't have a chance yet to try this on Online.

[tnozicka]

So I tried Online and you it denies you setting even pod's IP into endpoint without having endpoints/restricted permission, which regular user can't set.

There is always the possibility of running a pod for a while to do the redirection without special permissions but I'm trying to avoid it if possible.

[LorbusChris]

I'm guessing it's because they don't want to allow any traffic into the cluster that is not load-balanced.

Definitely am curious how things will look with envoy/istio...

btw I made an APB for deploying OpenShift-ACME:
https://github.com/LorbusChris/openshift-acme-installer
(I called it installer because it is a non-standard APB as it needs cluster-admin priveleges to deploy; it can still be used to manage the entire lifecycle (incl. updates/deprovisioning))

[tnozicka]

I found out it's because it would allow you to avoid network isolation by the network plugin. Testing the workaround now.

@LorbusChris fyi deploying should be really straightforward now with
https://github.com/tnozicka/openshift-acme/blame/rewrite/deploy/letsencrypt-live/cluster-wide/README.md#L15-L16

no need for APB (if there is ever need for it, but I am being biased here)
in deprovisioning you usually delete the namespace or do just something like oc delete all,role -l app=openshift-acme

(I don't think istio will be easy in multitenant environment.)

[LorbusChris]

@tnozicka ah ok interesting.
One of the main reasons I think APBs are cool is that you can create update roles. Simplifies things a lot for non-technical ppl.

Just one more Q: Isn't it possible to put the policy add-cluster-role-to-user in the template as well?
Edit: Just seeing it's not a template. But nonetheless I think one could create a cluster-role-binding directly from yaml.

[tnozicka]

One of the main reasons I think APBs are cool is that you can create update roles. Simplifies things a lot for non-technical ppl.

Not considering other factors, Just the prerequisity of installing ansible for those people is seems like inappropriate. Any user already has oc create, issuing that command seems simple enough to me. But I guess we are drifting away.

if it would be a template you could do that likely by adding this yaml output:
oc policy add-role-to-user edit -z openshift-acme -o yaml