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.
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
crl provider: Static and FileWatcher provider implementations #6670
crl provider: Static and FileWatcher provider implementations #6670
Changes from 21 commits
978cb44
7d032e0
cdbc298
95991d8
32e3158
00de36e
8033cab
338a7f4
d1f63fe
01afa97
401eb79
c88d12d
1feaae3
a9a84f1
5a0acad
f3c830b
4ea1b34
aeebd4e
735ac20
5c76a60
0bc7757
f844c8c
a4da85e
c3ba07e
ffe5c34
6d28181
7814373
1a46b65
d7f1555
2f1935d
0a7b086
8d05f28
ccbf7f6
99ecab0
9e5a70d
5643760
8898959
b16af8b
21f4301
51b42aa
f0c1ca4
08188d1
9b8d07e
ad15e23
8e02546
e6a690d
340757d
1e4c5ac
f654d18
53d6b05
1f398eb
f3dcca1
131e6e7
bc14ea8
96bf905
0ce6a2c
c57a08a
4c53c56
7fedab5
1025333
d9ba363
150e585
d7cf48f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 see you changed this test case and it's not a new one - just curious as to why you changed it?
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.
That's a good catch, I was experimenting with some chains and accidentally submitted (it doesn't influence the outputs). Will revert it back
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.
Done
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.
Can we name the certs for what they are rather than just being numbered?
e.g. if
ClientCert3
is a cert that is revoked, just name isClientRevokedCert
or something like that?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'd prefer to leave it as is for this PR since it fits the current pattern -
security/advancedtls/internal/testutils/testutils.go
Definitely something to address during 'Unifying API' effort (will be a separate PR)
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 don't think we should continue to add bad code that we plan to refactor later just to keep with a bad existing pattern - let's go ahead and name these variables with something meaningful rather than making more work for ourselves down the road.
In fact, I'd argue the current pattern isn't much of a pattern - it's just lazy variable naming. For there to be a pattern there would need to be some meaning to
Cert1
,Cert2
, etcThere 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.
Same comment about variable naming
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'd prefer to leave it as is for this PR since it fits the current pattern - security/advancedtls/internal/testutils/testutils.go
Definitely something to address during 'Unifying API' effort (will be a separate PR)
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.
See other thread
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.
nit:
non
->none
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.
You fixed the wrong
non
:)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.
Done
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.
Consider adding
serverExpectError: false
here to make the test case definition easier to parse at a glance.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.
It fits the current pattern, if
serverExpectError
is missing fromtest struct
it'sfalse
.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.
Please wrap all comments at 80 columns.
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.
Done
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.
"requires certain specifics" is too vague. Or are you saying people should construct them using those functions? If the latter, just omit the first half.
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.
Done
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.
This type is newly exported. The fields were already named as though they were exported. Are they supposed to be read (or set) by the user directly? If not, they should be unexported.
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.
After an offline discussion with @gtcooke94 we made this info private for both C++ and Go
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.
Done - unexported
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.
"...from the provided byte array" please
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.
Done
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.
suggest something like "ReadCRLFile reads a crl file from the provided path and returns a constructed CRL struct."
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.
Can you push your branch if you've addressed these? Please reply to the comments you've addressed as well so I can tell what is done vs. what was forgotten.
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.
Done
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.
os.ReadFile
?Also I don't think this is the right format for errors in Go. This is more the style for test failures, as it appears to be suited for programmers and not users. Errors would usually look more like:
(or
%w
instead of%v
if you want users to be able to unwrap the read error, and then this becomes part of your API.)You can see examples from the stdlib:
https://github.com/search?q=repo%3Agolang%2Fgo%20fmt.Errorf&type=code
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.
Thank you! I refactored the error messages, but didn't get your comment
Is that a wrong function to use here?
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.
That original line was about what you were printing.
readFile()
wasn't the thing that failed (and it doesn't exist); it wasos.ReadFile
that failed. So it would be better to use the proper name of the thing that failed and not a function that doesn't exist. But it's even better to not use debugging-style messages as errors, so then I suggested that.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.
Oh I see, sorry - GitHub always highlights few lines so I thought there might be a better than
os.ReadFile
function to use here.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.
This sounds like a cross-language thing. Has it been discussed?
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.
Yeah we had an offline discussion with @gtcooke94 and decided not to print any for both C++ and Go
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.
Done
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.
Please revert/re-delete these.
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.
Done
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 think we can just delete this file now, right?
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.
IIUC we need support 3 latest releases of Go and it's 1.21, 1.20, and 1.19 (https://go.dev/dl/) - so yes, it's safe to delete it (1.19+ is a prereq for deletion)
Do you prefer me to do it here or via a separate PR?
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 don't mind either way, but I don't think we should make any of the changes in this PR, since the file should be deleted instead.
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.
Ok I just created a separate PR and added you as a reviewer - #6721