1Thanks for helping us build Boulder! This page contains requirements and
2guidelines for Boulder contributions.
3
4# Patch Requirements
5
6* All new functionality and fixed bugs must be accompanied by tests.
7* All patches must meet the deployability requirements listed below.
8* We prefer pull requests from external forks be created with the ["Allow edits
9 from
10 maintainers"](https://github.com/blog/2247-improving-collaboration-with-forks)
11 checkbox selected.
12
13# Review Requirements
14
15* All pull requests must receive at least one approval through the GitHub UI.
16* We indicate review approval through GitHub's code review facility.
17* New commits pushed to a branch invalidate previous reviews. In other words, a
18 reviewer must give positive reviews of a branch after its most recent pushed
19 commit.
20* You cannot review your own code.
21* If a branch contains commits from multiple authors, it needs a reviewer who
22 is not an author of commits on that branch.
23* Review changes to or addition of tests just as rigorously as you review code
24 changes. Consider: Do tests actually test what they mean to test? Is this the
25 best way to test the functionality in question? Do the tests cover all the
26 functionality in the patch, including error cases?
27* Are there new RPCs or config fields? Make sure the patch meets the
28 Deployability rules below.
29
30# Patch Guidelines
31
32* Please include helpful comments. No need to gratuitously comment clear code,
33 but make sure it's clear why things are being done. Include information in
34 your pull request about what you're trying to accomplish with your patch.
35* Avoid named return values. See
36 [#3017](https://github.com/letsencrypt/boulder/pull/3017) for an example of a
37 subtle problem they can cause.
38* Do not include `XXX`s or naked `TODO`s. Use
39 the formats:
40
41 ```go
42 // TODO(<email-address>): Hoverboard + Time-machine unsupported until upstream patch.
43 // TODO(#<num>): Pending hoverboard/time-machine interface.
44 // TODO(@githubusername): Enable hoverboard kickflips once interface is stable.
45 ```
46
47# Squash merging
48
49Once a pull request is approved and the tests are passing, the author or any
50other committer can merge it. We always use [squash
51merges](https://github.com/blog/2141-squash-your-commits) via GitHub's web
52interface. That means that during the course of your review you should
53generally not squash or amend commits, or force push. Even if the changes in
54each commit are small, keeping them separate makes it easier for us to review
55incremental changes to a pull request. Rest assured that those tiny changes
56will get squashed into a nice meaningful-size commit when we merge.
57
58If the CI tests are failing on your branch, you should look at the logs
59to figure out why. Sometimes (though rarely) they fail spuriously, in which
60case you can post a comment requesting that a project owner kick the build.
61
62# Error handling
63
64All errors must be addressed in some way: That may be simply by returning an
65error up the stack, or by handling it in some intelligent way where it is
66generated, or by explicitly ignoring it and assigning to `_`. We use the
67`errcheck` tool in our integration tests to make sure all errors are
68addressed. Note that ignoring errors, even in tests, should be rare, since
69they may generate hard-to-debug problems.
70
71When handling errors, always do the operation which creates the error (usually
72a function call) and the error checking on separate lines:
73```
74err := someOperation(args)
75if err != nil {
76 return nil, fmt.Errorf("some operation failed: %w", err)
77}
78```
79We avoid the `if err := someOperation(args); err != nil {...}` style as we find
80it to be less readable and it can give rise to surprising scoping behavior.
81
82We define two special types of error. `BoulderError`, defined in
83errors/errors.go, is used specifically when an typed error needs to be passed
84across an RPC boundary. For instance, if the SA returns "not found", callers
85need to be able to distinguish that from a network error. Not every error that
86may pass across an RPC boundary needs to be a BoulderError, only those errors
87that need to be handled by type elsewhere. Handling by type may be as simple as
88turning a BoulderError into a specific type of ProblemDetail.
89
90The other special type of error is `ProblemDetails`. We try to treat these as a
91presentation-layer detail, and use them only in parts of the system that are
92responsible for rendering errors to end-users, i.e. WFE2. Note
93one exception: The VA RPC layer defines its own `ProblemDetails` type, which is
94returned to the RA and stored as part of a challenge (to eventually be rendered
95to the user).
96
97Within WFE2, ProblemDetails are sent to the client by calling
98`sendError()`, which also logs the error. For internal errors like timeout,
99or any error type that we haven't specifically turned into a ProblemDetail, we
100return a ServerInternal error. This avoids unnecessarily exposing internals.
101It's possible to add additional errors to a logEvent using `.AddError()`, but
102this should only be done when there is is internal-only information to log
103that isn't redundant with the ProblemDetails sent to the user. Note that the
104final argument to `sendError()`, `ierr`, will automatically get added to the
105logEvent for ServerInternal errors, so when sending a ServerInternal error it's
106not necessary to separately call `.AddError`.
107
108# Deployability
109
110We want to ensure that a new Boulder revision can be deployed to the
111currently running Boulder production instance without requiring config
112changes first. We also want to ensure that during a deploy, services can be
113restarted in any order. That means two things:
114
115## Good zero values for config fields
116
117Any newly added config field must have a usable [zero
118value](https://tour.golang.org/basics/12). That is to say, if a config field
119is absent, Boulder shouldn't crash or misbehave. If that config file names a
120file to be read, Boulder should be able to proceed without that file being
121read.
122
123Note that there are some config fields that we want to be a hard requirement.
124To handle such a field, first add it as optional, then file an issue to make
125it required after the next deploy is complete.
126
127In general, we would like our deploy process to be: deploy new code + old
128config; then immediately after deploy the same code + new config. This makes
129deploys cheaper so we can do them more often, and allows us to more readily
130separate deploy-triggered problems from config-triggered problems.
131
132## Flag-gating features
133
134When adding significant new features or replacing existing RPCs the
135`boulder/features` package should be used to gate its usage. To add a flag, a
136new `const FeatureFlag` should be added and its default value specified in
137`features.features` in `features/features.go`. In order to test if the flag
138is enabled elsewhere in the codebase you can use
139`features.Enabled(features.ExampleFeatureName)` which returns a `bool`
140indicating if the flag is enabled or not.
141
142Each service should include a `map[string]bool` named `Features` in its
143configuration object at the top level and call `features.Set` with that map
144immediately after parsing the configuration. For example to enable
145`UseNewMetrics` and disable `AccountRevocation` you would add this object:
146
147```json
148{
149 ...
150 "features": {
151 "UseNewMetrics": true,
152 "AccountRevocation": false,
153 }
154}
155```
156
157Avoid negative flag names such as `"DontCancelRequest": false` because such
158names are difficult to reason about.
159
160Feature flags are meant to be used temporarily and should not be used for
161permanent boolean configuration options. Once a feature has been enabled in
162both staging and production the flag should be removed making the previously
163gated functionality the default in future deployments.
164
165### Gating RPCs
166
167When you add a new RPC to a Boulder service (e.g. `SA.GetFoo()`), all
168components that call that RPC should gate those calls using a feature flag.
169Since the feature's zero value is false, a deploy with the existing config
170will not call `SA.GetFoo()`. Then, once the deploy is complete and we know
171that all SA instances support the `GetFoo()` RPC, we do a followup config
172deploy that sets the default value to true, and finally remove the flag
173entirely once we are confident the functionality it gates behaves correctly.
174
175### Gating migrations
176
177We use [database migrations](https://en.wikipedia.org/wiki/Schema_migration)
178to modify the existing schema. These migrations will be run on live data
179while Boulder is still running, so we need Boulder code at any given commit
180to be capable of running without depending on any changes in schemas that
181have not yet been applied.
182
183For instance, if we're adding a new column to an existing table, Boulder should
184run correctly in three states:
185
1861. Migration not yet applied.
1872. Migration applied, flag not yet flipped.
1883. Migration applied, flag flipped.
189
190Specifically, that means that all of our `SELECT` statements should enumerate
191columns to select, and not use `*`. Also, generally speaking, we will need a
192separate model `struct` for serializing and deserializing data before and
193after the migration. This is because the ORM package we use,
194[`borp`](https://github.com/letsencrypt/borp), expects every field in a struct to
195map to a column in the table. If we add a new field to a model struct and
196Boulder attempts to write that struct to a table that doesn't yet have the
197corresponding column (case 1), borp will fail with `Insert failed table posts
198has no column named Foo`. There are examples of such models in sa/model.go,
199along with code to turn a model into a `struct` used internally.
200
201An example of a flag-gated migration, adding a new `IsWizard` field to Person
202controlled by a `AllowWizards` feature flag:
203
204```go
205# features/features.go:
206
207const (
208 unused FeatureFlag = iota // unused is used for testing
209 AllowWizards // Added!
210)
211
212...
213
214var features = map[FeatureFlag]bool{
215 unused: false,
216 AllowWizards: false, // Added!
217}
218```
219
220```go
221# sa/sa.go:
222
223struct Person {
224 HatSize int
225 IsWizard bool // Added!
226}
227
228struct personModelv1 {
229 HatSize int
230}
231
232// Added!
233struct personModelv2 {
234 personModelv1
235 IsWizard bool
236}
237
238func (ssa *SQLStorageAuthority) GetPerson() (Person, error) {
239 if features.Enabled(features.AllowWizards) { // Added!
240 var model personModelv2
241 ssa.dbMap.SelectOne(&model, "SELECT hatSize, isWizard FROM people")
242 return Person{
243 HatSize: model.HatSize,
244 IsWizard: model.IsWizard,
245 }
246 } else {
247 var model personModelv1
248 ssa.dbMap.SelectOne(&model, "SELECT hatSize FROM people")
249 return Person{
250 HatSize: model.HatSize,
251 }
252 }
253}
254
255func (ssa *SQLStorageAuthority) AddPerson(p Person) (error) {
256 if features.Enabled(features.AllowWizards) { // Added!
257 return ssa.dbMap.Insert(context.Background(), personModelv2{
258 personModelv1: {
259 HatSize: p.HatSize,
260 },
261 IsWizard: p.IsWizard,
262 })
263 } else {
264 return ssa.dbMap.Insert(context.Background(), personModelv1{
265 HatSize: p.HatSize,
266 // p.IsWizard ignored
267 })
268 }
269}
270```
271
272You will also need to update the `initTables` function from `sa/database.go` to
273tell borp which table to use for your versioned model structs. Make sure to
274consult the flag you defined so that only **one** of the table maps is added at
275any given time, otherwise borp will error. Depending on your table you may also
276need to add `SetKeys` and `SetVersionCol` entries for your versioned models.
277Example:
278
279```go
280func initTables(dbMap *borp.DbMap) {
281 // < unrelated lines snipped for brevity >
282
283 if features.Enabled(features.AllowWizards) {
284 dbMap.AddTableWithName(personModelv2, "person")
285 } else {
286 dbMap.AddTableWithName(personModelv1, "person")
287 }
288}
289```
290
291New migrations should be added at `./sa/db-next`:
292
293```shell
294$ cd sa/db
295$ sql-migrate new -env="boulder_sa_test" AddWizards
296Created migration boulder_sa/20220906165519-AddWizards.sql
297```
298
299Finally, edit the resulting file
300(`sa/db-next/boulder_sa/20220906165519-AddWizards.sql`) to define your migration:
301
302```mysql
303-- +migrate Up
304ALTER TABLE people ADD isWizard BOOLEAN SET DEFAULT false;
305
306-- +migrate Down
307ALTER TABLE people DROP isWizard BOOLEAN SET DEFAULT false;
308```
309
310# Release Process
311
312The current Boulder release process is described in
313[release.md](https://github.com/letsencrypt/boulder/docs/release.md). New
314releases are tagged weekly, and artifacts are automatically produced for each
315release by GitHub Actions.
316
317# Dependencies
318
319We use [go modules](https://github.com/golang/go/wiki/Modules) and vendor our
320dependencies. As of Go 1.12, this may require setting the `GO111MODULE=on` and
321`GOFLAGS=-mod=vendor` environment variables. Inside the Docker containers for
322Boulder tests, these variables are set for you, but if you ever work outside
323those containers you will want to set them yourself.
324
325To add a dependency, add the import statement to your .go file, then run
326`go build` on it. This will automatically add the dependency to go.mod. Next,
327run `go mod vendor && git add vendor/` to save a copy in the vendor folder.
328
329When vendorizing dependencies, it's important to make sure tests pass on the
330version you are vendorizing. Currently we enforce this by requiring that pull
331requests containing a dependency update to any version other than a tagged
332release include a comment indicating that you ran the tests and that they
333succeeded, preferably with the command line you run them with. Note that you
334may have to get a separate checkout of the dependency (using `go get` outside
335of the boulder repository) in order to run its tests, as some vendored
336modules do not bring their tests with them.
337
338## Updating Dependencies
339
340To upgrade a dependency, [see the Go
341docs](https://github.com/golang/go/wiki/Modules#how-to-upgrade-and-downgrade-dependencies).
342Typically you want `go get <dependency>` rather than `go get -u
343<dependency>`, which can introduce a lot of unexpected updates. After running
344`go get`, make sure to run `go mod vendor && git add vendor/` to update the
345vendor directory. If you forget, CI tests will catch this.
346
347If you are updating a dependency to a version which is not a tagged release,
348see the note above about how to run all of a dependency's tests and note that
349you have done so in the PR.
350
351Note that updating dependencies can introduce new, transitive dependencies. In
352general we try to keep our dependencies as narrow as possible in order to
353minimize the number of people and organizations whose code we need to trust.
354As a rule of thumb: If an update introduces new packages or modules that are
355inside a repository where we already depend on other packages or modules, it's
356not a big deal. If it introduces a new dependency in a different repository,
357please try to figure out where that dependency came from and why (for instance:
358"package X, which we depend on, started supporting XML config files, so now we
359depend on an XML parser") and include that in the PR description. When there are
360a large number of new dependencies introduced, and we don't need the
361functionality they provide, we should consider asking the relevant upstream
362repository for a refactoring to reduce the number of transitive dependencies.
363
364# Go Version
365
366The [Boulder development
367environment](https://github.com/letsencrypt/boulder/blob/main/README.md#setting-up-boulder)
368does not use the Go version installed on the host machine, and instead uses a
369Go environment baked into a "boulder-tools" Docker image. We build a separate
370boulder-tools container for each supported Go version. Please see [the
371Boulder-tools
372README](https://github.com/letsencrypt/boulder/blob/main/test/boulder-tools/README.md)
373for more information on upgrading Go versions.
374
375# ACME Protocol Divergences
376
377While Boulder attempts to implement the ACME specification as strictly as
378possible there are places at which we will diverge from the letter of the
379specification for various reasons. We detail these divergences (for both the
380V1 and V2 API) in the [ACME divergences
381doc](https://github.com/letsencrypt/boulder/blob/main/docs/acme-divergences.md).
382
383# ACME Protocol Implementation Details
384
385The ACME specification allows developers to make certain decisions as to how
386various elements in the RFC are implemented. Some of these fully conformant
387decisions are listed in [ACME implementation details
388doc](https://github.com/letsencrypt/boulder/blob/main/docs/acme-implementation_details.md).
389
390## Code of Conduct
391
392The code of conduct for everyone participating in this community in any capacity
393is available for reference
394[on the community forum](https://community.letsencrypt.org/guidelines).
395
396## Problems or questions?
397
398The best place to ask dev related questions is on the [Community
399Forums](https://community.letsencrypt.org/).
View as plain text