-
Notifications
You must be signed in to change notification settings - Fork 344
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
fix 1802: leverage KEP-1755 to retrieve local image registry host an… #2696
Conversation
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.
Great! Last time I checked, Minikube was not going to support this. Has it changed?
Would it be possible to move that logic in the controller, like when reconciling the platform, instead of keeping it for the sole |
I guess it depends how much the |
Hehe, I asked it as a question more by courtesy but I don't expect any counter arguments 😃. |
Thanks for reviewing ! As @nicolaferraro said I'm not sure how public the "kube-public" namespace really is:
Although by convention it is supposed to be: "kube-public This namespace is created automatically and is readable by all users (including those not authenticated). This namespace is mostly reserved for cluster usage, in case that some resources should be visible and readable publicly throughout the whole cluster. The public aspect of this namespace is only a convention, not a requirement." Anyhow I've updated the PR so the logic is also in the operator. I've left it in the client as well so that
works without any other options (i.e without having to specify |
@@ -149,7 +150,15 @@ func configureRegistry(ctx context.Context, c client.Client, p *v1.IntegrationPl | |||
} | |||
} | |||
} | |||
|
|||
if p.Status.Build.Registry.Address == "" { |
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 it won't work for local install, as the operator service account do not have required permission.
I think the best solution is to create a dedicated ClusterRole with:
rules:
- apiGroups: [""]
resources: ["configmaps"]
resourceNames: ["local-registry-hosting"]
verbs: ["get"]
And a RoleBinding projecting that ClusterRole on the kube-public
namespace for the operator service account.
That means the operator service account do not have the required permission, but I would not deduce One solution is to create a dedicated ClusterRole with: rules:
- apiGroups: [""]
resources: ["configmaps"]
resourceNames: ["local-registry-hosting"]
verbs: ["get"] And a RoleBinding projecting that ClusterRole on the |
Ah yeah thanks, I think I tested with unauthorized users (which also didn't work) but I've added the RBAC files as you suggested which should make the question "is the kube-public namespace really public" null and void :) Thanks ! |
This PR has been automatically marked as stale due to 90 days of inactivity. |
Hey @johnpoth unfortunately this has some conflict ongoing now. Would you mind rebase with |
Thanks @squakez ! Rebased and merging.... |
…d port
Fixes #1802, this makes:
work on supporting clusters without additional arguments (tested successfully on k3d), thanks !
Release Note