feat: automate Kubernetes version lifecycle management in CloudProfiles - #43
Merged
valeryia-hurynovich merged 8 commits intoAug 11, 2026
Conversation
📝 WalkthroughWalkthroughThe change adds Kubernetes version update configuration, shared OSSync contracts, OCI and Landscape sources, GitHub authentication, CloudProfile reconciliation, and OCI image garbage collection. ChangesCloudProfile API and schema contracts
Shared OSSync and OCI source contracts
Landscape Kubernetes version source
Controller reconciliation and garbage collection
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedCloudProfileController
participant KubernetesVersionUpdater
participant LandscapeKubernetesSource
participant OCIRepository
participant GitHubAPI
ManagedCloudProfileController->>KubernetesVersionUpdater: Update CloudProfile versions
KubernetesVersionUpdater->>LandscapeKubernetesSource: FetchVersions(ctx)
LandscapeKubernetesSource->>OCIRepository: Read latest tag and component descriptor
LandscapeKubernetesSource->>GitHubAPI: Read provider classifications
LandscapeKubernetesSource-->>KubernetesVersionUpdater: Return intersected versions
KubernetesVersionUpdater-->>ManagedCloudProfileController: Update CloudProfileSpec
Possibly related PRs
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: 13
🧹 Nitpick comments (9)
cloudprofilesync/ossync/source/oci/os_source_test.go (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
_usitoSupportInPlaceUpdatemapping.This fixture includes
_usi, but it does not assertversions[0].SupportInPlaceUpdate. Add a true assertion here and a valid-feature fixture without_usithat asserts false. This covers the new source-to-updater contract.Proposed test addition
Expect(versions[0].Capabilities).To(Equal(gardencorev1beta1.Capabilities{ "architecture": {"amd64"}, "feature": {"sci", "_usi"}, })) +Expect(versions[0].SupportInPlaceUpdate).To(BeTrue())🤖 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/ossync/source/oci/os_source_test.go` around lines 104 - 108, Extend the OCI fixture tests around NewOCI to assert that a source entry containing _usi maps versions[0].SupportInPlaceUpdate to true. Add a separate valid-feature fixture without _usi and assert the same field is false, covering both sides of the source-to-updater contract.controllers/cloud_profile.go (1)
38-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving the network calls out of the
CreateOrPatchmutate function.
updateMachineImagescontacts an OCI registry.updateKubernetesVersionscontacts an OCI registry and the GitHub API. Both run inside the mutate closure passed tocontrollerutil.CreateOrPatch. Two consequences follow:
- The closure is not guaranteed to run exactly once. Any future conflict-retry wrapper around
CreateOrPatchrepeats every remote call.- A slow registry or a slow GitHub endpoint holds the closure open while the CloudProfile object is staged for patching, which lengthens the window for a conflicting write.
Resolve the source versions before the
CreateOrPatchcall, then apply the resolved values inside the closure. This also makes the closure pure and easier to test.This is a structural change. Defer it if the current behaviour is acceptable.
🤖 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 38 - 58, The CreateOrPatch mutate closure currently performs remote calls through updateMachineImages and updateKubernetesVersions. Resolve all machine-image and Kubernetes-version updates before invoking controllerutil.CreateOrPatch, then apply those precomputed values inside the closure while preserving error propagation and existing defaults.controllers/garbage_collection.go (3)
192-195: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry the CloudProfile update on conflict.
deleteVersionsperforms aGetat line 116 and anUpdateat line 192.reconcileCloudProfilepatched the same CloudProfile moments earlier in the same reconcile, and the informer cache may still serve the pre-patchresourceVersion. TheUpdatethen returns aConflicterror.Line 104 handles only
apierrors.IsInvalid, so a conflict propagates up, setsFailedReconcileStatus, and returns an error. The ManagedCloudProfile reports a failure for a transient and expected condition.Wrap the read-modify-write in
retry.RetryOnConflict.♻️ Proposed refactor
+import "k8s.io/client-go/util/retry"- if err := r.Update(ctx, &cp); err != nil { - return err - } - return nil + return r.Update(ctx, &cp)Then wrap the whole
Get-mutate-Updatebody ofdeleteVersionsinretry.RetryOnConflict(retry.DefaultRetry, func() error { ... })so that the retry re-reads the 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 192 - 195, Wrap the entire Get-mutate-Update flow in deleteVersions with retry.RetryOnConflict using retry.DefaultRetry, re-reading the CloudProfile on each attempt before applying mutations and calling Update. Return the retry result while preserving the existing invalid-error handling and successful nil result.
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit registry type instead of hostname substring matching.
getRegistryProviderselects the Keppel client when the lowercased registry host containskeppel. A Keppel deployment on a host without that substring falls through toerrors.New("no registry provider found for registry"), and garbage collection then fails for a valid configuration.An explicit
registryTypefield on the OCI source in the API removes the guess. This code moved unchanged during the file split, so treat it as a follow-up rather than a blocker for this PR.🤖 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 getRegistryProvider to select the provider from an explicit registryType field on the OCI source rather than matching “keppel” in the hostname. Propagate the registry type through the caller and return KeppelClient when the configured type is Keppel, while preserving validation for empty or unsupported types.
88-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffThe referenced-version snapshot and the CloudProfile update are not atomic.
getReferencedVersionslists Shoots, thendeleteVersionsupdates the CloudProfile. A Shoot created between the two steps can reference a version that this pass removes. The removal does not delete the registry image, so the effect is a Shoot that references a version absent from its CloudProfile. Gardener then fails to reconcile that Shoot.Two mitigations are available:
- Add a grace period so that only versions older than
MaxAgeplus a buffer are eligible, which shrinks the window.- Re-list the Shoots immediately before the
Updateand abort when the referenced set grew.The current 5-minute requeue does not close the gap, because the next pass makes the same decision.
🤖 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 88 - 101, Make the garbage-collection decision safe against Shoots created after the initial getReferencedVersions snapshot: before updating the CloudProfile, re-list the Shoots and compare the newly referenced set with the original, aborting the deletion/update when it has grown; retain the existing deletion flow only when no new references are detected. Alternatively, enforce a MaxAge grace buffer when selecting versions in the versionsToDelete loop.cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go (2)
145-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the JWT assertions to cover the signature and the claims.
The test only counts three dot-separated segments. It passes even if the signature is invalid,
issis wrong, orexpis missing. GitHub rejects all three cases at runtime, so the test gives little protection for the App authentication path.💚 Proposed fix to verify the signature and the claims
jwt, err := tr.mintJWT() if err != nil { t.Fatalf("unexpected error: %v", err) } - if parts := strings.Split(jwt, "."); len(parts) != 3 { - t.Fatalf("expected 3 JWT parts, got %d", len(parts)) - } + parts := strings.Split(jwt, ".") + if len(parts) != 3 { + t.Fatalf("expected 3 JWT parts, got %d", len(parts)) + } + + sig, err := base64.RawURLEncoding.DecodeString(parts[2]) + if err != nil { + t.Fatalf("decoding signature: %v", err) + } + digest := sha256.Sum256([]byte(parts[0] + "." + parts[1])) + if err := rsa.VerifyPKCS1v15(&key.PublicKey, crypto.SHA256, digest[:], sig); err != nil { + t.Fatalf("signature verification failed: %v", err) + } + + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + t.Fatalf("decoding payload: %v", err) + } + var claims struct { + Iat int64 `json:"iat"` + Exp int64 `json:"exp"` + Iss int64 `json:"iss"` + } + if err := json.Unmarshal(payload, &claims); err != nil { + t.Fatalf("decoding claims: %v", err) + } + if claims.Iss != 42 { + t.Errorf("expected iss 42, got %d", claims.Iss) + } + if claims.Exp <= claims.Iat { + t.Errorf("expected exp %d to be after iat %d", claims.Exp, claims.Iat) + }Add
"crypto","crypto/sha256", and"encoding/base64"to the imports.🤖 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, Strengthen TestGithubAppTransport_MintJWT by parsing the JWT and verifying its signature with the generated key, using SHA-256 and base64url decoding as needed. Assert that the claims include the expected app ID in iss and a valid exp value, while preserving the existing error and three-part checks.
208-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the PKCS8 branches.
parseRSAPrivateKeyhandles aPRIVATE KEYblock and rejects a non-RSA PKCS8 key. Neither branch is tested. Operators commonly convert a GitHub App key to PKCS8, so this path runs in production.💚 Proposed fix to add PKCS8 cases
t.Run("errors on unsupported PEM type", func(t *testing.T) { b := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: []byte("x")}) if _, err := parseRSAPrivateKey(b); err == nil || !strings.Contains(err.Error(), "unsupported") { t.Fatalf("expected unsupported error, got %v", err) } }) + t.Run("parses PKCS8 PEM", func(t *testing.T) { + key, _ := generateTestKey(t) + der, err := x509.MarshalPKCS8PrivateKey(key) + if err != nil { + t.Fatalf("marshalling PKCS8: %v", err) + } + b := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + if _, err := parseRSAPrivateKey(b); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + t.Run("errors on a non-RSA PKCS8 key", func(t *testing.T) { + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generating ed25519 key: %v", err) + } + der, err := x509.MarshalPKCS8PrivateKey(priv) + if err != nil { + t.Fatalf("marshalling PKCS8: %v", err) + } + b := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + if _, err := parseRSAPrivateKey(b); err == nil || !strings.Contains(err.Error(), "not RSA") { + t.Fatalf("expected not-RSA error, got %v", err) + } + })Add
"crypto/ed25519"to the imports.🤖 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 208 - 226, Extend TestParseRSAPrivateKey with PKCS8 coverage: generate a valid RSA private key encoded in a PRIVATE KEY PEM block and assert parseRSAPrivateKey succeeds, then generate an ed25519 key, encode it as PKCS8 in the same PEM type, and assert parsing fails with the expected non-RSA error. Add the crypto/ed25519 import needed for the unsupported-key case.controllers/managedcloudprofile_controller_test.go (1)
163-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a provider-config variant and set
Type.Two small improvements apply to this builder:
- Four fixtures wrap the builder in an immediately invoked function only to attach
ProviderConfig: lines 933-945, 1034-1047, 1137-1149, and 1227-1238. A second helper removes that repetition.- The doc comment states "a minimal valid CloudProfileSpec", but
Typeis never set.CloudProfileSpec.Typecarries no+optionalmarker, so the generated CloudProfile hastype: "". Setting a value makes the fixtures represent a CloudProfile that real Gardener admission accepts.♻️ Proposed refactor
func baseCloudProfileSpec(machineImages ...gardenerv1beta1.MachineImage) v1alpha1.CloudProfileSpec { amd64 := "amd64" usable := true spec := v1alpha1.CloudProfileSpec{ + Type: "ironcore-metal", Regions: []gardenerv1beta1.Region{ { Name: "foo", }, },// baseCloudProfileSpecWithProviderConfig returns baseCloudProfileSpec with the // given raw provider configuration attached. func baseCloudProfileSpecWithProviderConfig(raw []byte, machineImages ...gardenerv1beta1.MachineImage) v1alpha1.CloudProfileSpec { spec := baseCloudProfileSpec(machineImages...) spec.ProviderConfig = &runtime.RawExtension{Raw: raw} return spec }Then each fixture simplifies, for example at lines 933-945:
- CloudProfile: func() v1alpha1.CloudProfileSpec { - cp := baseCloudProfileSpec( - gardenerv1beta1.MachineImage{ - Name: "cap-image", - Versions: []gardenerv1beta1.MachineImageVersion{ - {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: rawTag}, Architectures: []string{"amd64"}}, - {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: cleanVersion}, Architectures: []string{"amd64"}}, - }, - }, - ) - cp.ProviderConfig = &runtime.RawExtension{Raw: raw} - return cp - }(), + CloudProfile: baseCloudProfileSpecWithProviderConfig(raw, + gardenerv1beta1.MachineImage{ + Name: "cap-image", + Versions: []gardenerv1beta1.MachineImageVersion{ + {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: rawTag}, Architectures: []string{"amd64"}}, + {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: cleanVersion}, Architectures: []string{"amd64"}}, + }, + }, + ),Setting
Typemay require updating the assertion at line 273, which compares the full spec.🤖 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 163 - 188, Update baseCloudProfileSpec to set CloudProfileSpec.Type to a valid provider type so generated fixtures satisfy Gardener admission, and adjust the full-spec assertion near the existing base builder tests. Add a baseCloudProfileSpecWithProviderConfig helper that accepts raw provider configuration, delegates to baseCloudProfileSpec, and attaches it via ProviderConfig; replace the four immediately invoked fixture builders with this helper.cloudprofilesync/kubernetessync/source/landscape/landscape_source.go (1)
401-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a maintained JWT library instead of hand-rolling RS256 signing.
github.com/golang-jwt/jwt/v5supportsSignMethodRS256for GitHub App tokens and avoids manual base64 encoding, SHA-256 signing, andmustJSONfailures unlessjwt.ParseRSAPrivateKeyFromPEMalready rejects the private key. Use the current implementation if keeping dependencies off this path is a hard preference.🤖 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 401 - 422, Replace the hand-rolled JWT construction in githubAppTransport.mintJWT with github.com/golang-jwt/jwt/v5, using SigningMethodRS256 and claims for iat, exp, and iss. Sign the token with t.key and preserve the existing error-wrapping and token validity timings; remove the manual header, payload, hashing, and base64-signing logic.
🤖 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 `@cloudprofilesync/kubernetessync/kubernetes_image_updater.go`:
- Around line 40-44: The expiration cutoff in kubernetes_image_updater.go lines
40-44 must use now plus ku.ExpirationThreshold so versions expiring within the
positive threshold are removed; add tests covering that case and negative
thresholds. In api/v1alpha1/managedcloudprofile.go lines 116-120, add
Kubebuilder validation rejecting durations below 0s. Regenerate
crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml lines 608-612 so
its schema enforces duration(self) >= duration('0s').
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go`:
- Around line 230-306: Extract the version-intersection loop from FetchVersions
into a pure intersectVersions helper and have FetchVersions call it; update the
test to cover that helper directly with ExpirableVersion values, removing the
unused githubSrv and abandoned comments. Split the ref query assertion into a
focused TestFetchGithubFileAppendsRef test around fetchClassification,
preserving the expected ref=v1.2.3 behavior.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 299-305: Limit response-body reads to 2048 bytes in both
fetchGithubFile
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:299-305)
and exchangeInstallationToken
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:439-445)
by wrapping each body with io.LimitReader before reading or decoding; preserve
JSON decoding on the limited reader in exchangeInstallationToken and include the
bounded content in errors.
- Around line 363-399: Add a mutex field to githubAppTransport and use it in
installationToken to guard the cached token check, token exchange, and
cached/expiresAt writes as one critical section. Ensure concurrent callers
cannot race or perform duplicate exchanges, while leaving JWT creation and
existing error behavior unchanged.
- Around line 126-131: Set a finite request timeout on the http.Client assigned
to githubClient in the LandscapeKubernetesSource constructor, rather than
leaving Timeout at its zero value. Choose the project’s established timeout
configuration or an appropriate bounded duration so FetchVersions cannot block
reconciliation indefinitely.
- Around line 183-191: Update the tag-selection logic around slices.MaxFunc to
first filter out every tag rejected by semver.ParseTolerant, then compute the
maximum using only parseable semantic versions and their semver comparison.
Remove the mixed string-comparison fallback, while preserving the existing
result handling when no valid tags remain.
- Around line 320-327: Update the expirable-version mapping loop to set
Classification to nil when v.Classification is empty, while retaining a pointer
to the value for non-empty classifications. Use k8s.io/utils/ptr to construct
the conditional pointer and preserve the existing Version and ExpirationDate
mappings.
In `@controllers/cloud_profile.go`:
- Around line 114-122: Validate that the provider configuration in the update
request is present before proceeding; when update.Provider.IroncoreMetal is nil,
return an explicit configuration error instead of leaving provider nil. Update
the provider-selection logic around ossync.Provider and preserve the existing
IroncoreProvider construction for valid configurations.
- Around line 75-86: Update reconcileCloudProfile to call
patchStatusAndCondition unconditionally after successful reconciliation, rather
than only when op != controllerutil.OperationResultNone, so unchanged
CloudProfiles recover stale Failed status. Preserve the existing success status
and CloudProfileApplied condition values, and verify patchStatusAndCondition
remains idempotent without updating LastTransitionTime or writing status when
nothing changed.
In `@controllers/garbage_collection.go`:
- Around line 198-204: Refactor reconcileGarbageCollection and
getReferencedVersions so the reconcile fetches the ShootList and CloudProfile
once before iterating mcp.Spec.MachineImageUpdates, then passes those snapshots
into getReferencedVersions for per-image filtering. Remove the duplicate
List/Get operations from getReferencedVersions, preserve its referenced-version
results, and verify the manager’s cached-client configuration intentionally
supports the all-Shoot snapshot.
- Around line 256-262: Extend RegistryClient.GetTags and its implementations to
accept the OCI connection parameters, then update reconcileGarbageCollection and
fetchKeppelTags to use them instead of hardcoding secure transport. Pass the
same oci.Params as updateMachineImages, including the password resolved by
getCredential, so insecure registries and authenticated Keppel requests work
consistently; update fake RegistryClient implementations and tests to match the
new signature.
- Around line 177-190: Pass the Shoot-referenced versions set from
reconcileGarbageCollection into deleteVersions, then update deleteVersions so
its cascade-delete predicate preserves any version present in that set before
removing clean versions with no remaining flavors. Add coverage for a
Shoot-referenced clean version whose provider config entry has no capability
flavors.
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 33-73: Add a KubernetesVersionSourceFactory field to Reconciler,
mirroring OCISourceFactory, and update updateKubernetesVersions to obtain its
source through that injectable factory instead of calling landscapeSetupSource
directly. Provide fake and configurable test sources, then add coverage for
writing Kubernetes versions, expiration filtering, missing source configuration,
missing GitHub credentials, and PAT/GitHub App credential resolution. Keep the
existing KubernetesVersionUpdateConfig-absent behavior unchanged.
---
Nitpick comments:
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go`:
- Around line 145-156: Strengthen TestGithubAppTransport_MintJWT by parsing the
JWT and verifying its signature with the generated key, using SHA-256 and
base64url decoding as needed. Assert that the claims include the expected app ID
in iss and a valid exp value, while preserving the existing error and three-part
checks.
- Around line 208-226: Extend TestParseRSAPrivateKey with PKCS8 coverage:
generate a valid RSA private key encoded in a PRIVATE KEY PEM block and assert
parseRSAPrivateKey succeeds, then generate an ed25519 key, encode it as PKCS8 in
the same PEM type, and assert parsing fails with the expected non-RSA error. Add
the crypto/ed25519 import needed for the unsupported-key case.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 401-422: Replace the hand-rolled JWT construction in
githubAppTransport.mintJWT with github.com/golang-jwt/jwt/v5, using
SigningMethodRS256 and claims for iat, exp, and iss. Sign the token with t.key
and preserve the existing error-wrapping and token validity timings; remove the
manual header, payload, hashing, and base64-signing logic.
In `@cloudprofilesync/ossync/source/oci/os_source_test.go`:
- Around line 104-108: Extend the OCI fixture tests around NewOCI to assert that
a source entry containing _usi maps versions[0].SupportInPlaceUpdate to true.
Add a separate valid-feature fixture without _usi and assert the same field is
false, covering both sides of the source-to-updater contract.
In `@controllers/cloud_profile.go`:
- Around line 38-58: The CreateOrPatch mutate closure currently performs remote
calls through updateMachineImages and updateKubernetesVersions. Resolve all
machine-image and Kubernetes-version updates before invoking
controllerutil.CreateOrPatch, then apply those precomputed values inside the
closure while preserving error propagation and existing defaults.
In `@controllers/garbage_collection.go`:
- Around line 192-195: Wrap the entire Get-mutate-Update flow in deleteVersions
with retry.RetryOnConflict using retry.DefaultRetry, re-reading the CloudProfile
on each attempt before applying mutations and calling Update. Return the retry
result while preserving the existing invalid-error handling and successful nil
result.
- Around line 33-41: Update getRegistryProvider to select the provider from an
explicit registryType field on the OCI source rather than matching “keppel” in
the hostname. Propagate the registry type through the caller and return
KeppelClient when the configured type is Keppel, while preserving validation for
empty or unsupported types.
- Around line 88-101: Make the garbage-collection decision safe against Shoots
created after the initial getReferencedVersions snapshot: before updating the
CloudProfile, re-list the Shoots and compare the newly referenced set with the
original, aborting the deletion/update when it has grown; retain the existing
deletion flow only when no new references are detected. Alternatively, enforce a
MaxAge grace buffer when selecting versions in the versionsToDelete loop.
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 163-188: Update baseCloudProfileSpec to set CloudProfileSpec.Type
to a valid provider type so generated fixtures satisfy Gardener admission, and
adjust the full-spec assertion near the existing base builder tests. Add a
baseCloudProfileSpecWithProviderConfig helper that accepts raw provider
configuration, delegates to baseCloudProfileSpec, and attaches it via
ProviderConfig; replace the four immediately invoked fixture builders with this
helper.
🪄 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: 931d66fb-dadb-465f-8997-fedeaae76b1c
📒 Files selected for processing (19)
api/v1alpha1/managedcloudprofile.goapi/v1alpha1/zz_generated.deepcopy.gocloudprofilesync/kubernetessync/kubernetes_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/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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cloudprofilesync/k8ssync/source/landscape/landscape_source_test.go (3)
287-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
freePortto match its return value.The function returns a full
host:portaddress, not a port. Callers use it as"http://"+addr. Rename it tofreeAddrso the name matches the value.🤖 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/k8ssync/source/landscape/landscape_source_test.go` around lines 287 - 296, Rename the helper function freePort to freeAddr and update every caller to use the new name, preserving its existing host:port return value and behavior.
167-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concurrent subtest for the installation-token cache.
This test drives
RoundTripsequentially, so it cannot detect a data race. The source addedmutogithubAppTransportto guardcachedandexpiresAt. A subtest that issues parallel requests through one transport, run under-race, protects that guarantee against future changes.🤖 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/k8ssync/source/landscape/landscape_source_test.go` around lines 167 - 215, Extend TestGithubAppTransport_TokenCaching with a concurrent subtest that reuses one githubAppTransport and issues multiple parallel RoundTrip calls, coordinating completion and failures safely. Run enough requests to verify the installation-token endpoint is called only once, while preserving the existing sequential cache assertion and ensuring the test is race-detector safe.
316-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe readiness loop can exit without failing the test.
The outer condition and the inner deadline check duplicate the same timeout. If
time.Now().Before(deadline)becomes false at the top of an iteration, the loop exits,startRegistryreturns the cleanup function, and no failure is reported. The doc comment on lines 298-300 states that the helper fails the test immediately when the registry is not ready. The tests then hit connection errors inFetchVersions, which hides the real cause. A single 500 ms budget is also short for a loaded CI runner.♻️ Proposed fix to make the readiness check deterministic
- deadline := time.Now().Add(500 * time.Millisecond) - for time.Now().Before(deadline) { + ready := false + deadline := time.Now().Add(5 * time.Second) + for !ready && time.Now().Before(deadline) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+addr, http.NoBody) if err != nil { cancel() t.Fatalf("building readiness request: %v", err) } resp, err := http.DefaultClient.Do(req) if err == nil { resp.Body.Close() - break + ready = true + break } time.Sleep(10 * time.Millisecond) - if time.Now().After(deadline) { - cancel() - t.Fatalf("registry on %s did not become ready within 500ms", addr) - } } + if !ready { + cancel() + t.Fatalf("registry on %s did not become ready in time", addr) + }🤖 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/k8ssync/source/landscape/landscape_source_test.go` around lines 316 - 333, Update the readiness check in startRegistry so exhausting the startup deadline always calls t.Fatalf instead of allowing the loop to return successfully. Use one timeout-controlled retry flow, preserve immediate failure for request-construction errors, and increase the readiness budget beyond 500 ms to accommodate loaded CI runners.cloudprofilesync/ocirepo/ocirepo.go (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail fast when only one credential is set.
Newapplies authentication only when bothUsernameandPasswordare non-empty. If an operator configures a username but omits the password, the repository silently falls back to anonymous access. The failure then surfaces later as a401or404from the registry, which hides the configuration mistake.♻️ Proposed fix to reject partial credentials
- if params.Username != "" && params.Password != "" { + switch { + case params.Username != "" && params.Password != "": repo.Client = &auth.Client{ Client: retry.DefaultClient, Cache: auth.NewCache(), Credential: auth.StaticCredential(params.Registry, auth.Credential{ Username: params.Username, Password: params.Password, }), } - } + case params.Username != "" || params.Password != "": + return nil, errors.New("both username and password must be set for registry authentication") + }Add
"errors"to the imports.🤖 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/ocirepo/ocirepo.go` around lines 27 - 36, Update New to validate Username and Password together: return an errors.New configuration error when exactly one credential is provided, while preserving authenticated setup when both are present and anonymous access when both are empty. Add the errors import and ensure the validation occurs before constructing repo.Client.cloudprofilesync/k8ssync/k8s_image_updater_test.go (3)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "image" naming after the rename to
KubernetesVersionUpdater. The updater writesspec.kubernetes.versionsand handles no images. The rename reached the type and the constructor, but the filenames and the test function name still say "image".
cloudprofilesync/k8ssync/k8s_image_updater_test.go#L27-L27: renameTestKubernetesImageUpdater_UpdatetoTestKubernetesVersionUpdater_Update, and rename the file tokubernetes_version_updater_test.go.cloudprofilesync/k8ssync/k8s_image_updater.go#L20-L26: rename the file tokubernetes_version_updater.goso it matches theKubernetesVersionUpdatertype it declares.🤖 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/k8ssync/k8s_image_updater_test.go` at line 27, Rename cloudprofilesync/k8ssync/k8s_image_updater_test.go to kubernetes_version_updater_test.go and update TestKubernetesImageUpdater_Update to TestKubernetesVersionUpdater_Update. Also rename cloudprofilesync/k8ssync/k8s_image_updater.go to kubernetes_version_updater.go; no implementation changes are needed.
59-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis subtest duplicates "refuses to wipe CloudProfile" and does not verify dropping.
The source returns one expired version, so
Updatereturns an error and never assigns. The assertions are the same as the subtest at lines 125-141. The name promises drop behavior, but the drop path is only observable in the "mixed" subtest at lines 90-114. Add a surviving version so the subtest verifies the drop.♻️ Proposed fix to make the subtest verify dropping
t.Run("drops version expired beyond threshold", func(t *testing.T) { src := &fakeSource{versions: []gardenerv1beta1.ExpirableVersion{ {Version: "1.29.0", ExpirationDate: expiry(now.Add(-60 * 24 * time.Hour))}, + {Version: "1.31.0"}, }} ku := NewKubernetesVersionUpdater(src, 30*24*time.Hour) var spec gardenerv1beta1.CloudProfileSpec - spec.Kubernetes.Versions = []gardenerv1beta1.ExpirableVersion{{Version: "existing"}} - err := ku.Update(context.Background(), &spec) - if err == nil { - t.Fatal("expected error when all versions filtered, got nil") - } - // CloudProfile must not have been modified. - if len(spec.Kubernetes.Versions) != 1 || spec.Kubernetes.Versions[0].Version != "existing" { - t.Errorf("spec was modified despite error: %v", spec.Kubernetes.Versions) - } + if err := ku.Update(context.Background(), &spec); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(spec.Kubernetes.Versions) != 1 || spec.Kubernetes.Versions[0].Version != "1.31.0" { + t.Errorf("expected only 1.31.0 to survive, got %v", spec.Kubernetes.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 `@cloudprofilesync/k8ssync/k8s_image_updater_test.go` around lines 59 - 74, Update the “drops version expired beyond threshold” subtest to include both an expired and a non-expired source version, so Update succeeds while filtering out only the expired entry. Assert that the resulting spec contains the surviving version and excludes the expired one, rather than asserting an error and unchanged “existing” data.
116-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a subtest for a source that returns an empty version list.
fakeSourcewithversions: niland no error exercises a distinct branch.Updatethen returns the "after expiration filtering" error even though no filtering occurred. A test pins the current contract and shows whether the message needs to distinguish the two causes.🤖 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/k8ssync/k8s_image_updater_test.go` around lines 116 - 123, Extend the Update tests with a subtest for an empty successful source response using fakeSource configured with versions: nil and no error. Call ku.Update with an empty CloudProfileSpec, assert it returns an error, and verify the error matches the current “after expiration filtering” contract.
🤖 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 `@cloudprofilesync/k8ssync/source/landscape/landscape_source.go`:
- Around line 196-203: Update the tag parsing loop in the semver selection flow
to accept only complete semantic-version tags, rejecting partial numeric values
such as “2024” and “2024.11” even when semver.ParseTolerant returns no error.
Ensure rejected tags are excluded from parseable so the existing “no semver tags
found” error remains returned when no full semver tags exist, while valid
release tags continue to reach slices.MaxFunc and fetchComponentDescriptor.
In `@cloudprofilesync/ocirepo/ocirepo.go`:
- Around line 21-40: Update New so both authenticated and anonymous repository
requests use an explicit HTTP client with a finite Timeout. Configure the client
used by auth.Client instead of relying on retry.DefaultClient, and assign the
same timeout-bounded client to repo.Client when credentials are absent; preserve
the existing authentication and repository setup.
---
Nitpick comments:
In `@cloudprofilesync/k8ssync/k8s_image_updater_test.go`:
- Line 27: Rename cloudprofilesync/k8ssync/k8s_image_updater_test.go to
kubernetes_version_updater_test.go and update TestKubernetesImageUpdater_Update
to TestKubernetesVersionUpdater_Update. Also rename
cloudprofilesync/k8ssync/k8s_image_updater.go to kubernetes_version_updater.go;
no implementation changes are needed.
- Around line 59-74: Update the “drops version expired beyond threshold” subtest
to include both an expired and a non-expired source version, so Update succeeds
while filtering out only the expired entry. Assert that the resulting spec
contains the surviving version and excludes the expired one, rather than
asserting an error and unchanged “existing” data.
- Around line 116-123: Extend the Update tests with a subtest for an empty
successful source response using fakeSource configured with versions: nil and no
error. Call ku.Update with an empty CloudProfileSpec, assert it returns an
error, and verify the error matches the current “after expiration filtering”
contract.
In `@cloudprofilesync/k8ssync/source/landscape/landscape_source_test.go`:
- Around line 287-296: Rename the helper function freePort to freeAddr and
update every caller to use the new name, preserving its existing host:port
return value and behavior.
- Around line 167-215: Extend TestGithubAppTransport_TokenCaching with a
concurrent subtest that reuses one githubAppTransport and issues multiple
parallel RoundTrip calls, coordinating completion and failures safely. Run
enough requests to verify the installation-token endpoint is called only once,
while preserving the existing sequential cache assertion and ensuring the test
is race-detector safe.
- Around line 316-333: Update the readiness check in startRegistry so exhausting
the startup deadline always calls t.Fatalf instead of allowing the loop to
return successfully. Use one timeout-controlled retry flow, preserve immediate
failure for request-construction errors, and increase the readiness budget
beyond 500 ms to accommodate loaded CI runners.
In `@cloudprofilesync/ocirepo/ocirepo.go`:
- Around line 27-36: Update New to validate Username and Password together:
return an errors.New configuration error when exactly one credential is
provided, while preserving authenticated setup when both are present and
anonymous access when both are empty. Add the errors import and ensure the
validation occurs before constructing repo.Client.
🪄 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: 11251af1-5b3e-45f2-a4f9-f699add9f58b
📒 Files selected for processing (13)
api/v1alpha1/managedcloudprofile.gocloudprofilesync/k8ssync/k8s_image_updater.gocloudprofilesync/k8ssync/k8s_image_updater_test.gocloudprofilesync/k8ssync/source/landscape/landscape_source.gocloudprofilesync/k8ssync/source/landscape/landscape_source_test.gocloudprofilesync/ocirepo/ocirepo.gocloudprofilesync/ossync/source/oci/os_source.gocloudprofilesync/ossync/source/oci/os_source_test.gocontrollers/cloud_profile.gocontrollers/garbage_collection.gocontrollers/managedcloudprofile_controller.gocontrollers/managedcloudprofile_controller_test.gocrd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- api/v1alpha1/managedcloudprofile.go
- cloudprofilesync/ossync/source/oci/os_source_test.go
- crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml
- controllers/garbage_collection.go
- controllers/managedcloudprofile_controller.go
- controllers/cloud_profile.go
- controllers/managedcloudprofile_controller_test.go
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
adziauho
force-pushed
the
update-kuberentes-versions
branch
from
August 11, 2026 11:30
1228aad to
52043ec
Compare
valeryia-hurynovich
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements automated Kubernetes version lifecycle management in
cloud-profile-sync, extending the operator to manage Kubernetes versions in CloudProfiles in addition to the existing machine image management.Closes cc/unified-kubernetes#1174 (SAP internal)
Changes
kubernetessyncpackage —KubernetesImageUpdaterwritesspec.kubernetes.versionsto a CloudProfile, filtering out versions whose expiration date has passed the configured thresholdLandscapeKubernetesSource— fetches Kubernetes versions from two sources and merges them:kube-apiserverversionsproviders[].versions[]) for version classifications (supported/deprecated/expired) and expiration dates; supports both PAT and GitHub App (JWT/RSA) authenticationManagedCloudProfileSpecgainskubernetesVersionUpdateConfigwithexpirationThresholdandlandscapeSetup(OCI + GitHub config)managedcloudprofile_controller.gosplit into:cloud_profile.go— CloudProfile reconciliation and machine image / Kubernetes version update logicgarbage_collection.go— GC logic (no functional changes)cloudprofilesync/reorganized intoossync/andkubernetessync/subpackages for cleaner separation of concernsTest plan
KubernetesImageUpdater(expiration threshold filtering)LandscapeKubernetesSource(OCI + GitHub fetch, version merging)KubernetesVersionUpdateConfigSummary by CodeRabbit