✨ Add PullSecret controller to save pull secret data locally #425
Conversation
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
anik120
force-pushed
the
secret-controller
branch
from
6119a87
to
8375843
Compare
|
Test performed: Created:
Controller logs: Log line to note from the Unpacker: Final result 🎉 : |
anik120
force-pushed
the
secret-controller
branch
from
8375843
to
2605514
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 35.28% 37.31% +2.02%
==========================================
Files 14 15 +1
Lines 802 922 +120
==========================================
+ Hits 283 344 +61
- Misses 472 529 +57
- Partials 47 49 +2 ☔ View full report in Codecov by Sentry. |
skattoju
reviewed
Oct 4, 2024
| err := r.Get(ctx, req.NamespacedName, secret) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| logger.Info("secret not found") |
There was a problem hiding this comment.
should this be a warn or error saying it will be unable to pull from pvt registries ?
There was a problem hiding this comment.
So there's no WARN for this particular logger we're using, only Info or Error. I don't think this needs to be an Error, especially since the function we're calling logs additional messages about the auth file being deleted.
skattoju
reviewed
Oct 4, 2024
anik120
force-pushed
the
secret-controller
branch
from
2605514
to
d8159ec
Compare
trgeiger
reviewed
Oct 4, 2024
cmd/manager/main.go
Outdated
| @@ -105,6 +113,7 @@ func main() { | |||
| flag.StringVar(&keyFile, "tls-key", "", "The key file used for serving catalog contents over HTTPS. Requires tls-cert.") | |||
| flag.IntVar(&webhookPort, "webhook-server-port", 9443, "The port that the mutating webhook server serves at.") | |||
| flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of CA certificate to use for verifying HTTPS connections to image registries.") | |||
| flag.StringVar(&globalPullSecret, "global-pull-secret", "", "The namespace/name of the global pull secret that is going to be used to pull bundle images.") | |||
There was a problem hiding this comment.
I think this could be a bit confusing at a glance. One could read this as the namespace OR the name. Could format it like the error message below "/"
There was a problem hiding this comment.
Good call, changed.
trgeiger
reviewed
Oct 4, 2024
| // https://github.com/containers/image/blob/main/docs/containers-auth.json.5.md | ||
| err := os.WriteFile(r.AuthFilePath, dockerConfigJSON, 0600) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to write secret data to file: %w", err) |
There was a problem hiding this comment.
similar comment here as Sid's below re: revealing file location
There was a problem hiding this comment.
I removed the other ones, but I have the urge to keep this one. We'd want to see the specific error message for this when we get a must-gather (logs) for a bug report related to this.
trgeiger
reviewed
Oct 4, 2024
| @@ -49,12 +49,13 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv | |||
| return nil, reconcile.TerminalError(fmt.Errorf("error parsing catalog, catalog %s has a nil image source", catalog.Name)) | |||
| } | |||
|
|
|||
| srcCtx, err := i.SourceContextFunc(l) | |||
There was a problem hiding this comment.
linter pointed this out, but no err check here
There was a problem hiding this comment.
Oops missed this, thanks for pointing it out. Added an error check below.
anik120
force-pushed
the
secret-controller
branch
5 times, most recently
from
71bb7a0
to
ad9c5b0
Compare
anik120
changed the title
tmshort
reviewed
Oct 7, 2024
cmd/manager/main.go
Outdated
| storageDir = "catalogs" | ||
| authFilePath = "/etc/catalogd/auth.json" | ||
| storageDir = "catalogs" | ||
| authFilePrefix = "catalogd-global-pull-secret.json" |
There was a problem hiding this comment.
Should this have the .json extension?
There was a problem hiding this comment.
Ah I see the confusion. It says "prefix" there but it's actually the suffix. Changing it. Also I just noticed the fmt.Sprintf already has the .json, thanks for pointing it out.
There was a problem hiding this comment.
Should be fixed now. Here's what it looks like https://go.dev/play/p/FJlQgd_KS5R
anik120
force-pushed
the
secret-controller
branch
from
ad9c5b0
to
c1bf01a
Compare
|
Should this have changes that match operator-framework/operator-controller#1322 ? |
@tmshort do you mean this? Am I missing any changes? I've been trying to keep both of them in sync and I don't see anything that I missed. |
everettraven
previously approved these changes
Oct 8, 2024
anik120
force-pushed
the
secret-controller
branch
from
c1bf01a
to
2427c8a
Compare
everettraven
approved these changes
Oct 8, 2024
Merged
via the queue into
operator-framework:main
with commit Oct 8, 2024
b1b145a
12 of 13 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv