-
Notifications
You must be signed in to change notification settings - Fork 3
feat: OCI image support #206
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you! I have not looked deeply into the changes yet (since it is in draft), but can already see that this changes the interface. It should be possible to do this in a non-breaking fashion, could you please look into that? |
|
Hi @maboehm 👋 thanks for the feedback. |
|
I think supporting both and defaulting to the new format is a good way forward. Old config will still work, and in case flux supports another source in the future it can be added without breaking changes. I think what's missing is:
@maboehm what do you think about the changes? |
Added in bdb2f86 |
pkg/apis/flux/v1alpha1/types.go
Outdated
| } | ||
|
|
||
| // Source configures how to bootstrap a Flux source object. | ||
| // For new configurations, use either GitRepository or OCIRepository. |
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 am not too happy about this API change, and I probably was not very clear in my comment about making this non-breaking - as this change as it stands will be breaking in a future release, but at least with a migration path.
The required configuration after your proposal feels redundant, since we embed the entire Type, which includes apiVersion+Kind:
source:
ociRepository:
template:
apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: OCIRepositoryWhat we can do instead, is the following: We keep the Template field, but change its go type to Template *runtime.RawExtension. Yes, this is breaking if you use this repo as a go dependency, but it is not breaking "on-the-wire", which is what I am mostly concerned about.
You can then use something like the following snippet to get to the underlying types:
func decode(raw []byte) (runtime.Object, error) {
typeMeta := &metav1.TypeMeta{}
if err := json.Unmarshal(raw, typeMeta); err != nil {
return nil, fmt.Errorf("failed to peek at GVK: %w", err)
}
gvk := typeMeta.GroupVersionKind()
if gvk.Kind == "" {
return nil, fmt.Errorf("could not find 'kind' in template")
}
obj, err := scheme.New(gvk)
if err != nil {
return nil, fmt.Errorf("scheme failed to create new %v: %w", gvk, err)
}
if err := runtime.DecodeInto(decoder, raw, obj); err != nil {
return nil, fmt.Errorf("failed to decode into %v: %w", gvk, err)
}
return obj, nil
}The returned runtime.Object can then be cast to a client.Object, or directly the specific type:
switch v := obj.(type) {
case *sourcev1.GitRepository:
slog.Info("gitRepository", "branch", v.Spec.Reference.Branch)
case *sourcev1.OCIRepository:
slog.Info("ociRepository", "tag", v.Spec.Reference.Tag)
default:
slog.Info("unknown type", "type", reflect.TypeOf(v))
}What do you think about this?
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.
thanks for the info @maboehm ! I'll take a look at this this afternoon.
|
@maboehm I've updated the code as suggested. It is now backwards compatible with the existing spec. |
maboehm
left a comment
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.
There is still a lot of unneeded dupliation that we can get rid of, but other than that, this looksr promising :)
| @@ -0,0 +1,201 @@ | |||
| package v1alpha1_test | |||
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 these tests are kind of pointless. What are we testing here, actually?
This mostly tests the encoding/decoding and runtime.RawExtension, which I think does not need to be tested this exhaustively.
I would probably just remove this file entirely...
|
|
||
| // decodeSourceTemplateForDefaulting decodes a runtime.RawExtension into a Flux source object. | ||
| // Returns the decoded object or nil if decoding fails (fails silently to not break defaulting). | ||
| func decodeSourceTemplateForDefaulting(raw *runtime.RawExtension) runtime.Object { |
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.
the decoding logis is duplicated between here and the actuator code. Is that necessary?
Should we export this here as DecodeSourceTemplate(d runtime.Decoder, raw *runtime.RawExtension) (runtime.Object, error) ?
| obj.Kustomization.Template.Spec.SourceRef.Namespace = sourceNamespace | ||
| // Decode source template to get name and namespace | ||
| sourceObj := decodeSourceTemplateForDefaulting(obj.Source.Template) | ||
| if sourceObj != nil { |
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.
wor accessing name and namespace you dont need to type switch, you can just use
obj2 := sourceObj.(client.Object)
obj2.GetNamespace()
| if obj.Source != nil && obj.Source.Template != nil { | ||
| // Decode, update namespace if needed, re-encode | ||
| sourceObj := decodeSourceTemplateForDefaulting(obj.Source.Template) | ||
| if sourceObj != nil { |
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.
similarly, you can use SetNamespace() after casting to client.Object
| switch v := sourceObj.(type) { | ||
| case *sourcev1.GitRepository: | ||
| SetDefaults_Flux_GitRepository(v) | ||
| modified = true |
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.
instead of always saying we modified the object, we could do something like
oldSource := v.DeepCopy()
// do defaulting
modified = !equality.Semantic.DeepEqual(oldSource, v)
|
|
||
| // decodeSourceTemplate decodes a runtime.RawExtension into a Flux source object. | ||
| // Returns the decoded object and its kind string, or an error if decoding fails. | ||
| func decodeSourceTemplate(raw *runtime.RawExtension) (runtime.Object, string, error) { |
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 again mostly duplicated.
| allErrs = append(allErrs, field.Required(specPath.Child("url"), "GitRepository must have a URL")) | ||
| } | ||
|
|
||
| // Validate secret references |
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.
can we dedupe the secret ref check?
| // usesDeprecatedSourceFormat checks if the providerConfig uses the old source format | ||
| // (source.template instead of source.gitRepository or source.ociRepository). | ||
| // This check is done on the raw JSON before defaulting/migration occurs. | ||
| func (a *actuator) usesDeprecatedSourceFormat(rawExtension *runtime.RawExtension) bool { |
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 can be removed, right?
| // Create OCIRepository | ||
| ociRepository := template.DeepCopy() | ||
| if _, err := controllerutil.CreateOrUpdate(ctx, shootClient, ociRepository, func() error { | ||
| template.Spec.DeepCopyInto(&ociRepository.Spec) | ||
| return nil | ||
| }); err != nil { | ||
| return fmt.Errorf("error applying OCIRepository template: %w", err) | ||
| } |
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.
technically, only this code needs to be repeated between the two types, the namespace creation and waitfor object calls should be deduped.
| }) | ||
| }) | ||
|
|
||
| var _ = Describe("usesDeprecatedSourceFormat", func() { |
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.
can be removed.
Summary
Adds support for OCI repositories as a Flux source type alongside the existing GitRepository support, enabling users to bootstrap Flux using OCI artifacts from container registries.
This is particularly useful when using code generators like CUE, Jsonnet, or KCL in CI pipelines to produce pre-built manifests stored as OCI artifacts.
Key changes:
OCIRepositorysource type with same bootstrap semantics as GitRepositoryChanges
API Types (
pkg/apis/flux/v1alpha1/)Sourcetype to support multiple source typesSourcecan contain eitherGitRepositoryorOCIRepository(mutually exclusive)GitRepositorySourceand added newOCIRepositorySourcetypesv1alpha1, existing configs automatically migrateValidation (
pkg/apis/flux/v1alpha1/validation/)ValidateOCIRepositorySource()function enforcing:ValidateSource()to enforce mutual exclusionDefaults (
pkg/apis/flux/v1alpha1/defaults.go)metadata.name: "flux-system"metadata.namespace: "flux-system"spec.interval: "1m"secretRef.name: "flux-system" (when secretResourceName provided)Controller (
pkg/controller/extension/)BootstrapSource()to dispatch to type-specific bootstrap functionsbootstrapOCIRepository()creates OCIRepository and waits for Ready conditionbootstrapGitRepository()(no behavior change for Git sources)Documentation
example/extension.yamlhack/api-reference/api.mdExample Configuration