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
✨ Certificate support for image registry #960
✨ Certificate support for image registry #960
Changes from 1 commit
b128ce3
0c20937
6155df5
0693c6a
cf5b281
421d281
3ca71ba
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.
Seems like it would be a nicer abstraction if this was:
In which case, we could add each file's content, collect errors reading/adding each file, and then tell callers which specific files failed for which reasons.
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 is not compatible with the current rukpak API, which expects a string of PEM. Maybe when rukpak is fully integrated into operator-controller? I agree that this would be better for a larger directory. There does not appear to be a way to extract certificates/PEM from a CertPool, so it can't be done with this signature, yet.
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 was just looking at
LoadCerts
being called inBuildHTTPClient
, which turns around and consumes thecerts
string by immediately adding to thecaCertPool
.And that made me think we could pass the
caCertPool
into this function and callAppendCertsFromPEM
for each file's contents. Then, when my suggested function signature returns, you end up withcaCertPool
having everything incaDir
added to it.Seems like I'm missing something though?
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's also used when creating a BundleDeployment for a rukpak API.
operator-controller/internal/controllers/clusterextension_controller.go
Line 252 in ed4e40e
So, we need to change the rukpak API to accept a caPool first, then we can do as you suggest.
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.
Is the intention to recurse through all subdirectories? I'm tempted to suggest that we stick to finding files in a single named directory without recursing, but I'm curious if you have a specific reason.
To avoid recursing, we can return
filepath.SkipDir
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.
No, it's intended to skip directories. I didn't see this in any examples.
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 appears to skip
"."
as a directory, meaning, it skips the current directory... so the e2es fail.(Or something equally nefarious.)
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, found one example, and the example has a comparison with a directory name, so we'd need to do that to properly use
filepathSkipDir
; not sure it's worth 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.
Maybe switch over to
os.ReadDir()
, which makes the intent more clear anyways?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.
will look