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

maintain: remove some unused server options #2174

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Jun 2, 2022

This PR removes some unused loadCertificate logic and options from the server. We may restore this in the future when it is being used. For now I think it's better to remove it so that contributors are not distracted by it. Removing it also avoids doing unnecessary work on server startup.

This PR also moves one test file into the pki package. It was only testing things in that package. The pki package is also not used, but at least it's well contained and is not called by any production code.

These certs are not used. Removing them helps keep the code base easier to understand
and reduces the work done on server startup.
@BruceMacD
Copy link
Collaborator

Any way we can capture this commit in an issue for loading certificates in Infra? It seems like an option we will want soon and I'd like an easy way to find this existing work.

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

It wasn't too long ago that I resolved the issue that asked for the ability to load user-specified certs. Seems like a step backwards. edit: misunderstood the issue

@dnephin
Copy link
Contributor Author

dnephin commented Jun 2, 2022

Good call to create an issue about this. I was thinking of adding support for that next. I've created #2176 to track it.

I'm not really sure how this code being removed relates to users being able to specify their own cert and key. Supporting that should be really simple, just creating a tls.Config out of the config they provided. As far as I can tell, the code being removed does not actually allow a user to specify their own certificate. The certs are never referenced by the tls.Config we pass to the HTTP server.

I suspect we won't use any of what is being removed here to support #2176.

@ssoroka
Copy link
Contributor

ssoroka commented Jun 2, 2022

Ah, sorry, this is the certificate provider stuff. This is related to the plan that we will manage certificate rotation for the user. Can't say I'm excited to see it go away.

@dnephin
Copy link
Contributor Author

dnephin commented Jun 2, 2022

Git makes it easy to restore it later. Is there an issue related to that feature? I can put a note into that issue to link back to this PR, so we can find it easily.

@ssoroka
Copy link
Contributor

ssoroka commented Jun 2, 2022

Relevant issues is #593

@ssoroka
Copy link
Contributor

ssoroka commented Jun 2, 2022

to explain more, this is part of the server-managed certificates, which is work-in-progress, and I think we still want to build and use it. Jeff put this on hold a couple months ago, so I'm not sure what the status is, but I don't think the code should be considered dead unless that feature is killed.

@infrahq infrahq deleted a comment from CLAassistant Jun 3, 2022
@CLAassistant
Copy link

CLAassistant commented Jun 3, 2022

CLA assistant check
All committers have signed the CLA.

@dnephin dnephin mentioned this pull request Jun 3, 2022
5 tasks
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

discussed concerns. good to go.

@dnephin
Copy link
Contributor Author

dnephin commented Jun 3, 2022

Thank you!

@dnephin dnephin merged commit cf12d22 into main Jun 3, 2022
@dnephin dnephin deleted the dnephin/remove-unused-tls branch June 3, 2022 20:00
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.

4 participants