Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Adds Certgen Job Support #31

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Adds Certgen Job Support #31

merged 2 commits into from
Oct 13, 2020

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Sep 28, 2020

Adds support for managing the Contour certgen Job.

Fixes #39

@jpeach @skriss @stevesloka @Miciah PTAL

@jpeach
Copy link
Contributor

jpeach commented Sep 29, 2020

Some meta-comments on the approach:

  1. It would be great if there was a Spec option to select whether certgen or cert-manager generates the secrets.
  2. certgen should probably run with a unique (generated?) job name
  3. IIUC most clusters don't have the necessary feature gate turned on, so the operator could delete the certgen job if it is successful
  4. Does the operator even need to run certgen? It could just generate secrets directly?
  5. Interested about your thoughts on keeping stuff like this in sync with the Contour deployment YAML?

@stevesloka
Copy link
Member

I think the operator should just create the certs without running a job, but that package isn't exposed in Contour so we need to do that work first. Duplicating the code doesn't seem like a good approach, but would unblock the operator work. I guess it could also call the contour binary like the certgen job as an another intermediate step.

@jpeach
Copy link
Contributor

jpeach commented Sep 29, 2020

I think the operator should just create the certs without running a job,

Yep, I agree that's a good option. However, I'd also like the operator to allow the possibility of using cert-manager to create the certificates, which implies some config in the contour spec WDYT @danehans ?

@danehans
Copy link
Contributor Author

danehans commented Oct 5, 2020

@jpeach My plan for the operator is to be consistent with https://projectcontour.io/quickstart/contour.yaml and then iterate from there. This will help in setting a baseline and attracting additional contributors. I do like the idea of providing cert gen options and a spec field will be need to expose the options. I think this can be a nice feature to distinguish the operator from other Contour management approaches. Are you OK with this direction? If so, I'll create an issue for managing cert generation.

@jpeach
Copy link
Contributor

jpeach commented Oct 5, 2020

@jpeach My plan for the operator is to be consistent with https://projectcontour.io/quickstart/contour.yaml and then iterate from there. This will help in setting a baseline and attracting additional contributors.

I see, that sounds reasonable to me. Let's capture this approach in the code comments?

@stevesloka
Copy link
Member

The operator should be able to generate certs for a user, this is super helpful to get running quickly as well as fill that gap for those who don't want to deal with their own cert generation, but it is important to allow certs to come from cert-manager, but also to come from standard k8s secrets directly should a user have their own internal system for generating certs.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Added some initial feedback.

controller/contour/job.go Show resolved Hide resolved
controller/contour/job.go Outdated Show resolved Hide resolved
controller/contour/job.go Show resolved Hide resolved
controller/contour/job.go Show resolved Hide resolved
controller/contour/job.go Outdated Show resolved Hide resolved

// currentJob gets the Job for contour from the api server, returning true
// and the Job if successful.
func (r *Reconciler) currentJob(ctx context.Context, contour *operatorv1alpha1.Contour) (bool, *batchv1.Job, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than trying to update the job in place, what I'd suggest is using GenerateName to name the job (IIRC that can be made to work) and labelling it with an owner ref. That way you can always List() to find the certgen jobs you own, and still generate unique names when you need a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

labelling it with an owner ref.

@jpeach the Contour that creates the Job resides in a different namespace, so it can't be used as an owner. Is the original job the owner?

controller/contour/job.go Outdated Show resolved Hide resolved
controller/contour/job.go Outdated Show resolved Hide resolved
controller/contour/controller.go Show resolved Hide resolved
@danehans
Copy link
Contributor Author

danehans commented Oct 8, 2020

@jpeach I've updated the PR based on your feedback, PTAL.

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

const (
jobContainerName = "contour"
jobImageName = "docker.io/projectcontour/contour:main"
Copy link
Contributor

Choose a reason for hiding this comment

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

The job should use the versioned contour container image. The reason we used main historically was that re-applying the YAML would fail on upgrades because that would attempt to modify the job spec. I don't think you gave that issue here since you are recreating the job rather than modifying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpeach I updated the desired Job to use the image from the Config API and removed jobImageName. I would expect the image from Config is set as part of the release process. If not, then the default image tag should be updated when the operator is ready for release.

@danehans danehans force-pushed the job_impl branch 4 times, most recently from 0f6ef73 to 4f4add4 Compare October 13, 2020 19:18
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
@jpeach jpeach merged commit 6cc3422 into projectcontour:main Oct 13, 2020
@danehans danehans deleted the job_impl branch November 2, 2020 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Certgen Support
3 participants