Skip to content

✨ Use creds if present for pulling bundle images #1303

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 26, 2024
Merged

✨ Use creds if present for pulling bundle images #1303

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

"github.com/containers/image/v5/types"
"github.com/go-logr/logr"
"github.com/spf13/pflag"
"go.uber.org/zap/zapcore"
apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
Expand Down Expand Up @@ -67,6 +68,8 @@ var (
defaultSystemNamespace = "olmv1-system"
)

const authFilePath = "/etc/operator-controller/auth.json"

// podNamespace checks whether the controller is running in a Pod vs.
// being run locally by inspecting the namespace file that gets mounted
// automatically for Pods at runtime. If that file doesn't exist, then
Expand Down Expand Up @@ -201,6 +204,7 @@ func main() {
SourceContext: &types.SystemContext{
DockerCertPath: caCertDir,
OCICertPath: caCertDir,
AuthFilePath: authFilePathIfPresent(setupLog),
},
}

Expand Down Expand Up @@ -311,3 +315,15 @@ type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Res
func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return f(ctx, obj)
}

func authFilePathIfPresent(logger logr.Logger) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could we name this something like mustAuthFilePathIfPresent or authFilePathIfPresentOrDie? I think that it would help someone reading through the code where this function is used to understand that if there is an error (unhandled error type in this case) that this function will exit the program.

Copy link
Contributor Author

@anik120 anik120 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're coming from.
....

not sure if I would understand what mustAuthFilePathIfPresent means if I didn't know the context....

authFilePathIfPresentOrDie sounds like it must find an auth file or panic if it's not present, which is not the case.

So I think this one's not that easy 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I could just leave comment on top that says unhandled error will cause is to exit with error code 1. Gets the job done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While writing the comment I realized I have to basically summarize in English words every single thing that the function is doing, just to get to the part about the error code 1, which seems superfluous 😅

// authFilePathIfPresent returns the value of the constant
// authFilePath ("/etc/catalogd/auth.json") if the file is
// present, emptry string ("") otherwise, using os.Stat(filePath).
// If os.Stat returns an error that's not IsNotExist, the function
// causes the program to exit with error code 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no reasonable way we can come up with to convey this as part of the function name that is fine. I won't hold the PR on this nit, just thought it worthwhile to call out

_, err := os.Stat(authFilePath)
if os.IsNotExist(err) {
return ""
}
if err != nil {
logger.Error(err, "could not stat auth file path", "path", authFilePath)
os.Exit(1)
}
return authFilePath
}