OCPBUGS-88036: Set cluster ownership tag on AzureCluster#590
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@matzew: This pull request references Jira Issue OCPBUGS-88036, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Warning Review limit reached
More reviews will be available in 24 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesResource tag construction and wiring
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/controllers/infracluster/azure.go`:
- Around line 245-253: Currently user-provided tags from
r.Infra.Status.PlatformStatus.Azure.ResourceTags are merged into the tags map
after the ownership tag, allowing them to overwrite the ownership key; instead,
populate the tags map with user tags first (iterate ResourceTags and set
tags[t.Key] = t.Value) and only after that set the ownership entry using the
computed key fmt.Sprintf("kubernetes.io_cluster.%s",
r.Infra.Status.InfrastructureName) = "owned" so the ownership tag cannot be
overwritten (adjust code around the tags variable initialization and the loop
over ResourceTags).
- Around line 245-246: The AdditionalTags key generation in newAzureCluster uses
Infra.Status.InfrastructureName directly, which can produce invalid cluster-id
characters; update the newAzureCluster code path (the function that sets
AdditionalTags/ResourceTags) to sanitize/validate
Infra.Status.InfrastructureName to only allow alphanumeric, '-', '.', and '_'
(e.g., strip or replace invalid chars and/or return an error) before composing
fmt.Sprintf("kubernetes.io_cluster.%s", ...). Add a unit test under
pkg/controllers/infracluster (e.g., newazurecluster_test.go) that calls
newAzureCluster (or the tag-generation helper) with various InfrastructureName
inputs (valid and containing bad chars) and asserts the produced AdditionalTags
key equals kubernetes.io_cluster.<sanitized-cluster-id> and that invalid inputs
are handled as expected.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af6215e3-09b8-47ea-a0b6-b5b3618ccc46
📒 Files selected for processing (1)
pkg/controllers/infracluster/azure.go
| tags := azurev1.Tags{ | ||
| fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName): "owned", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering Azure cluster tagging and tag format validation
# Search for test files covering newAzureCluster
rg -n --type=go -A 5 -B 2 'newAzureCluster|AdditionalTags' --glob '*_test.go' --glob '**/infracluster/*'
# Search for Azure tag key format patterns in codebase
rg -n --type=go 'kubernetes\.io_cluster\.|kubernetes_io_cluster'
# Search for tag format documentation or validation
rg -n --type=go 'ResourceTags|AdditionalTags' pkg/controllers/infracluster/Repository: openshift/cluster-capi-operator
Length of output: 4207
Ensure Azure cluster tag key is compliant and covered by tests
pkg/controllers/infracluster/azure.gosetsAdditionalTagstokubernetes.io_cluster.<r.Infra.Status.InfrastructureName>(dot, no slashes), matching the repo’s documentedkubernetes.io_cluster.<cluster-id>tag expectation used by the image-registry operator.- No existing
*_test.gounderpkg/controllers/infracluster/was found coveringnewAzureCluster/AdditionalTags/ResourceTags; add a unit test for this tag generation and ensureInfrastructureNameis constrained/sanitized to the documented allowed characters (alphanumeric,-,.,_) so the resulting<cluster-id>won’t violate downstream expectations.
🤖 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 `@pkg/controllers/infracluster/azure.go` around lines 245 - 246, The
AdditionalTags key generation in newAzureCluster uses
Infra.Status.InfrastructureName directly, which can produce invalid cluster-id
characters; update the newAzureCluster code path (the function that sets
AdditionalTags/ResourceTags) to sanitize/validate
Infra.Status.InfrastructureName to only allow alphanumeric, '-', '.', and '_'
(e.g., strip or replace invalid chars and/or return an error) before composing
fmt.Sprintf("kubernetes.io_cluster.%s", ...). Add a unit test under
pkg/controllers/infracluster (e.g., newazurecluster_test.go) that calls
newAzureCluster (or the tag-generation helper) with various InfrastructureName
inputs (valid and containing bad chars) and asserts the produced AdditionalTags
key equals kubernetes.io_cluster.<sanitized-cluster-id> and that invalid inputs
are handled as expected.
| tags := azurev1.Tags{ | ||
| fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName): "owned", | ||
| } | ||
|
|
||
| if r.Infra.Status.PlatformStatus.Azure != nil { | ||
| for _, t := range r.Infra.Status.PlatformStatus.Azure.ResourceTags { | ||
| tags[t.Key] = t.Value | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: User tags can overwrite the ownership tag.
User-provided resource tags are merged into the map after the ownership tag, so if a user specifies a tag with key kubernetes.io_cluster.<infraID>, it will overwrite the system-generated ownership tag. According to the PR motivation, the installer's destroy logic relies on this tag to identify CAPI-created resources; overwriting it would cause resource leaks.
🔒 Proposed fix to protect the ownership tag
Insert user tags first, then set the ownership tag to ensure it cannot be overwritten:
- tags := azurev1.Tags{
- fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName): "owned",
- }
+ tags := azurev1.Tags{}
if r.Infra.Status.PlatformStatus.Azure != nil {
for _, t := range r.Infra.Status.PlatformStatus.Azure.ResourceTags {
tags[t.Key] = t.Value
}
}
+
+ // Always set the ownership tag last to prevent user overrides
+ tags[fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName)] = "owned"📝 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.
| tags := azurev1.Tags{ | |
| fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName): "owned", | |
| } | |
| if r.Infra.Status.PlatformStatus.Azure != nil { | |
| for _, t := range r.Infra.Status.PlatformStatus.Azure.ResourceTags { | |
| tags[t.Key] = t.Value | |
| } | |
| } | |
| tags := azurev1.Tags{} | |
| if r.Infra.Status.PlatformStatus.Azure != nil { | |
| for _, t := range r.Infra.Status.PlatformStatus.Azure.ResourceTags { | |
| tags[t.Key] = t.Value | |
| } | |
| } | |
| // Always set the ownership tag last to prevent user overrides | |
| tags[fmt.Sprintf("kubernetes.io_cluster.%s", r.Infra.Status.InfrastructureName)] = "owned" |
🤖 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 `@pkg/controllers/infracluster/azure.go` around lines 245 - 253, Currently
user-provided tags from r.Infra.Status.PlatformStatus.Azure.ResourceTags are
merged into the tags map after the ownership tag, allowing them to overwrite the
ownership key; instead, populate the tags map with user tags first (iterate
ResourceTags and set tags[t.Key] = t.Value) and only after that set the
ownership entry using the computed key fmt.Sprintf("kubernetes.io_cluster.%s",
r.Infra.Status.InfrastructureName) = "owned" so the ownership tag cannot be
overwritten (adjust code around the tags variable initialization and the loop
over ResourceTags).
Set the kubernetes.io_cluster.<infraID>=owned tag on the AzureCluster object via AdditionalTags so that CAPI-created Azure resources are visible to the installer's destroy logic. Azure tag keys do not permit slashes, so underscores and a dot are used instead. Also propagate user-defined resource tags from Infrastructure.Status.PlatformStatus.Azure.ResourceTags to match MAPI behaviour. Without this tag, openshift-install destroy cluster cannot identify CAPI-created resources and they are leaked. Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
matzew
force-pushed
the
OCPBUGS-88036-azure-ownership-tags
branch
from
35600b0 to
57f24a7
Compare
|
@matzew: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Set the kubernetes.io_cluster.=owned tag on the AzureCluster object via AdditionalTags so that CAPI-created Azure resources are visible to the installer's destroy logic. Azure tag keys do not permit slashes, so underscores and a dot are used instead. Also propagate user-defined resource tags from
Infrastructure.Status.PlatformStatus.Azure.ResourceTags to match MAPI behaviour.
Without this tag, openshift-install destroy cluster cannot identify CAPI-created resources and they are leaked.
Summary by CodeRabbit