-
Notifications
You must be signed in to change notification settings - Fork 244
fix: process imported_config outside switch #1651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cf8ac4b
to
0a413d5
Compare
0a413d5
to
06eee5d
Compare
} | ||
|
||
// imported_config is special because it applies to *any imported cluster*, | ||
// including hosted providers (AKS, EKS, GKE) where `imported = true` is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, if we are mixing here settings. For hosted providers imported
flag is set inside hosted_provider_spec, e.g. https://github.com/rancher/aks-operator/blob/main/pkg/apis/aks.cattle.io/v1/types.go#L43
Even if hosted provider imported cluster is set, then it should be managed by hoster provider operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got this example from customer
So do you mean that this is incorrect way of initialising it? And it should be inside the aks_config_v2
?
resource "rancher2_cluster" "aks_imported" {
name = var.aks_cluster_name
description = "Imported AKS cluster via Terraform"
aks_config_v2 {
cloud_credential_id = rancher2_cloud_credential.aks.id
resource_group = var.resource_group_name
resource_location = var.azure_location
imported = true
}
imported_config {
private_registry_url = "registry.suse.com"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it looks good to me, I had to understand bug description 👍
This looks good from a general Terraform point of view, but we still need approval from the responsible team to merge. |
} | ||
|
||
// imported_config is special because it applies to *any imported cluster*, | ||
// including hosted providers (AKS, EKS, GKE) where `imported = true` is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it looks good to me, I had to understand bug description 👍
Process imported_code flag outside of switch statement. (cherry picked from commit bc8d237)
Process imported_code flag outside of switch statement. (cherry picked from commit bc8d237)
Process imported_code flag outside of switch statement. (cherry picked from commit bc8d237)
Co-authored-by: Krunal Hingu <[email protected]>
Issue
Problem
Currently, the
imported_config
block is only processed when the driver is explicitly set to"imported"
.This causes updates to fail or silently ignore
imported_config
settings (e.g.,private_registry_url
) for hosted imported clusters, since they use their respective driver blocks (aks_config_v2
,eks_config_v2
,gke_config_v2
) instead of the"imported"
driver.Solution
The handling of
imported_config
has been pulled out of the driver switch block and is now applied independently of driver type.This ensures
imported_config
is always processed when defined, including for hosted imported clusters during updates.Testing
imported_config
block (e.g., settingprivate_registry_url
).imported_config
remain unaffected.Engineering Testing
Manual Testing
aks_config_v2
withimported = true
) into Rancher.imported_config
defined and verified the changes were applied correctly."imported"
driver clusters still acceptimported_config
.Automated Testing
imported_config
.QA Testing Considerations
imported_config
to ensure changes apply."imported"
driver clusters continue to work.imported_config
) succeed without errors.Regressions Considerations
"imported"
driver clusters.