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

Use consistent naming for dnsName gRPC fields #7654

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Conversation

aarongable
Copy link
Contributor

Find all gRPC fields which represent DNS Names -- sometimes called "identifier", "hostname", "domain", "identifierValue", or other things -- and unify their naming. This naming makes it very clear that these values are strings which may be included in the SAN extension of a certificate with type dnsName.

As we move towards issuing IP Address certificates, all of these fields will need to be replaced by fields which carry both an identifier type and value, not just a single name. This unified naming makes it very clear which messages and methods need to be updated to support non-dnsName identifiers.

Part of #7647

@aarongable aarongable requested a review from a team as a code owner August 9, 2024 16:55
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

Overall I love this change. I think it also clarifies what a huge task it'll be to support multiple identifier types.

@@ -125,7 +125,7 @@ type ValidationRecord struct {
URL string `json:"url,omitempty"`

// Shared
Hostname string `json:"hostname,omitempty"`
DnsName string `json:"hostname,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to adjust the JSON field of hostname here? If so, there could be some downstream workflows that will need changes as well (e.g. log checkers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was my thinking that it's okay to leave the json name of this field as-is for now, since we're going to have to replace this field entirely in the near future anyway. No need for the downstream tools to have to go through two changes, when the main point of this change is to just get Boulder using the same language everywhere to make future changes easier.

wfe2/wfe_test.go Outdated Show resolved Hide resolved
sa/model.go Show resolved Hide resolved
@beautifulentropy beautifulentropy self-requested a review August 12, 2024 21:00
@aarongable aarongable merged commit 46859a2 into main Aug 12, 2024
12 checks passed
@aarongable aarongable deleted the rename-proto-fields branch August 12, 2024 21:32
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.

2 participants