-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat: replace image registry dynamically #8018
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8018 +/- ##
==========================================
+ Coverage 61.32% 61.39% +0.07%
==========================================
Files 348 349 +1
Lines 40357 40457 +100
==========================================
+ Hits 24749 24840 +91
- Misses 13371 13391 +20
+ Partials 2237 2226 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/controllerutil/image_util.go
Outdated
} | ||
|
||
if dstNamespace == "" { | ||
dstNamespace = namespace |
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.
so if namespace is not found, the original namespace rather than the defaultNamespace is used.
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 add a DefaultNamespace
field to the RegistryConfig
struct:
type RegistryConfig struct {
From string
To string
DefaultNamespace string
Namespaces []RegistryNamespaceConfig
}
When the namespace is not found in Namespaces
, use the DefaultNamespace if it's not empty, otherwise use the original namespace.
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 simply use the global defaultNamespace is enough.
It does seem better.
@@ -52,4 +52,6 @@ const ( | |||
CfgKBReconcileWorkers = "KUBEBLOCKS_RECONCILE_WORKERS" | |||
CfgClientQPS = "CLIENT_QPS" | |||
CfgClientBurst = "CLIENT_BURST" | |||
|
|||
CfgRegistries = "registries" |
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.
A global flag is hard to config and maintain for the user, why not using the kb manager config CM?
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 does uses the manager config CM. This flag is the first level key in the config.
When does image replcaement happen
Bascially, replacement will happen when workload resources (like pod, job, deployment, statefulset) are created.
Specifically, the replacement will inject to:
instanceset controller, so that it affects:
But it won't affect:
addon controller, so that it affects helm install jobs
backup/restore controller
Replace Configs
sample config:
The replacement rules are as follows:
registriyConfig
field), or registry does not match any specific registry config,defaultRegistry
anddefaultNamespace
will be used.defaultNamespace
is not defined, namespace remains unchanged.defaultRegistry
is not defined, registry remains unchanged.from
andto
should not be empty. Registryto
will be used.registryDefaultNamespace
will be used. IfregistryDefaultNamespace
is not defined, namespace remains unchanged.to
will be used.from
andto
of the namespace may be empty (empty namespace is legal)Other things to note
One side effect of this change is that omitted image name (e.g.
busybox
) will be expanded to full format (e.g.docker.io/library/busybox
). IMO this will not have any user facing change.