nginx: separate cert paths from server_name#814
Merged
alxndrsn merged 1 commit intogetodk:nextfrom Dec 7, 2024
Merged
Conversation
Working on getodk#809 I noticed that the location of SSL certs is based either on the domain name, or on the method of supply of SSL certs. Cert provision approach should probably not affect the nginx "server_name" setting. Also, the old variable name `CNAME` (short for "certificate name?") is easily confused with the DNS concept of CNAME records ("canonical names").
2 tasks
alxndrsn
pushed a commit
to alxndrsn/odk-central
that referenced
this pull request
Dec 3, 2024
brontolosone
approved these changes
Dec 6, 2024
Contributor
brontolosone
left a comment
There was a problem hiding this comment.
Makes sense!
I, for one, was confused by seeing the term CNAME.
Contributor
Author
Contributor
|
at the time, I cursorily concluded "they probably call it cname as it's maybe DNS CNAMEs with which they do vanity domains for cloud ODK or somesuch so at some point it made sense to call this variable cname as it's where the cname record's label happened to go" 😆 |
yanokwa
approved these changes
Dec 6, 2024
Member
yanokwa
left a comment
There was a problem hiding this comment.
I don't know why I called it CNAME. The change looks good to me.
Contributor
Author
You didn't introduce the original |
yanokwa
pushed a commit
to yanokwa/odk-central
that referenced
this pull request
Jun 11, 2025
Working on getodk#809 I noticed that the location of SSL certs is based either on the domain name, or on the method of supply of SSL certs. Cert provision approach should probably not affect the nginx "server_name" setting. Also, the old variable name `CNAME` (short for "certificate name?") is easily confused with the DNS concept of CNAME records ("canonical names").
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working on #809 I noticed that the location of SSL certs is based either on the domain name, or on the method of supply of SSL certs.
Cert provision approach should probably not affect the nginx "server_name" setting.
Also, the old variable name
CNAME(short for "certificate name?") is easily confused with the DNS concept of CNAME records ("canonical names") (https://en.wikipedia.org/wiki/CNAME_record).What has been done to verify that this works as intended?
Ran tests.
Why is this the best possible solution? Were any other approaches considered?
It may not be - there may be a subtle reason for the current use and/or naming of
CNAME.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Behaviour that may have accidentally been affected:
Servers with
SSL_TYPE="customssl"may have their nginxserver_namechanged. This is probably a positive change - currently it has no effect, but will do once #809 is merged.Does this change require updates to documentation? If so, please file an issue here and include the link below.
It should not.
Before submitting this PR, please make sure you have:
nextbranch OR only changed documentation/infrastructure (masteris stable and used in production)