Certgen should generate secrets that are compatible with cert-manager

[jpeach]

As noted by @tsaarni in #2149, the secret that certgen generates isn't interchangeable with how cert-manager generates secrets.

Maybe we can update certgen to be compatible with cert-manager, which would make it easier for sites to switch.

[tsaarni]

Aligning with cert-manager would fix the problem between certgen and cert-manager.

Since there is no standard, the way how secrets are generated is implementation specific and cert-manager is just another take on this, just like certgen. Aligning would allow us to support both certgen and cert-manager, but obviously it still isn't a generic solution - but I doubt there is good generic solutions for this.

Depending how people are using certgen and/or example manifest, there can be backwards compatibility issues to be considered if doing the change.

[jpeach]

This patch switches contour certgen over to generating standard TLS secrets compatible with cert-manager.

As @tsaarni points out, the trick here is migrating upgrading existing clusters. Since the certgen job only runs once, the xDS secrets from the previous version will not be updated. This means that any changes we make to the deployment will also break since the files referenced by the command flags won't be present.

Since the new command doesn't generate a separate secret for the CA cert, online migration is a problem. The only alternative I can think of is forcing the updated certgen job to run (maybe by adding a version suffix to the job name). This will also force an envoy restart, which might not be desirable.

/cc @youngnick

[youngnick]

I don't think there's any way around this being a breaking change. The only way here is to just make it all explicit:

• certgen Job gets a new name
• secrets get new names
• Contour deployment and Envoy Daemonset updated to use new secrets

Upgrade instructions include clear direction about what the difference is, why it's been done, why you won't need to do this again (ie now the certs are compatible with cert-manager, can be rotated transparently, etc), and that upgrading requires restarts of Contour and Envoy,

Then, instructions on how to clean up the old stuff.

That way, operators who are using our examples directly will understand what they need to do, and others who aren't will have a clear picture of the steps to do.

[jpeach]

OK I went down a bit of a rabbit hole on this. You can't use generateName on the certgen job as per kubernetes/kubernetes#44501. However if we publish YAML via kustomize, we can use that to make an equivalent change. If we did this, then certgen could run on each Contour install. Although the TTL Controller is probably not enabled on most clusters, I expect operators would be able to deal with one extra Job object per month.

However, in the kustomize PR, since we place certgen in a different base (to make it more easily decoupled), it's not so easy to change the name of the secrets (which need to be consistent across the certgen, contour and envoy kustomize bases). However, if xDS certificate rotation is working well after landing all of @tsaarni 's changes, maybe each certgen run could overwrite the secrets, which would be a general improvement.

[jpeach]

I'm working on this, because we probably need to rationalize this in order to ship a release where operators can reload xDS certificates in a reasonable way (i.e. without having to vendor large chunks of Contour YAML).

Basically working, barring a race around doing the update.

• There's some issue in my kind setup where it doesn't run the right Contour version in the certgen job which means that the new --overwrite flag doesn't work
• If the new Secret isn't laid down when Envoy is restarted, then it will never pick it up because th bootstrap config is rejected

[2020-05-18 07:14:46.193][1][debug][misc] [source/common/filesystem/posix/filesystem_impl.cc:139] Unable to determine canonical path for /certs/ca.crt: No such file or directory
[2020-05-18 07:14:46.194][1][warning][config] [source/common/config/filesystem_subscription_impl.cc:40] Filesystem config update rejected: Invalid path: /certs/ca.crt

[tsaarni]

I guess it could be rather complicated to have Envoy started, but remain in a special "uninitialized" state, waiting for cert/key files to appear. Having a new secret name would be easy way to "synchronize" and keep Envoy pods from starting before secret with the a new file layout is created.

However, in the kustomize PR, since we place certgen in a different base (to make it more easily decoupled), it's not so easy to change the name of the secrets (which need to be consistent across the certgen, contour and envoy kustomize bases).

I did not understand it fully, can you explain it a bit more? Aren't all manifest components expected to come from single Contour version and combined together before applying?

[jpeach]

I did not understand it fully, can you explain it a bit more? Aren't all manifest components expected to come from single Contour version and combined together before applying?

In the kustomize proposal, the Contour deployment is broken into 4 components:

• base types
• Contour
• certgen
• envoy

The problem is that the xDS secret needs to be configured for both contour and envoy components, leading to an implicit coupling. In kustomize, there is syntax to copy the value of one field into other fields, but that has to be done in the base, and can't retro-actively be applied when you consume the base. You would have to do it all with JSON patches or something.

I guess it could be rather complicated to have Envoy started, but remain in a special "uninitialized" state, waiting for cert/key files to appear.

The bootstrap can actually do this, because it should know whether the files are available.

[tsaarni]

The problem is that the xDS secret needs to be configured for both contour and envoy components, leading to an implicit coupling. In kustomize, there is syntax to copy the value of one field into other fields, but that has to be done in the base, and can't retro-actively be applied when you consume the base.

I thought that the secret would just be renamed and those new names would still remain hardcoded in contour and envoy volume mounts like they are today (even in the kustomize PR pod specs). The change in certgen code, contour & envoy kustomize components would be done at once.

But I think I'm still missing some complexity here. Why it becomes necessary to retro-actively modify / parametrize the kustomize step?

The bootstrap can actually do this, because it should know whether the files are available.

Envoy knows when files become available and in theory it could lazily initialize xDS upstream cluster at that point. The outcome would be like a chain of dependencies in pending state:

1. xDS cluster is uninitialized and pending its dependencies
2. TLS transport socket is uninitialized and pending its dependencies
3. SDS and the file watch is pending its dependencies (actual cert/key files)

I think currently the file watch is simply associated to reloading cert/key files into existing SSL context and there might not be mechanism to do lazy initialization like this.

[tsaarni]

Ah sorry, do you mean contour bootstrap would also introduce a file watcher?

[jpeach]

The problem is that the xDS secret needs to be configured for both contour and envoy components, leading to an implicit coupling. In kustomize, there is syntax to copy the value of one field into other fields, but that has to be done in the base, and can't retro-actively be applied when you consume the base.

I thought that the secret would just be renamed and those new names would still remain hardcoded in contour and envoy volume mounts like they are today (even in the kustomize PR pod specs). The change in certgen code, contour & envoy kustomize components would be done at once.

But I think I'm still missing some complexity here. Why it becomes necessary to retro-actively modify / parametrize the kustomize step?

What I'm saying is that it is hard to do the transform after the fact in kustomize. We can just change the original sources, but in the original statement I was thinking about how we would do it after the fact.

The bootstrap can actually do this, because it should know whether the files are available.

Envoy knows when files become available and in theory it could lazily initialize xDS upstream cluster at that point. The outcome would be like a chain of dependencies in pending state:

1. xDS cluster is uninitialized and pending its dependencies
2. TLS transport socket is uninitialized and pending its dependencies
3. SDS and the file watch is pending its dependencies (actual cert/key files)

I think currently the file watch is simply associated to reloading cert/key files into existing SSL context and there might not be mechanism to do lazy initialization like this.

We don't need to change envoy. I have a 5 line change to the bootstrapped that exits if the files aren't already present. There's no point continuing because envoy will reject the static config update.

[tsaarni]

Thanks for explaining :)

If we are willing to do non-backwards compatible change, would it be feasible to simply rename the secret in original sources?

[jpeach]

Thanks for explaining :)

If we are willing to do non-backwards compatible change, would it be feasible to simply rename the secret in original sources?

We don't even need to do that. I have a working change to fix everything, but it does require generating a unique name for the certgen job.