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

RA/CA: Make MaxNames field consistent and supply default #7256

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Jan 11, 2024

Update RA and CA configuration to be more consistent with the identical MaxNames field added to the WFE by #7201.

@beautifulentropy beautifulentropy requested a review from a team as a code owner January 11, 2024 18:53
aarongable
aarongable previously approved these changes Jan 12, 2024
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Is there a particular reason we're interested in these having a default value? We already supply this config field everywhere, so the added flexibility here isn't buying us much. Especially since we're considering lowering the maxNames (at least on a per-profile basis), having a default may make that transition more confusing.

But if we do want this, then LGTM, the code does what it says on the tin

@beautifulentropy
Copy link
Member Author

Is there a particular reason we're interested in these having a default value? We already supply this config field everywhere, so the added flexibility here isn't buying us much. Especially since we're considering lowering the maxNames (at least on a per-profile basis), having a default may make that transition more confusing.

But if we do want this, then LGTM, the code does what it says on the tin

Honestly, I added the identical field to the WFE and realized that it would be nice if the implementation and documentation were consistent across all three components. If you think we should strip the defaulting logic, I'm open to it.

@aarongable
Copy link
Contributor

Yeah, having the default in the WFE makes sense because it makes for easy deployability. But I don't think its necessary to do the same here, especially since I'm likely to be changing how MaxNames works (moving it inside issuance.ProfileConfig) anyway so the extra scaffolding will just make that more complex in a week or two.

jsha
jsha previously approved these changes Jan 16, 2024
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Like Aaron, I think this is fine, though maybe not needed.

@beautifulentropy beautifulentropy dismissed stale reviews from jsha and aarongable via 6658079 January 16, 2024 21:58
@beautifulentropy
Copy link
Member Author

Like Aaron, I think this is fine, though maybe not needed.

Alright, thanks for the comments @jsha and @aarongable. I've reverted the defaulting.

cmd/boulder-ca/main.go Outdated Show resolved Hide resolved
@beautifulentropy beautifulentropy merged commit 551c10f into main Jan 17, 2024
18 checks passed
@beautifulentropy beautifulentropy deleted the make-maxnames-field-consistent branch January 17, 2024 19:48
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