feat: automate gardenlinux image lifecycle management in OpenStack CloudProfiles - #44
Conversation
📝 WalkthroughWalkthroughThe change adds shared machine-image synchronization models, a Glance source for regional image discovery, an OpenStack provider for CloudProfile updates, controller wiring, API fields, CRD schema entries, and generated deepcopy support. ChangesMachine image synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CloudProfileController
participant Glance
participant OpenStackProvider
participant CloudProfileSpec
CloudProfileController->>Glance: GetVersions
Glance-->>CloudProfileController: Return SourceImage versions
CloudProfileController->>OpenStackProvider: Configure versions
OpenStackProvider->>CloudProfileSpec: Merge provider configuration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (24)
cloudprofilesync/kubernetessync/kuberentes_image_updater.go (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the file name spelling.
The file is named
kuberentes_image_updater.go. The intended name iskubernetes_image_updater.go. The package namekubernetessyncand the typeKubernetesImageUpdaterare spelled correctly, so only the file name is affected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/kuberentes_image_updater.go` around lines 1 - 3, Rename the file from kuberentes_image_updater.go to kubernetes_image_updater.go; leave the kubernetessync package and KubernetesImageUpdater type unchanged.api/v1alpha1/managedcloudprofile.go (4)
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
omitemptyto thelandscapeSetupJSON tag.The field carries
+optionalbut the tag isjson:"landscapeSetup". Serialization then always emitslandscapeSetup: nullwhen the pointer is nil. Every other optional pointer field in this file usesomitempty.♻️ Proposed change
- LandscapeSetup *LandscapeSetup `json:"landscapeSetup"` + LandscapeSetup *LandscapeSetup `json:"landscapeSetup,omitempty"`Also applies to: 116-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/managedcloudprofile.go` around lines 22 - 25, Update the JSON tag for the optional landscapeSetup pointer field to include omitempty, matching the other optional pointer fields and preventing nil values from being serialized as landscapeSetup: null.
1-1: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTwo union types declare mutually exclusive fields but no schema validation enforces the combination. Both structs expose several optional pointer fields, mark them as alternatives in comments only, and rely on the consumer to pick one. The API server accepts a resource that sets none or all of them, and the consumer then resolves the ambiguity silently.
api/v1alpha1/managedcloudprofile.go#L148-155: add+kubebuilder:validation:XValidation:rule="has(self.personalAccessTokenSecret) != has(self.githubApp)"toKubernetesVersionSourceGithub. Todaycontrollers/cloud_profile.goevaluatesPersonalAccessTokenSecretfirst, so a resource that sets both ignores the GitHub App configuration.api/v1alpha1/managedcloudprofile.go#L170-177: add+kubebuilder:validation:XValidation:rule="has(self.oci) != has(self.glance)"toMachineImageUpdateSource.Regenerate the CRD after adding the markers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/managedcloudprofile.go` at line 1, Add CEL XValidation markers to KubernetesVersionSourceGithub and MachineImageUpdateSource enforcing exactly one mutually exclusive option is set: personalAccessTokenSecret versus githubApp, and oci versus glance. Regenerate the CRD manifests so the schema includes both validations.
170-177: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce the OCI/Glance source exclusivity in the schema.
Both fields are optional and no validation restricts the combination. A
MachineImageUpdateSourcewith neither field set, or with both set, passes admission. The consumer then decides silently. Add a CEL rule that requires exactly one source.🛡️ Proposed marker
+// +kubebuilder:validation:XValidation:rule="has(self.oci) != has(self.glance)",message="exactly one of oci or glance must be set" type MachineImageUpdateSource struct {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/managedcloudprofile.go` around lines 170 - 177, Update the MachineImageUpdateSource schema markers to add a CEL validation rule requiring exactly one of OCI or Glance to be set, rejecting both-empty and both-populated configurations while allowing either single-source configuration.
148-155: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce the PAT/GithubApp exclusivity in the schema.
The comments declare
PersonalAccessTokenSecretandGithubAppas mutually exclusive, but nothing enforces this.controllers/cloud_profile.go(lines 203-252) evaluatesPersonalAccessTokenSecretfirst in theswitch, so a resource that sets both silently ignores the GitHub App configuration. A resource that sets neither is only rejected at reconcile time, after admission.Add a CEL validation so the API server rejects both cases.
🛡️ Proposed marker
+// +kubebuilder:validation:XValidation:rule="has(self.personalAccessTokenSecret) != has(self.githubApp)",message="exactly one of personalAccessTokenSecret or githubApp must be set" type KubernetesVersionSourceGithub struct {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/managedcloudprofile.go` around lines 148 - 155, Add a CEL schema validation marker to the managed cloud profile authentication fields so exactly one of PersonalAccessTokenSecret and GithubApp is set: reject resources with both fields present and resources with neither. Ensure the generated CRD validation reflects this admission-time constraint while preserving the existing field definitions.crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml (1)
831-836: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire at least one Glance region.
regionsis required but an empty array satisfies the schema.Glance.GetVersionsthen starts no goroutines, and the guardlen(imagesByVersion) == 0 && len(skipped) == len(g.params.Regions)evaluates0 == 0, so reconciliation fails with the message "all 0 regions failed". Add+kubebuilder:validation:MinItems=1toGlanceSource.Regionsand regenerate the CRD.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml` around lines 831 - 836, Update GlanceSource.Regions with kubebuilder validation requiring at least one item, then regenerate the CRD so the managedcloudprofiles schema rejects empty regions arrays. Preserve the existing required regions behavior and generated schema structure.cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go (2)
233-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test does not exercise
FetchVersions.
TestFetchVersions_IntersectsAndFiltersnever callsFetchVersions. Lines 262-272 reimplement the intersection loop inside the test and then assert on that local result. The test therefore validates its own code and gives a false coverage signal for the production path at lines 145-160 oflandscape_source.go.Two related problems in the same block:
githubSrvat lines 235-240 is started and closed but no request ever reaches it.- The comment block at lines 242-254 records abandoned approaches. It describes a nil
ociRepo, a "thin wrapper", and a "helper that skips the OCI network call", none of which exist.Extract the intersection into an unexported function such as
intersect(supported []string, classification []kubernetessync.ExpirableVersion) []gardenerv1beta1.ExpirableVersion, call it fromFetchVersions, and assert on it here. Keep the?ref=assertion at lines 284-305 as a separate test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go` around lines 233 - 283, Refactor the intersection logic into an unexported helper such as intersect, accepting supported versions and classification values and returning ExpirableVersion results, then call that helper from FetchVersions. Update TestFetchVersions_IntersectsAndFilters to test the helper rather than reimplementing the loop, remove the unused githubSrv and abandoned-approach comments, and keep the existing ?ref= assertion as a separate test.
145-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the JWT signature and claims.
The test only counts the dot-separated segments. A
mintJWTthat emits a wrongalg, a wrongiss, or an invalid signature still passes. Decode the payload and checkissagainstappID, then verify the signature with the generated public key. The test already holdskey.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go` around lines 145 - 156, Extend TestGithubAppTransport_MintJWT beyond checking JWT segment count: decode the minted token’s payload and assert its iss claim matches the transport’s appID, then verify the token signature using the generated key’s public key. Keep the existing error and three-part validation while exercising the actual JWT algorithm and signature.cloudprofilesync/kubernetessync/source/landscape/landscape_source.go (3)
179-187: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSkip tags that are not valid semver instead of falling back to string comparison.
slices.MaxFunccompares pairwise. When either tag in a pair failssemver.ParseTolerant, the comparator falls back tocmp.Compareon the raw strings. A single non-semver tag such aslatestthen participates in the ordering and can win, because"latest" > "1.9.0"lexicographically. The function returns that tag as the "latest semver tag", and every downstream fetch uses the wrong artifact.Filter the tag list first, then take the maximum over the parsed versions.
♻️ Proposed fix
- latest := slices.MaxFunc(tags, func(a, b string) int { - va, ea := semver.ParseTolerant(a) - vb, eb := semver.ParseTolerant(b) - if ea != nil || eb != nil { - return cmp.Compare(a, b) - } - return va.Compare(vb) - }) - return latest, nil + parsable := make([]string, 0, len(tags)) + for _, tag := range tags { + if _, err := semver.ParseTolerant(tag); err == nil { + parsable = append(parsable, tag) + } + } + if len(parsable) == 0 { + return "", fmt.Errorf("no semver tags found in %s", s.ociRepo.Reference) + } + return slices.MaxFunc(parsable, func(a, b string) int { + va, _ := semver.ParseTolerant(a) + vb, _ := semver.ParseTolerant(b) + return va.Compare(vb) + }), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go` around lines 179 - 187, Update the tag-selection logic around slices.MaxFunc to exclude tags that semver.ParseTolerant cannot parse before determining the maximum. Compare only valid parsed semver values, preserve the existing latest-tag return contract, and handle the case where no valid semver tags remain without allowing raw string ordering to select a tag.
218-227: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the reads from the registry and from GitHub.
Three reads have no size limit:
- Line 222:
content.FetchAllloads the whole first layer into memory.- Line 248:
io.ReadAll(tr)reads the tar entry without a cap, so a decompression-bomb style entry can exhaust memory.- Lines 296 and 303:
io.ReadAll(resp.Body)reads the GitHub response and the error body without a cap.A component descriptor and a versions YAML are both small. Wrap each read in an
io.LimitReaderwith an explicit maximum and return an error when the limit is reached. This keeps a misbehaving or hostile registry from causing an out-of-memory kill of the controller.Also applies to: 248-251, 296-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go` around lines 218 - 227, Bound all external reads in the landscape source: replace the layer retrieval around content.FetchAll, tar-entry read in extractComponentDescriptor, and GitHub response/error-body reads with explicit-size LimitedReaders. Detect when each limit is exceeded and return a descriptive error, while preserving normal parsing for component descriptors and versions YAML.
208-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the component-descriptor layer by media type
The OCI manifest does not assign a special role to
manifest.Layers[0]. Select the OCM component-descriptor layer by media type, or scan all layers, before callingextractComponentDescriptor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go` around lines 208 - 233, Update fetchComponentDescriptor to locate the manifest layer whose media type identifies an OCM component descriptor instead of assuming manifest.Layers[0]. Scan manifest.Layers for that media type, fetch the matching descriptor layer, and return an appropriate error when no matching layer exists before calling extractComponentDescriptor.controllers/managedcloudprofile_controller_test.go (4)
681-684: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion depends on an exact
oraserror string.The matcher requires the message
invalid reference: invalid repository "/registry/account/repository". That text comes from theoras-golibrary. A dependency upgrade that rewords the error breaks this test even though the controller behavior is unchanged.Assert on the stable part only, for example
failed to initialize OCI source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/managedcloudprofile_controller_test.go` around lines 681 - 684, Update the message matcher in the managed cloud profile apply-failure assertion to check only the stable phrase “failed to initialize OCI source,” removing the dependency on the exact oras-go error text while preserving the existing ApplyFailed condition checks.
933-945: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe inline spec-building closure is repeated in four tests.
This
func() v1alpha1.CloudProfileSpec { ... }()pattern that callsbaseCloudProfileSpecand then setsProviderConfigappears at Line 933, Line 1034, Line 1137, and Line 1227. Add a helper next tobaseCloudProfileSpec, for examplecloudProfileSpecWithProviderConfig(raw []byte, images ...gardenerv1beta1.MachineImage), and call it from all four tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/managedcloudprofile_controller_test.go` around lines 933 - 945, Extract the repeated inline CloudProfileSpec construction into a helper next to baseCloudProfileSpec, such as cloudProfileSpecWithProviderConfig, accepting raw provider configuration bytes and variadic MachineImage values. Have it build the base spec, assign ProviderConfig, and return the result; replace all four inline func() v1alpha1.CloudProfileSpec closures with calls to this helper.
127-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
expectAppliedConditionasserts against a possibly stale object.The helper reads
mcp.Status.Conditionsfrom the in-memory object. It does not fetch the object, so it only works when the caller already calledexpectReconcileStatus, which refreshesmcp. A future test that calls this helper alone asserts against stale conditions and can pass incorrectly.Fetch the object inside the helper, or wrap the assertion in
Eventuallywith aGet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/managedcloudprofile_controller_test.go` around lines 127 - 136, Update expectAppliedCondition to retrieve the current ManagedCloudProfile from the Kubernetes client before asserting conditions, rather than reading the potentially stale mcp.Status.Conditions directly. Preserve the existing status and extra matcher checks, and use the refreshed object for the assertion.
166-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPopulate
CloudProfileSpec.TypeinbaseCloudProfileSpec.The CRD requires
type, butjson:"type"serializes the zero value as"type": "", so the API server accepts the fixture. SetType: "test"so successful reconcile tests use a valid provider type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/managedcloudprofile_controller_test.go` around lines 166 - 188, Update baseCloudProfileSpec to set CloudProfileSpec.Type to "test" when constructing the fixture, ensuring successful reconcile tests use a valid provider type while preserving the existing fields.controllers/garbage_collection.go (6)
358-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport garbage collection failures on a separate condition type.
failWithStatusUpdatewrites toCloudProfileAppliedConditionType.reconcileCloudProfilealready set that condition toTruewith reasonAppliedin the same reconcile pass. A garbage collection failure therefore flips the "applied" condition toFalse, even though the CloudProfile was applied. Consumers cannot distinguish the two failures.Add a dedicated condition type, for example
GarbageCollectionSucceeded, and keepCloudProfileAppliedfor the apply step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 358 - 369, Update failWithStatusUpdate to write the failure condition using a dedicated GarbageCollectionSucceeded condition type instead of CloudProfileAppliedConditionType. Preserve CloudProfileAppliedConditionType for the successful apply status set by reconcileCloudProfile, and define or reuse the dedicated condition constant consistently.
103-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the skipped deletion when the update is rejected as invalid.
If
deleteVersionsreturns anInvalidAPI error, the loop continues silently. The ManagedCloudProfile status stays unchanged, so an operator gets no signal that garbage collection did not apply for that image. Add a log entry with the image name and the error beforecontinue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 103 - 108, Update the deleteVersions error handling in the garbage-collection loop so apierrors.IsInvalid(err) logs the skipped deletion before continuing. Include updates.ImageName and the original error in the log entry, while preserving the existing continue behavior and status handling.
68-91: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftHoist the Shoot listing out of the per-image loop.
getReferencedVersionslists every Shoot in every namespace, and the loop calls it once per entry inmcp.Spec.MachineImageUpdates. It also performs a separateGetof the same CloudProfile on each call. With several image updates, each reconcile issues several full Shoot list calls against the API server, and the controller reconciles every 5 minutes.List the Shoots once before the loop, then filter per image name.
♻️ Sketch of the restructured flow
cutoff := time.Now().Add(-mcp.Spec.GarbageCollection.MaxAge.Duration) + + shootList := &gardenerv1beta1.ShootList{} + if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err != nil { + return r.failWithStatusUpdate(ctx, mcp, fmt.Errorf("failed to list Shoots: %w", err)) + } for _, updates := range mcp.Spec.MachineImageUpdates {Then change
getReferencedVersionsto accept the pre-fetchedshootListand the already-loaded CloudProfile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 68 - 91, Hoist shared data loading out of the MachineImageUpdates loop: list all Shoots once and load the CloudProfile once before iterating. Update getReferencedVersions to accept the pre-fetched Shoot list and CloudProfile, then filter references by each updates.ImageName without issuing additional list or Get calls per image.
192-195: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd conflict handling to the CloudProfile read-modify-write.
deleteVersionsperformsGetthenUpdatewith no retry.reconcileCloudProfilepatches the same CloudProfile earlier in the same reconcile, and the Gardener controllers also write to it. AConflicterror is notInvalid, so it propagates tofailWithStatusUpdate, which sets the ManagedCloudProfile status toFailedfor a transient condition.Wrap the read-modify-write in
retry.RetryOnConflict, or usecontrollerutil.CreateOrPatchso the update is applied as a patch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 192 - 195, Update deleteVersions to perform its CloudProfile Get-and-Update operation through retry.RetryOnConflict, retrying the read-modify-write when a resource version conflict occurs while preserving the existing error handling for non-conflict failures.
292-308: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe manifests response is read as a single page.
fetchKeppelTagsdecodes one response body and never follows pagination. If the Keppel account holds more manifests than one page returns, the missing tags never appear intags. Garbage collection then skips those versions. The direction is safe, because nothing extra is deleted, but old versions accumulate without any signal.Add marker or limit handling, or log the manifest count so the truncation is visible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 292 - 308, Update fetchKeppelTags to handle paginated Keppel manifest responses by following the response’s marker or limit metadata and aggregating manifests across all pages before building tagMap. Ensure tags from every page are included, or at minimum log a clear signal when the response is truncated if pagination cannot be implemented.
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hostname substring heuristic with explicit configuration.
getRegistryProviderselects the Keppel client when the registry host contains the substringkeppel. A Keppel deployment behind a vanity hostname does not match, and garbage collection then fails the whole reconcile with "no registry provider found for registry". Add an explicit registry type field to the OCI source configuration, and keep the substring match only as a fallback.The receiver
ris also unused; the function can be a package-level function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/garbage_collection.go` around lines 33 - 41, Update OCI source configuration to include an explicit registry type and make getRegistryProvider use that type to select KeppelClient, retaining the existing hostname substring check only when the type is unset. Convert getRegistryProvider from a Reconciler method to a package-level function and update its callers accordingly, preserving empty-registry and unsupported-provider errors.controllers/cloud_profile.go (3)
98-108: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider making the OCI parallelism configurable.
parallelis hard-coded to1. The OCI source uses this value as the semaphore weight when it fetches one manifest per tag, so all manifest fetches run sequentially. The Glance source already exposesParallelthroughv1alpha1.GlanceSource. Add an equivalent field for the OCI source, or define a named constant that documents the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/cloud_profile.go` around lines 98 - 108, Update the OCI source initialization in the relevant controller method so its parallelism is no longer an unexplained hard-coded value of 1: preferably expose an OCI parallelism field through the OCI source configuration and pass it to OCISourceFactory.Create, or define and use a named constant documenting the intentional sequential behavior.
110-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a factory for the Glance source.
The OCI branch resolves its source through
r.OCISourceFactory, which lets tests inject a fake. The Glance branch callsglance.NewGlancedirectly, so the Glance path cannot be exercised without live OpenStack endpoints. Add aGlanceSourceFactoryfield onReconcilerwith a default implementation, in the same way asDefaultOCISourceFactory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/cloud_profile.go` around lines 110 - 130, Add a GlanceSourceFactory field to Reconciler with a default implementation matching DefaultOCISourceFactory, then update the Glance branch in the source update flow to create the source through that factory instead of calling glance.NewGlance directly. Preserve the existing Glance parameters and initialization error handling while enabling tests to inject a fake factory.
177-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
KubernetesImageUpdaterinterface, or use it.
updateKubernetesVersionscallskubernetessync.NewKubernetesImageUpdaterand uses the concrete type. Nothing in this file references theKubernetesImageUpdaterinterface. Either delete it, or declare the updater through it so the Kubernetes path becomes injectable in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/cloud_profile.go` around lines 177 - 179, Remove the unused KubernetesImageUpdater interface declaration, since updateKubernetesVersions currently uses the concrete updater returned by kubernetessync.NewKubernetesImageUpdater; alternatively, change that path to depend on the interface and preserve injectable test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/managedcloudprofile.go`:
- Around line 181-184: Update NewGlance to validate AuthURLFormat before using
fmt.Sprintf, requiring exactly one %s placeholder and rejecting all other
formatting directives, including malformed verbs. Return a clear validation
error for invalid values while preserving the existing empty-value handling and
valid region URL generation.
In `@cloudprofilesync/kubernetessync/kuberentes_image_updater.go`:
- Line 57: Update the Kubernetes version handling in Update so
cpSpec.Kubernetes.Versions preserves base CloudProfile entries and merges
provider versions by version, with source values winning conflicts. Keep
operator-declared versions that are absent from the update source, matching the
machine-image provider merge behavior.
- Around line 48-55: Align the version filtering in the updater with the
documented expiration behavior: use a future cutoff based on
time.Now().Add(ku.ExpirationThreshold) and retain only versions expiring after
that cutoff. Update both related API field comments in managedcloudprofile.go to
describe the same behavior and ensure the implementation and documentation
consistently remove versions expiring within the threshold.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 92-94: Update GithubPATTransport and githubAppTransport.RoundTrip
to retain and use the configured apiBase when applying Authorization. Compare
each request’s destination host with the API base host, attach the bearer token
only for matching hosts, and leave the header unset for cross-host redirects.
- Around line 278-304: Update NewLandscapeKubernetesSource and
exchangeInstallationToken to ensure GitHub HTTP requests use a finite client
timeout, including requests sent through base.RoundTrip. In fetchGithubFile,
build the ref query parameter with standard URL query encoding instead of
concatenating it, preserving valid requests for tags containing &, #, or spaces.
- Around line 356-386: Protect the cached token check and refresh in
githubAppTransport.installationToken with a mutex, including reads of
cached/expiresAt and the mintJWT/exchangeInstallationToken sequence, so
concurrent RoundTrip calls cannot race or duplicate exchanges. Add the mutex to
githubAppTransport and preserve the existing cache-expiry behavior; do not
change transport construction or caching scope.
In `@cloudprofilesync/ossync/os_image_updater.go`:
- Around line 157-159: Update the reconciliation logic around image version
classification and expiration so both existing full-tag and clean-version
entries always assign InPlaceUpdates from SourceImage.SupportInPlaceUpdate on
every reconciliation. Ensure both false-to-true and true-to-false transitions
overwrite the prior value, and add tests covering each transition for both entry
types.
In `@cloudprofilesync/ossync/source/glance/os_source.go`:
- Around line 154-176: Update GetVersions so region workers cannot block sending
results after an early context cancellation return: make the out channel
buffered to accommodate every configured region result, while preserving the
existing cancellation and deadline error behavior.
In `@cloudprofilesync/ossync/source/oci/os_source.go`:
- Around line 186-193: Add a separate raw-tag field to ossync.SourceImage and
populate it with tag, while retaining the normalized strings.ReplaceAll value in
Version for Gardener metadata. Update the Ironcore provider image-reference
construction and garbage-collection protection logic to use the raw-tag field,
ensuring registry lookups and comparisons preserve underscores.
In `@controllers/garbage_collection.go`:
- Around line 311-325: Update keppelURL to construct the endpoint with
url.URL.JoinPath instead of fmt.Sprintf, ensuring the base URL, account, repo,
and "_manifests" components are joined with account and repo safely escaped as
path segments while preserving the existing splitKeppelRepository error handling
and return contract.
- Around line 205-218: Update the Shoot filtering logic in the
garbage-collection loop to resolve both spec.cloudProfileName and
NamespacedCloudProfile references, including following
NamespacedCloudProfile.spec.parent to the effective parent CloudProfile. Match
the resolved parent against cloudProfileName before collecting worker image
versions in referenced, preserving all applicable references before deletion.
- Around line 256-268: Update reconcileGarbageCollection and fetchKeppelTags to
resolve OCI credentials and propagate the registry’s Insecure setting; extend
the RegistryClient.GetTags call and fetchKeppelTags parameters accordingly.
Replace the hard-coded registryBaseURL(registry, false) and unauthenticated
request with the existing registry authentication flow, including credentials
and insecure transport configuration.
- Around line 125-175: Update the ProviderConfig handling in deleteVersions to
run only for an explicitly identified Ironcore provider, or mutate its raw JSON
while retaining unknown provider-specific fields. Preserve fields such as
constraints and per-version regions for non-Ironcore configurations, and do not
determine provider identity solely from apiVersion or kind because TypeMeta may
be unset.
In `@crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml`:
- Around line 604-612: Update KubernetesVersionUpdateConfig.ExpirationThreshold
in api/v1alpha1/managedcloudprofile.go to include the existing non-negative
validation marker used by garbageCollection.maxAge, then regenerate the CRD so
the expirationThreshold schema contains the corresponding CEL rule.
In `@go.mod`:
- Line 45: Update the go.mod require declarations for
github.com/gardener/gardener-extension-provider-openstack and
github.com/gophercloud/gophercloud/v2 to remove the indirect markers and place
both modules in the direct require block, then run go mod tidy to ensure the
module file is consistent.
---
Nitpick comments:
In `@api/v1alpha1/managedcloudprofile.go`:
- Around line 22-25: Update the JSON tag for the optional landscapeSetup pointer
field to include omitempty, matching the other optional pointer fields and
preventing nil values from being serialized as landscapeSetup: null.
- Line 1: Add CEL XValidation markers to KubernetesVersionSourceGithub and
MachineImageUpdateSource enforcing exactly one mutually exclusive option is set:
personalAccessTokenSecret versus githubApp, and oci versus glance. Regenerate
the CRD manifests so the schema includes both validations.
- Around line 170-177: Update the MachineImageUpdateSource schema markers to add
a CEL validation rule requiring exactly one of OCI or Glance to be set,
rejecting both-empty and both-populated configurations while allowing either
single-source configuration.
- Around line 148-155: Add a CEL schema validation marker to the managed cloud
profile authentication fields so exactly one of PersonalAccessTokenSecret and
GithubApp is set: reject resources with both fields present and resources with
neither. Ensure the generated CRD validation reflects this admission-time
constraint while preserving the existing field definitions.
In `@cloudprofilesync/kubernetessync/kuberentes_image_updater.go`:
- Around line 1-3: Rename the file from kuberentes_image_updater.go to
kubernetes_image_updater.go; leave the kubernetessync package and
KubernetesImageUpdater type unchanged.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go`:
- Around line 233-283: Refactor the intersection logic into an unexported helper
such as intersect, accepting supported versions and classification values and
returning ExpirableVersion results, then call that helper from FetchVersions.
Update TestFetchVersions_IntersectsAndFilters to test the helper rather than
reimplementing the loop, remove the unused githubSrv and abandoned-approach
comments, and keep the existing ?ref= assertion as a separate test.
- Around line 145-156: Extend TestGithubAppTransport_MintJWT beyond checking JWT
segment count: decode the minted token’s payload and assert its iss claim
matches the transport’s appID, then verify the token signature using the
generated key’s public key. Keep the existing error and three-part validation
while exercising the actual JWT algorithm and signature.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 179-187: Update the tag-selection logic around slices.MaxFunc to
exclude tags that semver.ParseTolerant cannot parse before determining the
maximum. Compare only valid parsed semver values, preserve the existing
latest-tag return contract, and handle the case where no valid semver tags
remain without allowing raw string ordering to select a tag.
- Around line 218-227: Bound all external reads in the landscape source: replace
the layer retrieval around content.FetchAll, tar-entry read in
extractComponentDescriptor, and GitHub response/error-body reads with
explicit-size LimitedReaders. Detect when each limit is exceeded and return a
descriptive error, while preserving normal parsing for component descriptors and
versions YAML.
- Around line 208-233: Update fetchComponentDescriptor to locate the manifest
layer whose media type identifies an OCM component descriptor instead of
assuming manifest.Layers[0]. Scan manifest.Layers for that media type, fetch the
matching descriptor layer, and return an appropriate error when no matching
layer exists before calling extractComponentDescriptor.
In `@controllers/cloud_profile.go`:
- Around line 98-108: Update the OCI source initialization in the relevant
controller method so its parallelism is no longer an unexplained hard-coded
value of 1: preferably expose an OCI parallelism field through the OCI source
configuration and pass it to OCISourceFactory.Create, or define and use a named
constant documenting the intentional sequential behavior.
- Around line 110-130: Add a GlanceSourceFactory field to Reconciler with a
default implementation matching DefaultOCISourceFactory, then update the Glance
branch in the source update flow to create the source through that factory
instead of calling glance.NewGlance directly. Preserve the existing Glance
parameters and initialization error handling while enabling tests to inject a
fake factory.
- Around line 177-179: Remove the unused KubernetesImageUpdater interface
declaration, since updateKubernetesVersions currently uses the concrete updater
returned by kubernetessync.NewKubernetesImageUpdater; alternatively, change that
path to depend on the interface and preserve injectable test behavior.
In `@controllers/garbage_collection.go`:
- Around line 358-369: Update failWithStatusUpdate to write the failure
condition using a dedicated GarbageCollectionSucceeded condition type instead of
CloudProfileAppliedConditionType. Preserve CloudProfileAppliedConditionType for
the successful apply status set by reconcileCloudProfile, and define or reuse
the dedicated condition constant consistently.
- Around line 103-108: Update the deleteVersions error handling in the
garbage-collection loop so apierrors.IsInvalid(err) logs the skipped deletion
before continuing. Include updates.ImageName and the original error in the log
entry, while preserving the existing continue behavior and status handling.
- Around line 68-91: Hoist shared data loading out of the MachineImageUpdates
loop: list all Shoots once and load the CloudProfile once before iterating.
Update getReferencedVersions to accept the pre-fetched Shoot list and
CloudProfile, then filter references by each updates.ImageName without issuing
additional list or Get calls per image.
- Around line 192-195: Update deleteVersions to perform its CloudProfile
Get-and-Update operation through retry.RetryOnConflict, retrying the
read-modify-write when a resource version conflict occurs while preserving the
existing error handling for non-conflict failures.
- Around line 292-308: Update fetchKeppelTags to handle paginated Keppel
manifest responses by following the response’s marker or limit metadata and
aggregating manifests across all pages before building tagMap. Ensure tags from
every page are included, or at minimum log a clear signal when the response is
truncated if pagination cannot be implemented.
- Around line 33-41: Update OCI source configuration to include an explicit
registry type and make getRegistryProvider use that type to select KeppelClient,
retaining the existing hostname substring check only when the type is unset.
Convert getRegistryProvider from a Reconciler method to a package-level function
and update its callers accordingly, preserving empty-registry and
unsupported-provider errors.
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 681-684: Update the message matcher in the managed cloud profile
apply-failure assertion to check only the stable phrase “failed to initialize
OCI source,” removing the dependency on the exact oras-go error text while
preserving the existing ApplyFailed condition checks.
- Around line 933-945: Extract the repeated inline CloudProfileSpec construction
into a helper next to baseCloudProfileSpec, such as
cloudProfileSpecWithProviderConfig, accepting raw provider configuration bytes
and variadic MachineImage values. Have it build the base spec, assign
ProviderConfig, and return the result; replace all four inline func()
v1alpha1.CloudProfileSpec closures with calls to this helper.
- Around line 127-136: Update expectAppliedCondition to retrieve the current
ManagedCloudProfile from the Kubernetes client before asserting conditions,
rather than reading the potentially stale mcp.Status.Conditions directly.
Preserve the existing status and extra matcher checks, and use the refreshed
object for the assertion.
- Around line 166-188: Update baseCloudProfileSpec to set CloudProfileSpec.Type
to "test" when constructing the fixture, ensuring successful reconcile tests use
a valid provider type while preserving the existing fields.
In `@crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml`:
- Around line 831-836: Update GlanceSource.Regions with kubebuilder validation
requiring at least one item, then regenerate the CRD so the managedcloudprofiles
schema rejects empty regions arrays. Preserve the existing required regions
behavior and generated schema structure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2575b40e-5d83-4647-9691-042c86660395
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
api/v1alpha1/managedcloudprofile.goapi/v1alpha1/zz_generated.deepcopy.gocloudprofilesync/kubernetessync/kuberentes_image_updater.gocloudprofilesync/kubernetessync/source/landscape/landscape_source.gocloudprofilesync/kubernetessync/source/landscape/landscape_source_test.gocloudprofilesync/ossync/os_image_updater.gocloudprofilesync/ossync/os_image_updater_test.gocloudprofilesync/ossync/provider/ironcore/provider.gocloudprofilesync/ossync/provider/ironcore/provider_test.gocloudprofilesync/ossync/provider/openstack/provider.gocloudprofilesync/ossync/provider/openstack/provider_test.gocloudprofilesync/ossync/source/glance/os_source.gocloudprofilesync/ossync/source/glance/os_source_test.gocloudprofilesync/ossync/source/oci/os_source.gocloudprofilesync/ossync/source/oci/os_source_test.gocloudprofilesync/ossync/source/oci/suite_test.gocloudprofilesync/ossync/suite_test.gocontrollers/cloud_profile.gocontrollers/garbage_collection.gocontrollers/managedcloudprofile_controller.gocontrollers/managedcloudprofile_controller_test.gocrd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yamlgo.mod
| // AuthURLFormat is the Keystone endpoint format string with a single "%s" for the region. | ||
| AuthURLFormat string `json:"authURLFormat"` | ||
| // Regions is the list of OpenStack regions to query. | ||
| Regions []string `json:"regions"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how AuthURLFormat is consumed and whether it is validated.
fd -t f 'os_source.go' cloudprofilesync/ossync/source/glance --exec rg -n -C 5 'AuthURLFormat|Sprintf|authURL'
rg -n -C 3 'AuthURLFormat' --type=goRepository: cobaltcore-dev/cloud-profile-sync
Length of output: 2347
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(managedcloudprofile\.go|os_source\.go|.*cloudprofile.*|.*crd.*|.*managed.*profile.*)$' | head -200
printf '%s\n' '--- API type and validation context ---'
sed -n '130,220p' api/v1alpha1/managedcloudprofile.go
rg -n -C 8 'AuthURLFormat|authURLFormat' .
printf '%s\n' '--- Glance constructor and formatter context ---'
fd -t f . cloudprofilesync/ossync/source/glance --exec sh -c 'echo "--- $1"; sed -n "1,135p" "$1"; sed -n "210,240p" "$1"' sh {}
printf '%s\n' '--- tests covering constructor or AuthURLFormat ---'
rg -n -C 8 'NewGlance|GlanceParams|AuthURLFormat|authURLFormat' --glob '*_test.go' .Repository: cobaltcore-dev/cloud-profile-sync
Length of output: 32898
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- generated CRD schema ---'
sed -n '785,800p' crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml
printf '%s\n' '--- standalone fmt.Sprintf behavior ---'
cat >/tmp/check_fmt.go <<'GO'
package main
import "fmt"
func main() {
for _, format := range []string{
"https://identity.%s/v3",
"https://identity.example/v3",
"https://identity.%s.%s/v3",
"https://identity.%x/v3",
"https://identity.%%s/v3",
} {
fmt.Printf("%q => %q\n", format, fmt.Sprintf(format, "eu-de-1"))
}
}
GO
if command -v go >/dev/null 2>&1; then
go run /tmp/check_fmt.go
else
printf '%s\n' 'go unavailable'
fi
printf '%s\n' '--- constructor validation and format call ---'
rg -n -C 5 'if params\.AuthURLFormat|fmt\.Sprintf\(g\.params\.AuthURLFormat|authenticate:|region %s: authenticate' \
cloudprofilesync/ossync/source/glance/os_source.goRepository: cobaltcore-dev/cloud-profile-sync
Length of output: 3021
Reject invalid AuthURLFormat values. NewGlance checks only for an empty string, then passes the value to fmt.Sprintf; invalid verbs produce incorrect URLs or %! output and region-scoped authentication errors. Validate one %s and no other formatting directives in NewGlance, or add an equivalent CRD constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/managedcloudprofile.go` around lines 181 - 184, Update NewGlance
to validate AuthURLFormat before using fmt.Sprintf, requiring exactly one %s
placeholder and rejecting all other formatting directives, including malformed
verbs. Return a clear validation error for invalid values while preserving the
existing empty-value handling and valid region URL generation.
| deleteThreshold := time.Now().Add(-ku.ExpirationThreshold) | ||
| cpVersions := make([]gardenerv1beta1.ExpirableVersion, 0, len(versions)) | ||
| for _, v := range versions { | ||
| if v.ExpirationDate != nil && v.ExpirationDate.Time.Before(deleteThreshold) { //nolint:staticcheck | ||
| continue | ||
| } | ||
| cpVersions = append(cpVersions, v) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The filter direction contradicts the API documentation.
deleteThreshold is now - ExpirationThreshold, and a version is skipped only when its expiration date is before that point. The code therefore keeps already-expired versions for an extra grace period equal to the threshold.
The API field documentation in api/v1alpha1/managedcloudprofile.go (lines 117-118) states the opposite: "Versions that are expiring within this threshold will be removed from the CloudProfile". That describes a cutoff of now + ExpirationThreshold.
Decide which behavior is intended, then align the code and the two doc comments. If the API doc is correct, the cutoff must be time.Now().Add(ku.ExpirationThreshold) and the comparison must keep versions that expire after it.
🐛 Fix if the API documentation states the intended behavior
- deleteThreshold := time.Now().Add(-ku.ExpirationThreshold)
+ deleteThreshold := time.Now().Add(ku.ExpirationThreshold)
cpVersions := make([]gardenerv1beta1.ExpirableVersion, 0, len(versions))
for _, v := range versions {
if v.ExpirationDate != nil && v.ExpirationDate.Time.Before(deleteThreshold) { //nolint:staticcheck
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deleteThreshold := time.Now().Add(-ku.ExpirationThreshold) | |
| cpVersions := make([]gardenerv1beta1.ExpirableVersion, 0, len(versions)) | |
| for _, v := range versions { | |
| if v.ExpirationDate != nil && v.ExpirationDate.Time.Before(deleteThreshold) { //nolint:staticcheck | |
| continue | |
| } | |
| cpVersions = append(cpVersions, v) | |
| } | |
| deleteThreshold := time.Now().Add(ku.ExpirationThreshold) | |
| cpVersions := make([]gardenerv1beta1.ExpirableVersion, 0, len(versions)) | |
| for _, v := range versions { | |
| if v.ExpirationDate != nil && v.ExpirationDate.Time.Before(deleteThreshold) { //nolint:staticcheck | |
| continue | |
| } | |
| cpVersions = append(cpVersions, v) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudprofilesync/kubernetessync/kuberentes_image_updater.go` around lines 48
- 55, Align the version filtering in the updater with the documented expiration
behavior: use a future cutoff based on time.Now().Add(ku.ExpirationThreshold)
and retain only versions expiring after that cutoff. Update both related API
field comments in managedcloudprofile.go to describe the same behavior and
ensure the implementation and documentation consistently remove versions
expiring within the threshold.
| cpVersions = append(cpVersions, v) | ||
| } | ||
|
|
||
| cpSpec.Kubernetes.Versions = cpVersions |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The assignment discards Kubernetes versions from the base CloudProfile spec.
Update replaces cpSpec.Kubernetes.Versions instead of merging. In controllers/cloud_profile.go the reconciler first assigns cloudProfile.Spec = CloudProfileSpecToGardener(&mcp.Spec.CloudProfile), then calls updateKubernetesVersions. Any version that an operator declared under spec.cloudProfile.kubernetes.versions is therefore dropped without a warning as soon as kubernetesVersionUpdateConfig is set. The CRD marks cloudProfile.kubernetes as required, so operators do supply that block.
The machine-image providers merge into existing entries rather than replacing them. Confirm that replacement is intended here. If it is, document it on the KubernetesVersionUpdateConfig API type. If it is not, merge by version and let the source win on conflicts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudprofilesync/kubernetessync/kuberentes_image_updater.go` at line 57,
Update the Kubernetes version handling in Update so cpSpec.Kubernetes.Versions
preserves base CloudProfile entries and merges provider versions by version,
with source values winning conflicts. Keep operator-declared versions that are
absent from the update source, matching the machine-image provider merge
behavior.
| func GithubPATTransport(apiBase, token string) http.RoundTripper { | ||
| return &patTransport{token: token, base: http.DefaultTransport} | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The PAT transport attaches the token to every request host.
GithubPATTransport accepts apiBase and discards it. patTransport.RoundTrip then sets Authorization: Bearer <token> on every request that passes through the client, without checking the destination.
fetchGithubFile uses s.githubClient, which follows redirects by default. Go strips sensitive headers on a cross-host redirect only for headers that the http.Client itself carries forward. This transport re-adds the header on each hop, so the redirected request to the new host still carries the token. A redirect returned by the configured repositoryApiUrl host therefore leaks the personal access token to an arbitrary host.
The same applies to githubAppTransport.RoundTrip at lines 361-369, which already stores apiBase.
Compare the request host against the configured API base and skip the header when they differ.
🛡️ Proposed fix
-func GithubPATTransport(apiBase, token string) http.RoundTripper {
- return &patTransport{token: token, base: http.DefaultTransport}
-}
+func GithubPATTransport(apiBase, token string) http.RoundTripper {
+ return &patTransport{token: token, apiBase: apiBase, base: http.DefaultTransport}
+}
+
+// sameHost reports whether the request targets the configured API base host.
+func sameHost(apiBase string, req *http.Request) bool {
+ base, err := url.Parse(apiBase)
+ if err != nil {
+ return false
+ }
+ return strings.EqualFold(base.Host, req.URL.Host)
+} type patTransport struct {
token string
+ apiBase string
base http.RoundTripper
}
func (t *patTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+ if !sameHost(t.apiBase, req) {
+ return t.base.RoundTrip(req)
+ }
r := req.Clone(req.Context())
r.Header.Set("Authorization", "Bearer "+t.token)
return t.base.RoundTrip(r)
}Also applies to: 340-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go` around
lines 92 - 94, Update GithubPATTransport and githubAppTransport.RoundTrip to
retain and use the configured apiBase when applying Authorization. Compare each
request’s destination host with the API base host, attach the bearer token only
for matching hosts, and leave the header unset for cross-host redirects.
| func (s *LandscapeKubernetesSource) fetchGithubFile(ctx context.Context, ref string) ([]byte, error) { | ||
| url := s.fileURL | ||
| if ref != "" { | ||
| url += "?ref=" + ref | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating request: %w", err) | ||
| } | ||
| req.Header.Set("Accept", "application/vnd.github.raw") | ||
|
|
||
| resp, err := s.githubClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("executing request: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("can't read body, github API returned %d: %w", resp.StatusCode, err) | ||
| } | ||
| return nil, fmt.Errorf("github API returned %d: %s", resp.StatusCode, body) | ||
| } | ||
|
|
||
| return io.ReadAll(resp.Body) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set an HTTP client timeout and escape the ref query value.
Two issues in this request path:
NewLandscapeKubernetesSourcebuilds&http.Client{Transport: gh.Transport}with noTimeout.exchangeInstallationTokenat line 420 callsbase.RoundTripdirectly, which also has no timeout. Cancellation depends entirely on the caller's context. If the reconcile context carries no deadline, a stalled GitHub endpoint blocks the worker goroutine indefinitely.- Line 281 concatenates
refinto the query without escaping.refcomes fromLatestTag, so a registry tag that contains&,#, or a space produces a malformed request URL.
🛡️ Proposed fix
url := s.fileURL
if ref != "" {
- url += "?ref=" + ref
+ url += "?" + neturl.Values{"ref": {ref}}.Encode()
} return &LandscapeKubernetesSource{
ociRepo: repo,
- githubClient: &http.Client{Transport: gh.Transport},
+ githubClient: &http.Client{Transport: gh.Transport, Timeout: 30 * time.Second},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go` around
lines 278 - 304, Update NewLandscapeKubernetesSource and
exchangeInstallationToken to ensure GitHub HTTP requests use a finite client
timeout, including requests sent through base.RoundTrip. In fetchGithubFile,
build the ref query parameter with standard URL query encoding instead of
concatenating it, preserving valid requests for tags containing &, #, or spaces.
| for _, shoot := range shootList.Items { | ||
| if shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName { | ||
| continue | ||
| } | ||
|
|
||
| for _, worker := range shoot.Spec.Provider.Workers { | ||
| if worker.Machine.Image == nil || worker.Machine.Image.Name != imageName { | ||
| continue | ||
| } | ||
| if worker.Machine.Image.Version != nil { | ||
| referenced[*worker.Machine.Image.Version] = struct{}{} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether ShootSpec still exposes CloudProfileName and how it is defaulted.
set -euo pipefail
go env GOMODCACHE >/dev/null 2>&1 && rg -n 'gardener/gardener ' go.mod
rg -n -C 3 'CloudProfileName|CloudProfile \*CloudProfileReference' \
--glob '**/gardener/pkg/apis/core/v1beta1/types_shoot.go' . || \
echo "vendored gardener types not present in the sandbox"Repository: cobaltcore-dev/cloud-profile-sync
Length of output: 171
🌐 Web query:
Gardener Shoot spec.cloudProfileName deprecated vs spec.cloudProfile 1.145
💡 Result:
In Gardener, the spec.cloudProfileName field in the Shoot resource has been deprecated and replaced by the spec.cloudProfile field [1][2][3]. This change is part of an evolution in how Gardener handles cloud profile references, moving toward a more robust structure that supports both standard CloudProfiles and NamespacedCloudProfiles [4][5][6]. Key details regarding this migration include: Deprecation and Timeline: The spec.cloudProfileName field is deprecated and its usage has been systematically restricted based on the Shoot's Kubernetes version [1][7][2]. Starting with Kubernetes v1.33, Gardener began issuing API warnings for Shoots using the legacy spec.cloudProfileName field [1][7]. Starting with Kubernetes v1.34, setting the spec.cloudProfileName field is forbidden [1][8][9]. Existing Shoots may have this field dropped by the system [1][9]. Migration: Users are advised to migrate to the new spec.cloudProfile field [1][10]. This field is a reference type (CloudProfileReference) that allows specifying the name of the desired CloudProfile, offering a more flexible and future-proof configuration [3][11][12]. For example, a configuration that previously used: spec: cloudProfileName: my-profile Should be updated to use: spec: cloudProfile: name: my-profile This structure also enables the use of NamespacedCloudProfiles, which allow for project-level customization of CloudProfiles, further enhancing the flexibility of environment definitions within Gardener [5][6].
Citations:
- 1: https://github.com/gardener/gardener/releases/tag/v1.124.0
- 2: metal-stack/gardener@48f2e5d
- 3: https://github.com/gardener/gardener/blob/master/pkg/apis/core/v1beta1/types_shoot.go
- 4: ☂️ [GEP-25] Introduce
NamespacedCloudProfiles gardener/gardener#9504 - 5: https://github.com/gardener/gardener/blob/master/docs/concepts/apiserver.md
- 6: https://gardener.cloud/docs/gardener/project/namespaced-cloud-profiles/
- 7: gardener/gardener@48f2e5d
- 8: metal-stack/gardener@7991776
- 9: https://gardener.cloud/community/review-meetings/2025-reviews/
- 10: https://gardener.cloud/docs/gardener/shoot/shoot_kubernetes_versions/
- 11: https://github.com/gardener/gardener/blob/master/docs/api-reference/core.md
- 12: https://github.com/gardener/gardener/blob/master/example/90-shoot.yaml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- module references ---'
rg -n 'gardener|CloudProfileName|CloudProfileReference|NamespacedCloudProfile' \
go.mod go.sum controllers . --glob '!vendor/**' --glob '!*.sum' 2>/dev/null | head -250 || true
printf '%s\n' '--- target function context ---'
rg -n -C 20 'referenced\[|CloudProfileName|CloudProfile\.Name|NamespacedCloudProfile' controllers/garbage_collection.go
printf '%s\n' '--- repository files related to Gardener types and tests ---'
git ls-files | rg '(^|/)(go\.mod|go\.sum|vendor/.*gardener|.*garbage.*collection.*|.*cloud.*profile.*)' | head -250Repository: cobaltcore-dev/cloud-profile-sync
Length of output: 34817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
modcache="$(go env GOMODCACHE)"
module_dir="$modcache/github.com/gardener/gardener/pkg/apis@v1.145.0"
printf '%s\n' '--- module path ---'
printf '%s\n' "$module_dir"
printf '%s\n' '--- ShootSpec and cloud-profile reference definitions ---'
rg -n -C 8 'type ShootSpec struct|CloudProfileName|CloudProfileReference|NamespacedCloudProfile|CloudProfile' \
"$module_dir/core/v1beta1" \
--glob '*.go' | head -300
printf '%s\n' '--- relevant API comments and conversion/defaulting code ---'
rg -n -C 10 'CloudProfileName|CloudProfileReference|NamespacedCloudProfile' \
"$module_dir/core/v1beta1" \
--glob '*.go' | head -400Repository: cobaltcore-dev/cloud-profile-sync
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
module_dir="$(go env GOMODCACHE)/github.com/gardener/gardener/pkg/apis@v1.145.0"
printf '%s\n' '--- ShootSpec definition ---'
shoot_file="$module_dir/core/v1beta1/types_shoot.go"
rg -n -A 140 '^type ShootSpec struct' "$shoot_file" | head -170
printf '%s\n' '--- legacy field presence in v1.145.0 ---'
rg -n -C 3 'CloudProfileName|cloudProfileName' "$module_dir/core/v1beta1" --glob '*.go' || true
printf '%s\n' '--- reference kinds and NCP parent semantics ---'
sed -n '40,100p' "$module_dir/core/v1beta1/types_namespacedcloudprofile.go"
rg -n -C 8 'NamespacedCloudProfile|CloudProfileReference|Parent\.Name|Kind.*NamespacedCloudProfile' \
"$module_dir" --glob '*.go' | head -300Repository: cobaltcore-dev/cloud-profile-sync
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("controllers/garbage_collection.go").read_text()
api = Path.home() / "go/pkg/mod/github.com/gardener/gardener/pkg/apis@v1.145.0/core/v1beta1"
assert "shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName" in source
shoot_spec = (api / "types_shoot.go").read_text()
ncp_spec = (api / "types_namespacedcloudprofile.go").read_text()
assert "CloudProfileName *string" in shoot_spec
assert 'CloudProfile *CloudProfileReference' in shoot_spec
assert "Parent CloudProfileReference" in ncp_spec
def current_predicate(cloud_profile, cloud_profile_name):
return cloud_profile is not None and cloud_profile["name"] == cloud_profile_name
cases = {
"legacy-only Shoot": (None, "base-profile"),
"direct CloudProfile reference": ({"kind": "CloudProfile", "name": "base-profile"}, "base-profile"),
"NamespacedCloudProfile reference": ({"kind": "NamespacedCloudProfile", "name": "project-profile"}, "base-profile"),
}
for label, (reference, target) in cases.items():
print(f"{label}: {'matches' if current_predicate(reference, target) else 'skipped'}")
print("API fields: CloudProfileName, CloudProfileReference, NamespacedCloudProfile.Parent")
PYRepository: cobaltcore-dev/cloud-profile-sync
Length of output: 362
Preserve all effective CloudProfile references during garbage collection
When a Shoot has only spec.cloudProfileName, this check skips it. When a Shoot references a NamespacedCloudProfile, this check also skips it because it compares the namespaced profile name with the parent CloudProfile name. Resolve both reference forms and follow NamespacedCloudProfile.spec.parent before deleting versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/garbage_collection.go` around lines 205 - 218, Update the Shoot
filtering logic in the garbage-collection loop to resolve both
spec.cloudProfileName and NamespacedCloudProfile references, including following
NamespacedCloudProfile.spec.parent to the effective parent CloudProfile. Match
the resolved parent against cloudProfileName before collecting worker image
versions in referenced, preserving all applicable references before deletion.
| func keppelURL(baseURL, repository string) (string, error) { | ||
| account, repo, err := splitKeppelRepository(repository) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| keppelURL := fmt.Sprintf( | ||
| "%s/keppel/v1/accounts/%s/repositories/%s/_manifests", | ||
| baseURL, | ||
| account, | ||
| repo, | ||
| ) | ||
|
|
||
| return keppelURL, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape the path segments when you build the Keppel URL.
keppelURL inserts account and repo into the URL with fmt.Sprintf and no escaping. Both values come from spec.machineImageUpdates[].source.oci.repository, which a ManagedCloudProfile author controls. A value such as acct/../../v1/other changes the request path, and a value containing ? or # changes the query or fragment.
Build the URL with url.URL.JoinPath, which escapes each segment.
🛡️ Proposed fix
- keppelURL := fmt.Sprintf(
- "%s/keppel/v1/accounts/%s/repositories/%s/_manifests",
- baseURL,
- account,
- repo,
- )
-
- return keppelURL, nil
+ u, err := url.Parse(baseURL)
+ if err != nil {
+ return "", fmt.Errorf("invalid registry base URL %q: %w", baseURL, err)
+ }
+ // repo may itself contain "/" separated segments; split so each one is escaped.
+ segments := append([]string{"keppel", "v1", "accounts", account, "repositories"},
+ strings.Split(repo, "/")...)
+ segments = append(segments, "_manifests")
+
+ return u.JoinPath(segments...).String(), nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/garbage_collection.go` around lines 311 - 325, Update keppelURL
to construct the endpoint with url.URL.JoinPath instead of fmt.Sprintf, ensuring
the base URL, account, repo, and "_manifests" components are joined with account
and repo safely escaped as path segments while preserving the existing
splitKeppelRepository error handling and return contract.
| kubernetesVersionUpdateConfig: | ||
| description: KubernetesVersionUpdateConfig contains the source and | ||
| provider information to automate Kubernetes version updates. | ||
| properties: | ||
| expirationThreshold: | ||
| description: |- | ||
| ExpirationThreshold defines the threshold for expiring Kubernetes versions. | ||
| Versions that are expiring within this threshold will be removed from the CloudProfile. | ||
| type: string |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject a negative expirationThreshold.
KubernetesImageUpdater.Update computes deleteThreshold := time.Now().Add(-ku.ExpirationThreshold). If expirationThreshold is negative, the cutoff moves into the future and the updater drops Kubernetes versions that have not expired yet. The sibling field garbageCollection.maxAge (lines 595-602) already guards this case with a CEL rule; this field does not.
Add the same marker to KubernetesVersionUpdateConfig.ExpirationThreshold in api/v1alpha1/managedcloudprofile.go and regenerate the CRD.
🛡️ Proposed marker on the Go type
// +optional
+ // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('0s')",message="expirationThreshold must not be negative"
ExpirationThreshold metav1.Duration `json:"expirationThreshold,omitempty"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml` around lines
604 - 612, Update KubernetesVersionUpdateConfig.ExpirationThreshold in
api/v1alpha1/managedcloudprofile.go to include the existing non-negative
validation marker used by garbageCollection.maxAge, then regenerate the CRD so
the expirationThreshold schema contains the corresponding CEL rule.
| github.com/felixge/httpsnoop v1.0.4 // indirect | ||
| github.com/fsnotify/fsnotify v1.10.1 // indirect | ||
| github.com/fxamacker/cbor/v2 v2.9.2 // indirect | ||
| github.com/gardener/gardener-extension-provider-openstack v1.57.0 // indirect |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the OpenStack and Gophercloud modules to the direct require block.
Both modules are imported by first-party production packages, but both carry the // indirect marker:
github.com/gardener/gardener-extension-provider-openstackis imported bycloudprofilesync/ossync/provider/openstack/provider.go.github.com/gophercloud/gophercloud/v2is imported bycloudprofilesync/ossync/source/glance/os_source.go.
go mod tidy promotes such modules into the direct require block and removes the marker. The current state indicates that go mod tidy was not re-run after these packages were added. A CI tidiness check will fail on this.
Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 45, Update the go.mod require declarations for
github.com/gardener/gardener-extension-provider-openstack and
github.com/gophercloud/gophercloud/v2 to remove the indirect markers and place
both modules in the direct require block, then run go mod tidy to ensure the
module file is consistent.
Add machine-image discovery for OpenStack CloudProfiles: - Glance source: discover public gardenlinux images across regions, parse versions from image names, keep the newest N (default 3), skip _usi variants. - OpenStackProvider: write per-region image UUIDs into the gardener-extension-provider-openstack providerConfig. - Lifecycle: mark the oldest kept version deprecated and stamp its expirationDate once on the transition, preserving it thereafter (ImageUpdater.resolveExpiration). - Wire GlanceSource into the ManagedCloudProfile API and controller; regenerate CRD and deepcopy. - Unit tests for expiration, usi skipping, and provider config.
1bb4416 to
7ef8a49
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/managedcloudprofile.go (1)
170-176: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce exactly one configuration alternative.
The API schema does not enforce mutual exclusion. The controller selects the first configured alternative:
OCI,IroncoreMetal, orPersonalAccessTokenSecret.Reject manifests that set both or neither alternative for each pair:
ociandglanceironcoreMetalandopenStackpersonalAccessTokenSecretandgithubAppAdd CEL or admission validation, then regenerate and test the CRD schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/managedcloudprofile.go` around lines 170 - 176, Add schema-level CEL or admission validation for api/v1alpha1/managedcloudprofile.go:170-176, 222-228, and 148-155 so each alternative pair requires exactly one configured field: oci versus glance, ironcoreMetal versus openStack, and personalAccessTokenSecret versus githubApp. Regenerate the CRD schema and add tests covering both-neither and both-set invalid manifests, while preserving acceptance of exactly one alternative.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/v1alpha1/managedcloudprofile.go`:
- Around line 170-176: Add schema-level CEL or admission validation for
api/v1alpha1/managedcloudprofile.go:170-176, 222-228, and 148-155 so each
alternative pair requires exactly one configured field: oci versus glance,
ironcoreMetal versus openStack, and personalAccessTokenSecret versus githubApp.
Regenerate the CRD schema and add tests covering both-neither and both-set
invalid manifests, while preserving acceptance of exactly one alternative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f896cb59-9b72-4794-9348-16ebbb4256b6
📒 Files selected for processing (4)
api/v1alpha1/managedcloudprofile.gocloudprofilesync/ossync/source/glance/os_source_test.gocontrollers/cloud_profile.gocrd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- controllers/cloud_profile.go
- cloudprofilesync/ossync/source/glance/os_source_test.go
- crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml
1 participant
Summary by CodeRabbit
ManagedCloudProfileschema with the new configuration options.