-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
admin: Add pause-identifier and unpause-account subcommands #7668
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
063b5a1
initial commit
pgporada bb4a68a
Working, but without tests
pgporada 79c48bf
Address comments
pgporada 54a9367
Fix comment
pgporada aef149f
Address comments
pgporada b10e81c
Fix subcommand names
pgporada c717780
Update new subcommand descriptions
pgporada f8b4998
Begin adding tests and addressing comments
pgporada a023c4f
Remove static test data
pgporada 73ff9f5
Testing for reading paused csv files
pgporada 4457b15
Test for reading unpause account files
pgporada 1dc42b1
Tests
pgporada 9b1321a
Fix test error
pgporada 6944ff5
Address comments
pgporada 95f00dd
Remove unused variable
pgporada 13bcab1
Address nit
pgporada File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
package main | ||
|
||
import ( | ||
"context" | ||
"encoding/csv" | ||
"errors" | ||
"flag" | ||
"fmt" | ||
"io" | ||
"os" | ||
"strconv" | ||
|
||
"github.com/letsencrypt/boulder/identifier" | ||
sapb "github.com/letsencrypt/boulder/sa/proto" | ||
) | ||
|
||
// subcommandPauseIdentifier encapsulates the "admin pause-identifiers" command. | ||
type subcommandPauseIdentifier struct { | ||
batchFile string | ||
} | ||
|
||
var _ subcommand = (*subcommandPauseIdentifier)(nil) | ||
|
||
func (p *subcommandPauseIdentifier) Desc() string { | ||
return "Administratively pause an account preventing it from attempting certificate issuance" | ||
} | ||
|
||
func (p *subcommandPauseIdentifier) Flags(flag *flag.FlagSet) { | ||
flag.StringVar(&p.batchFile, "batch-file", "", "Path to a CSV file containing (account ID, identifier type, identifier value)") | ||
} | ||
|
||
func (p *subcommandPauseIdentifier) Run(ctx context.Context, a *admin) error { | ||
if p.batchFile == "" { | ||
return errors.New("the -batch-file flag is required") | ||
} | ||
|
||
identifiers, err := a.readPausedAccountFile(p.batchFile) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
_, err = a.pauseIdentifiers(ctx, identifiers) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// pauseIdentifiers allows administratively pausing a set of domain names for an | ||
// account. It returns a slice of PauseIdentifiersResponse or an error. | ||
func (a *admin) pauseIdentifiers(ctx context.Context, incoming []pauseCSVData) ([]*sapb.PauseIdentifiersResponse, error) { | ||
if len(incoming) <= 0 { | ||
return nil, errors.New("cannot pause identifiers because no pauseData was sent") | ||
} | ||
|
||
var responses []*sapb.PauseIdentifiersResponse | ||
for _, data := range incoming { | ||
req := sapb.PauseRequest{ | ||
RegistrationID: data.accountID, | ||
Identifiers: []*sapb.Identifier{ | ||
{ | ||
Type: string(data.identifierType), | ||
Value: data.identifierValue, | ||
}, | ||
}, | ||
} | ||
response, err := a.sac.PauseIdentifiers(ctx, &req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
responses = append(responses, response) | ||
} | ||
|
||
return responses, nil | ||
} | ||
|
||
// pauseCSVData contains a golang representation of the data loaded in from a | ||
// CSV file for pausing. | ||
type pauseCSVData struct { | ||
accountID int64 | ||
identifierType identifier.IdentifierType | ||
identifierValue string | ||
} | ||
|
||
// readPausedAccountFile parses the contents of a CSV into a slice of | ||
// `pauseCSVData` objects and returns it or an error. It will skip malformed | ||
// lines and continue processing until either the end of file marker is detected | ||
// or other read error. | ||
func (a *admin) readPausedAccountFile(filePath string) ([]pauseCSVData, error) { | ||
fp, err := os.Open(filePath) | ||
if err != nil { | ||
return nil, fmt.Errorf("opening paused account data file: %w", err) | ||
} | ||
defer fp.Close() | ||
|
||
reader := csv.NewReader(fp) | ||
|
||
// identifierValue can have 1 or more entries | ||
reader.FieldsPerRecord = -1 | ||
reader.TrimLeadingSpace = true | ||
|
||
var parsedRecords []pauseCSVData | ||
lineCounter := 0 | ||
|
||
// Process contents of the CSV file | ||
for { | ||
record, err := reader.Read() | ||
if errors.Is(err, io.EOF) { | ||
break | ||
} else if err != nil { | ||
return nil, err | ||
} | ||
|
||
lineCounter++ | ||
|
||
// We should have strictly 3 fields, note that just commas is considered | ||
// a valid CSV line. | ||
if len(record) != 3 { | ||
a.log.Infof("skipping: malformed line %d, should contain exactly 3 fields\n", lineCounter) | ||
continue | ||
} | ||
|
||
recordID := record[0] | ||
accountID, err := strconv.ParseInt(recordID, 10, 64) | ||
if err != nil || accountID == 0 { | ||
a.log.Infof("skipping: malformed accountID entry on line %d\n", lineCounter) | ||
continue | ||
} | ||
|
||
// Ensure that an identifier type is present, otherwise skip the line. | ||
if len(record[1]) == 0 { | ||
a.log.Infof("skipping: malformed identifierType entry on line %d\n", lineCounter) | ||
continue | ||
} | ||
|
||
if len(record[2]) == 0 { | ||
a.log.Infof("skipping: malformed identifierValue entry on line %d\n", lineCounter) | ||
continue | ||
} | ||
|
||
parsedRecord := pauseCSVData{ | ||
accountID: accountID, | ||
identifierType: identifier.IdentifierType(record[1]), | ||
identifierValue: record[2], | ||
} | ||
parsedRecords = append(parsedRecords, parsedRecord) | ||
} | ||
a.log.Infof("detected %d valid record(s) from input file\n", len(parsedRecords)) | ||
|
||
return parsedRecords, nil | ||
} |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
package main | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"os" | ||
"path" | ||
"strings" | ||
"testing" | ||
|
||
blog "github.com/letsencrypt/boulder/log" | ||
sapb "github.com/letsencrypt/boulder/sa/proto" | ||
"github.com/letsencrypt/boulder/test" | ||
"google.golang.org/grpc" | ||
) | ||
|
||
func TestReadingPauseCSV(t *testing.T) { | ||
t.Parallel() | ||
|
||
testCases := []struct { | ||
name string | ||
data []string | ||
expectedRecords int | ||
}{ | ||
{ | ||
name: "No data in file", | ||
data: nil, | ||
}, | ||
{ | ||
name: "valid", | ||
data: []string{"1,dns,example.com"}, | ||
expectedRecords: 1, | ||
}, | ||
{ | ||
name: "valid with duplicates", | ||
data: []string{"1,dns,example.com", "2,dns,example.org", "1,dns,example.com", "1,dns,example.net", "3,dns,example.gov", "3,dns,example.gov"}, | ||
expectedRecords: 6, | ||
}, | ||
{ | ||
name: "invalid with multiple domains on the same line", | ||
data: []string{"1,dns,example.com,example.net"}, | ||
}, | ||
{ | ||
name: "invalid just commas", | ||
data: []string{",,,"}, | ||
}, | ||
{ | ||
name: "invalid only contains accountID", | ||
data: []string{"1"}, | ||
}, | ||
{ | ||
name: "invalid only contains accountID and identifierType", | ||
data: []string{"1,dns"}, | ||
}, | ||
{ | ||
name: "invalid missing identifierType", | ||
data: []string{"1,,example.com"}, | ||
}, | ||
{ | ||
name: "invalid accountID isnt an int", | ||
data: []string{"blorple"}, | ||
}, | ||
} | ||
|
||
for _, testCase := range testCases { | ||
t.Run(testCase.name, func(t *testing.T) { | ||
t.Parallel() | ||
log := blog.NewMock() | ||
a := admin{log: log} | ||
|
||
csvFile := path.Join(t.TempDir(), path.Base(t.Name()+".csv")) | ||
err := os.WriteFile(csvFile, []byte(strings.Join(testCase.data, "\n")), os.ModePerm) | ||
test.AssertNotError(t, err, "could not write temporary file") | ||
|
||
parsedData, err := a.readPausedAccountFile(csvFile) | ||
test.AssertNotError(t, err, "no error expected, but received one") | ||
test.AssertEquals(t, len(parsedData), testCase.expectedRecords) | ||
}) | ||
} | ||
} | ||
|
||
// mockSAPaused is a mock which always succeeds. It records the PauseRequest it | ||
// received, and returns the number of identifiers as a | ||
// PauseIdentifiersResponse. It does not maintain state of repaused identifiers. | ||
type mockSAPaused struct { | ||
sapb.StorageAuthorityClient | ||
reqs []*sapb.PauseRequest | ||
} | ||
|
||
func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, in *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { | ||
msa.reqs = append(msa.reqs, in) | ||
|
||
return &sapb.PauseIdentifiersResponse{Paused: int64(len(in.Identifiers))}, nil | ||
} | ||
|
||
// mockSAPausedBroken is a mock which always errors. | ||
type mockSAPausedBroken struct { | ||
sapb.StorageAuthorityClient | ||
} | ||
|
||
func (msa *mockSAPausedBroken) PauseIdentifiers(ctx context.Context, in *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { | ||
return nil, errors.New("its all jacked up") | ||
} | ||
|
||
func TestPauseIdentifiers(t *testing.T) { | ||
t.Parallel() | ||
|
||
testCases := []struct { | ||
name string | ||
data []pauseCSVData | ||
saImpl sapb.StorageAuthorityClient | ||
expectErr bool | ||
}{ | ||
{ | ||
name: "no data", | ||
data: nil, | ||
expectErr: true, | ||
}, | ||
{ | ||
name: "valid single entry", | ||
data: []pauseCSVData{ | ||
{ | ||
accountID: 1, | ||
identifierType: "dns", | ||
identifierValue: "example.com", | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "valid single entry but broken SA", | ||
expectErr: true, | ||
saImpl: &mockSAPausedBroken{}, | ||
data: []pauseCSVData{ | ||
{ | ||
accountID: 1, | ||
identifierType: "dns", | ||
identifierValue: "example.com", | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "valid multiple entries with duplicates", | ||
data: []pauseCSVData{ | ||
{ | ||
accountID: 1, | ||
identifierType: "dns", | ||
identifierValue: "example.com", | ||
}, | ||
{ | ||
accountID: 1, | ||
identifierType: "dns", | ||
identifierValue: "example.com", | ||
}, | ||
{ | ||
accountID: 2, | ||
identifierType: "dns", | ||
identifierValue: "example.org", | ||
}, | ||
{ | ||
accountID: 3, | ||
identifierType: "dns", | ||
identifierValue: "example.net", | ||
}, | ||
{ | ||
accountID: 3, | ||
identifierType: "dns", | ||
identifierValue: "example.org", | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, testCase := range testCases { | ||
t.Run(testCase.name, func(t *testing.T) { | ||
t.Parallel() | ||
|
||
log := blog.NewMock() | ||
|
||
// Default to a working mock SA implementation | ||
if testCase.saImpl == nil { | ||
testCase.saImpl = &mockSAPaused{} | ||
} | ||
a := admin{sac: testCase.saImpl, log: log} | ||
|
||
responses, err := a.pauseIdentifiers(context.Background(), testCase.data) | ||
if testCase.expectErr { | ||
test.AssertError(t, err, "should have errored, but did not") | ||
} else { | ||
test.AssertNotError(t, err, "should not have errored") | ||
test.AssertEquals(t, len(responses), len(testCase.data)) | ||
} | ||
}) | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the PauseIdentifiers gRPC method can take many identifiers as input. Maybe collect rows from the CSV into batches of 100, and then send those in a single request?
Even better, make the batch size controllable with a command line flag that defaults to 100 or so, sort of like the
-parallelism
flag on other admin subcommands.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to skip this for now. This sounds like a good future improvement.