Conversation
WalkthroughThe changes introduce network resource synchronization for both AWS and Azure cloud providers in the CLI tool. New subcommands for syncing VPCs, subnets, and virtual networks are added, with full implementations for resource discovery, metadata extraction, and upsert into Ctrlplane. Supporting utilities, refactorings, and dependency updates are included for robust cloud integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CloudProvider (AWS/Azure)
participant CtrlplaneAPI
User->>CLI: Run sync aws networks / sync azure networks
CLI->>CloudProvider (AWS/Azure): List networks/subnets (per region or subscription)
CloudProvider (AWS/Azure)-->>CLI: Return network and subnet data
CLI->>CtrlplaneAPI: Upsert network and subnet resources
CtrlplaneAPI-->>CLI: Acknowledge upsert
CLI-->>User: Sync complete (with logs/errors)
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
cmd/ctrlc/root/sync/aws/common/provider.go (2)
10-12: Incomplete function documentation.The function documentation comment starts with "The" but doesn't complete the sentence.
// ComputeProviderDetails generates a provider name and region string based on the provided parameters. -// The +// The function modifies the name pointer to hold the computed provider name.
17-19: Check for empty regions slice.When checking if regions are provided, the code checks both
regions != nilandlen(regions) > 0. Since go slices are never nil after initialization but may be empty, you could simplify this check.// Use regions for name if none provided - if regions != nil && len(regions) > 0 { + if len(regions) > 0 { providerRegion = strings.Join(regions, "-") }cmd/ctrlc/root/sync/azure/common/resourceGroup.go (1)
33-38: Consider checking for nil properties in resource group objects.Azure SDK might return resource group objects with nil Name or Location properties in exceptional cases. Consider adding nil checks before dereferencing.
for _, rg := range page.Value { + // Skip resource groups with nil properties + if rg.Name == nil || rg.Location == nil { + continue + } results = append(results, ResourceGroupInfo{ Name: *rg.Name, Location: *rg.Location, }) }cmd/ctrlc/root/sync/aws/aws.go (1)
19-26: Consider updating command examples.With the addition of the networks sync command, it would be helpful to update the example documentation to include a networks example.
Example: heredoc.Doc(` # Make sure AWS credentials are configured via environment variables or ~/.aws/credentials # Sync all EC2 instances from a region $ ctrlc sync aws ec2 --region us-west-2 # Sync EC2 instances from a region every 5 minutes $ ctrlc sync aws ec2 --region us-west-2 --interval 5m + + # Sync VPC networks from a region + $ ctrlc sync aws networks --region us-west-2 `),cmd/ctrlc/root/sync/aws/networks/networks.go (2)
21-31: Doc-strings & comments reference Google – copy-paste errorSeveral comments still say “Google Networks / application default credentials / Compute Engine”.
This is misleading for maintainers.-// NewSyncNetworksCmd creates a new cobra command for syncing Google Networks +// NewSyncNetworksCmd creates a new cobra command for syncing AWS VPC networks ... - # Make sure AWS credentials are configured via environment variables or application default credentials + # Make sure AWS credentials are configured via environment variables or AWS CLI
408-431: Resource-provider relationship rules omittedUnlike the EKS sync, network resources are not linked to relationship rules (e.g. VPC → Subnet).
Consider adding them before the first upsert to preserve graph integrity.cmd/ctrlc/root/sync/azure/networks/networks.go (4)
208-211: Log message mentions “clusters” instead of “networks”Minor wording mismatch that can confuse operators when reading logs.
- log.Warn("Some clusters failed to sync", "errors", len(syncErrors)) + log.Warn("Some networks failed to sync", "errors", len(syncErrors))
294-303: Incorrectazure/resource-typevalue for virtual-network metadataThe resource type for a VNet should be
Microsoft.Network/virtualNetworks, not.../virtualNetworks/subnets.- "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", + "azure/resource-type": "Microsoft.Network/virtualNetworks",
342-349: Azure Portal URL missing tenant & subnet name
- Azure Portal URLs conventionally include the tenant segment (
#@TENANT_ID/…).- Subnet URLs should end with
/subnets/<subnetName>so the user lands on the specific subnet.Example fix for both helpers:
-func getVirtualNetworkConsoleUrl(resourceGroup, subscriptionID, networkName string) string { +func getVirtualNetworkConsoleUrl(resourceGroup, subscriptionID, tenantID, networkName string) string { return fmt.Sprintf( - "https://portal.azure.com/#@/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s", - subscriptionID, - resourceGroup, - networkName, + "https://portal.azure.com/#@%s/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s", + tenantID, + subscriptionID, + resourceGroup, + networkName, ) } -func getSubnetConsoleUrl(resourceGroup, subscriptionID, networkName string) string { +func getSubnetConsoleUrl(resourceGroup, subscriptionID, tenantID, networkName, subnetName string) string { return fmt.Sprintf( - "https://portal.azure.com/#@/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets", - subscriptionID, - resourceGroup, - networkName, + "https://portal.azure.com/#@%s/resource/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s", + tenantID, + subscriptionID, + resourceGroup, + networkName, + subnetName, ) }Call-sites will need to pass the new parameters.
365-373: Duplicate provider name defaulting
runSyncalready assigns a default when the name is empty; repeating the same check here is redundant.
Consider removing one of the two blocks to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
cmd/ctrlc/root/sync/aws/aws.go(2 hunks)cmd/ctrlc/root/sync/aws/common/provider.go(1 hunks)cmd/ctrlc/root/sync/aws/eks/eks.go(4 hunks)cmd/ctrlc/root/sync/aws/networks/networks.go(1 hunks)cmd/ctrlc/root/sync/azure/azure.go(2 hunks)cmd/ctrlc/root/sync/azure/common/resourceGroup.go(1 hunks)cmd/ctrlc/root/sync/azure/networks/networks.go(1 hunks)go.mod(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/ctrlc/root/sync/aws/common/provider.go (1)
cmd/ctrlc/root/sync/aws/common/regions.go (2)
InitAWSConfig(81-119)GetAccountID(71-78)
cmd/ctrlc/root/sync/azure/networks/networks.go (5)
internal/api/client.gen.go (1)
CreateResource(130-138)cmd/ctrlc/root/sync/azure/common/resourceGroup.go (2)
ResourceGroupInfo(10-13)GetResourceGroupInfo(15-42)internal/kinds/db.go (1)
CtrlplaneMetadataLinks(4-4)internal/api/client.go (1)
NewAPIKeyClientWithResponses(9-18)internal/api/resource_provider.go (1)
NewResourceProvider(11-50)
🔇 Additional comments (12)
go.mod (4)
6-12: New Azure SDK dependencies added correctly.These Azure SDK dependencies have been properly added to support the new Azure network scanning functionality.
18-22: AWS SDK dependencies added to support network scanning.Additional AWS SDK dependencies have been properly included to support the new AWS network scanning capabilities.
25-25: GitHub API client dependency added.The GitHub client addition will support potential GitHub-related operations.
35-35: OAuth2 and YAML dependencies promoted from indirect to direct.These dependencies have been promoted from indirect to direct usage, which improves clarity in dependency management.
Also applies to: 38-38
cmd/ctrlc/root/sync/azure/common/resourceGroup.go (2)
10-13: LGTM: Well-defined ResourceGroupInfo struct.The struct clearly captures the essential information from Azure resource groups.
15-42: LGTM: Well-implemented resource group retrieval function.The function correctly:
- Creates an Azure Resource Groups client
- Uses a pager to efficiently handle potentially large lists of resource groups
- Properly handles errors with descriptive messages
- Returns a clean data structure with the results
cmd/ctrlc/root/sync/aws/aws.go (2)
7-7: LGTM: Clean import of the networks package.The new import for the AWS networks package follows the existing pattern.
37-37: LGTM: Network scanning command added consistently.The new networks command has been added with interval support, consistent with the other sync commands.
cmd/ctrlc/root/sync/azure/azure.go (1)
6-7: Azure networks sub-command wired correctly – nothing to add 👍The new import and
networks.NewSyncNetworksCmd()registration are consistent with the existing pattern for AKS. No functional, concurrency, or error-propagation issues observed in this change.Also applies to: 34-37
cmd/ctrlc/root/sync/aws/eks/eks.go (2)
118-120: Upsert call now omits region – verify downstream expectationsThe helper signature changed to accept only
name.
Please double-check that:
api.NewResourceProvider(..., *name)does not rely on a non-empty string (it panics on""in earlier versions).- All other callers were updated.
No code change required here if the above holds true.
219-231: Good catch on thenormalizedStatustypoThe spelling fix and exhaustive switch values improve clarity. No further action required.
Also applies to: 244-245
cmd/ctrlc/root/sync/azure/networks/networks.go (1)
323-324: Consider using the fully qualified subnet resource typeFor subnets the type should be
Microsoft.Network/virtualNetworks/subnets(currently correct).
No action needed, just ensure consistency with the previous comment.
| // If name is not provided, try to get account ID to include in the provider name | ||
| if *name == "" { | ||
| // Get AWS account ID for provider name using common package | ||
| cfg, err := InitAWSConfig(ctx, regions[0]) |
There was a problem hiding this comment.
Potential panic with empty regions slice.
The function tries to access regions[0] without checking if the slice has any elements, which could cause a panic if regions is empty.
- cfg, err := InitAWSConfig(ctx, regions[0])
+ // Default to first region, but handle empty regions case
+ region := "us-east-1" // Default region
+ if len(regions) > 0 {
+ region = regions[0]
+ }
+ cfg, err := InitAWSConfig(ctx, region)📝 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.
| cfg, err := InitAWSConfig(ctx, regions[0]) | |
| // Default to first region, but handle empty regions case | |
| region := "us-east-1" // Default region | |
| if len(regions) > 0 { | |
| region = regions[0] | |
| } | |
| cfg, err := InitAWSConfig(ctx, region) |
| func ComputeProviderDetails( | ||
| ctx context.Context, prefix string, regions []string, name *string, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function missing return value.
Despite its name suggesting it computes and returns provider details, this function has a void return type and modifies the input name pointer instead. Consider returning the computed provider name instead of modifying the pointer.
func ComputeProviderDetails(
ctx context.Context, prefix string, regions []string, name *string,
-) {
+) string {📝 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.
| func ComputeProviderDetails( | |
| ctx context.Context, prefix string, regions []string, name *string, | |
| ) { | |
| func ComputeProviderDetails( | |
| ctx context.Context, prefix string, regions []string, name *string, | |
| ) string { |
cmd/ctrlc/root/sync/aws/eks/eks.go
Outdated
| common.ComputeProviderDetails(ctx, "aws-eks", regionsToSync, name) | ||
|
|
There was a problem hiding this comment.
Ignore-error pitfall – propagate ComputeProviderDetails failure
common.ComputeProviderDetails almost certainly returns an error, but it is being discarded.
If that helper fails (e.g. can’t resolve the AWS account), the subsequent upsert will still be attempted with an empty/invalid provider name, masking the root cause.
- common.ComputeProviderDetails(ctx, "aws-eks", regionsToSync, name)
+ if err := common.ComputeProviderDetails(ctx, "aws-eks", regionsToSync, name); err != nil {
+ return fmt.Errorf("computing provider details: %w", err)
+ }| consoleUrl := getSubnetConsoleUrl(subnet, region) | ||
| subnetName := getSubnetName(subnet) | ||
|
|
||
| metadata := map[string]string{ | ||
| "network/type": "subnet", | ||
| "network/name": subnetName, | ||
| "network/vpc": *subnet.VpcId, | ||
| "network/region": region, | ||
| "network/cidr": *subnet.CidrBlock, | ||
| "network/block-public-access": string(subnet.BlockPublicAccessStates.InternetGatewayBlockMode), | ||
|
|
||
| "google/resource-type": "compute.googleapis.com/Subnetwork", | ||
| "google/console-url": consoleUrl, | ||
| "google/region": region, | ||
| "google/id": *subnet.SubnetId, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compile-time error & wrong cloud-metadata keys in subnet metadata
types.Subnet has no field BlockPublicAccessStates; this will not compile.
Additionally, the keys are prefixed with google/…, not aws/….
- "network/block-public-access": string(subnet.BlockPublicAccessStates.InternetGatewayBlockMode),
-
- "google/resource-type": "compute.googleapis.com/Subnetwork",
- "google/console-url": consoleUrl,
- "google/region": region,
- "google/id": *subnet.SubnetId,
+ "network/public-ip-on-launch": strconv.FormatBool(
+ subnet.MapPublicIpOnLaunch != nil && *subnet.MapPublicIpOnLaunch),
+
+ "aws/resource-type": "ec2:subnet",
+ "aws/console-url": consoleUrl,
+ "aws/region": region,
+ "aws/id": *subnet.SubnetId,|
|
||
| return api.CreateResource{ | ||
| Version: "ctrlplane.dev/network/v1", | ||
| Kind: "AWSNetwork", |
There was a problem hiding this comment.
| Kind: "AWSNetwork", | |
| Kind: "AmazonNetwork", |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
cmd/ctrlc/root/sync/aws/common/provider.go (1)
27-27:⚠️ Potential issuePotential panic with empty regions slice.
The function tries to access
regions[0]without checking if the slice has any elements, which could cause a panic if regions is empty.- cfg, err := InitAWSConfig(ctx, regions[0]) + // Default to first region, but handle empty regions case + region := "us-east-1" // Default region + if len(regions) > 0 { + region = regions[0] + } + cfg, err := InitAWSConfig(ctx, region)cmd/ctrlc/root/sync/azure/networks/networks.go (2)
328-333:⚠️ Potential issueSame nil-pointer pitfall for subnet provisioning state
subnet.Propertiesandsubnet.Properties.ProvisioningStateneed checks before dereferencing.- "azure/status": func() string { - if network.Properties != nil { - return string(*subnet.Properties.ProvisioningState) - } - return "" - }(), + "azure/status": func() string { + if subnet.Properties != nil && subnet.Properties.ProvisioningState != nil { + return string(*subnet.Properties.ProvisioningState) + } + return "" + }(),
382-385:⚠️ Potential issue
getSubnetStatemirrors the same issue – add nil guards-func getSubnetState(subnet *armnetwork.Subnet) string { - if subnet.Properties != nil { - return string(*subnet.Properties.ProvisioningState) - } - return "" +func getSubnetState(subnet *armnetwork.Subnet) string { + if subnet.Properties != nil && subnet.Properties.ProvisioningState != nil { + return string(*subnet.Properties.ProvisioningState) + } + return "" }
🧹 Nitpick comments (5)
cmd/ctrlc/root/sync/aws/common/provider.go (4)
10-12: Fix incomplete function documentation.The comment for
EnsureProviderDetailsbegins with "The" but doesn't complete the sentence. This makes it difficult for users of this function to understand its full purpose and behavior.// EnsureProviderDetails generates a provider name and region string based on the provided parameters. -// The +// The function attempts to retrieve the AWS account ID to create a detailed provider name. +// If unable to retrieve the account ID, it falls back to a simpler naming format.
17-19: Simplify the condition by removing unnecessary nil check.In Go,
len()is safely defined for nil slices and returns 0, making the nil check redundant.- if regions != nil && len(regions) > 0 { + if len(regions) > 0 { providerRegion = strings.Join(regions, "-") }🧰 Tools
🪛 golangci-lint (1.64.8)
17-17: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
12-14: Consider returning the provider name instead of modifying a pointer.The function currently modifies the input
namepointer instead of returning the computed value. This makes the function harder to test and use, and is generally considered less idiomatic in Go.func EnsureProviderDetails( ctx context.Context, prefix string, regions []string, name *string, -) { +) string { + // If name is already provided and not empty, return it + if name != nil && *name != "" { + return *name + } + providerRegion := "all-regions" // Use regions for name if none provided if len(regions) > 0 { providerRegion = strings.Join(regions, "-") } - // If name is not provided, try to get account ID to include in the provider name - if name == nil { - name = new(string) - } - if *name == "" { + // Try to get account ID to include in the provider name // Get AWS account ID for provider name using common package region := "us-east-1" // Default region if len(regions) > 0 { region = regions[0] } cfg, err := InitAWSConfig(ctx, region) if err != nil { log.Warn("Failed to load AWS config for account ID retrieval", "error", err) - *name = fmt.Sprintf("%s-%s", prefix, providerRegion) + return fmt.Sprintf("%s-%s", prefix, providerRegion) } else { accountID, err := GetAccountID(ctx, cfg) if err == nil { log.Info("Retrieved AWS account ID", "account_id", accountID) - *name = fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion) + return fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion) } else { log.Warn("Failed to get AWS account ID", "error", err) - *name = fmt.Sprintf("%s-%s", prefix, providerRegion) + return fmt.Sprintf("%s-%s", prefix, providerRegion) } } - } }Note: This refactoring would require updating all call sites to use the returned value instead of passing a pointer.
31-40: Duplicate error handling logic can be simplified.The same fallback provider name pattern is used in both error cases. This can be simplified to reduce duplication.
if err != nil { log.Warn("Failed to load AWS config for account ID retrieval", "error", err) - *name = fmt.Sprintf("%s-%s", prefix, providerRegion) } else { accountID, err := GetAccountID(ctx, cfg) if err == nil { log.Info("Retrieved AWS account ID", "account_id", accountID) *name = fmt.Sprintf("%s-%s-%s", prefix, accountID, providerRegion) - } else { - log.Warn("Failed to get AWS account ID", "error", err) - *name = fmt.Sprintf("%s-%s", prefix, providerRegion) + return } + log.Warn("Failed to get AWS account ID", "error", err) } + *name = fmt.Sprintf("%s-%s", prefix, providerRegion)cmd/ctrlc/root/sync/azure/networks/networks.go (1)
209-212: Misleading log message – “clusters” vs “networks”
log.Warn("Some clusters failed to sync" …)was probably copied from another command. Replace clusters with networks to keep logs accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/ctrlc/root/sync/aws/common/provider.go(1 hunks)cmd/ctrlc/root/sync/aws/eks/eks.go(4 hunks)cmd/ctrlc/root/sync/aws/networks/networks.go(1 hunks)cmd/ctrlc/root/sync/azure/networks/networks.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ctrlc/root/sync/aws/networks/networks.go
- cmd/ctrlc/root/sync/aws/eks/eks.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/ctrlc/root/sync/azure/networks/networks.go (5)
internal/api/client.gen.go (1)
CreateResource(130-138)cmd/ctrlc/root/sync/azure/common/resourceGroup.go (2)
ResourceGroupInfo(10-13)GetResourceGroupInfo(15-42)internal/kinds/db.go (1)
CtrlplaneMetadataLinks(4-4)internal/api/client.go (1)
NewAPIKeyClientWithResponses(9-18)internal/api/resource_provider.go (1)
NewResourceProvider(11-50)
cmd/ctrlc/root/sync/aws/common/provider.go (1)
cmd/ctrlc/root/sync/aws/common/regions.go (2)
InitAWSConfig(81-119)GetAccountID(71-78)
🪛 golangci-lint (1.64.8)
cmd/ctrlc/root/sync/aws/common/provider.go
17-17: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
🔇 Additional comments (1)
cmd/ctrlc/root/sync/azure/networks/networks.go (1)
176-182: Loop-variable capture bug fixed – good job!Storing
rg.Namein the localrgNamevariable and passing it as an argument to the goroutine eliminates the data-race / “wrong RG” bug that was raised in the previous review.
Nothing further to add here. 👍
| func getNetworkState(network *armnetwork.VirtualNetwork) string { | ||
| return func() string { | ||
| if network.Properties != nil { | ||
| return string(*network.Properties.ProvisioningState) | ||
| } | ||
| return "" | ||
| }() |
There was a problem hiding this comment.
getNetworkState may panic – guard ProvisioningState
network.Properties.ProvisioningState can be nil.
-func getNetworkState(network *armnetwork.VirtualNetwork) string {
- return func() string {
- if network.Properties != nil {
- return string(*network.Properties.ProvisioningState)
- }
- return ""
- }()
+func getNetworkState(network *armnetwork.VirtualNetwork) string {
+ if network == nil || network.Properties == nil || network.Properties.ProvisioningState == nil {
+ return ""
+ }
+ return string(*network.Properties.ProvisioningState)
}📝 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.
| func getNetworkState(network *armnetwork.VirtualNetwork) string { | |
| return func() string { | |
| if network.Properties != nil { | |
| return string(*network.Properties.ProvisioningState) | |
| } | |
| return "" | |
| }() | |
| func getNetworkState(network *armnetwork.VirtualNetwork) string { | |
| if network == nil || network.Properties == nil || network.Properties.ProvisioningState == nil { | |
| return "" | |
| } | |
| return string(*network.Properties.ProvisioningState) | |
| } |
| networkName := network.Name | ||
| metadata := initNetworkMetadata(network, resourceGroup, subscriptionID, tenantID) | ||
|
|
||
| // Build console URL | ||
| consoleUrl := getNetworkConsoleUrl(resourceGroup, subscriptionID, *networkName) | ||
| metadata[kinds.CtrlplaneMetadataLinks] = fmt.Sprintf("{ \"Azure Portal\": \"%s\" }", consoleUrl) | ||
|
|
||
| resources = append(resources, api.CreateResource{ | ||
| Version: "ctrlplane.dev/network/v1", | ||
| Kind: "AzureNetwork", | ||
| Name: *networkName, | ||
| Identifier: *network.ID, | ||
| Config: map[string]any{ | ||
| // Common cross-provider options | ||
| "name": networkName, | ||
| "type": "vpc", | ||
| "id": network.ID, | ||
|
|
There was a problem hiding this comment.
Potential nil-pointer dereference when building the resource object
network.Name, network.ID, network.Type, and network.Location are all pointers returned by the SDK and are not guaranteed to be non-nil. Any of them being nil will panic.
Suggested defensive fix:
- networkName := network.Name
+ if network.Name == nil || network.ID == nil {
+ return nil, fmt.Errorf("network object missing Name or ID")
+ }
+ networkName := network.Name
...
- Identifier: *network.ID,
+ Identifier: *network.ID,
...
- "region": network.Location,
+ "region": func() *string {
+ if network.Location != nil {
+ return network.Location
+ }
+ empty := ""
+ return &empty
+ }(),Apply similar checks for every pointer you dereference in this block.
| var vpcName = getVpcName(vpc) | ||
| var consoleUrl = getVpcConsoleUrl(vpc, region) |
There was a problem hiding this comment.
| var vpcName = getVpcName(vpc) | |
| var consoleUrl = getVpcConsoleUrl(vpc, region) | |
| vpcName := getVpcName(vpc) | |
| consoleUrl := getVpcConsoleUrl(vpc, region) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/ctrlc/root/sync/azure/networks/networks.go (3)
30-41: Fix incorrect terminology in the examples.The examples incorrectly mention "AKS VPCs and subnets" which doesn't match the command's purpose. This command syncs Azure Virtual Networks, not specifically AKS (Azure Kubernetes Service) resources.
- # Sync all AKS VPCs and subnets from the subscription + # Sync all Azure Virtual Networks and subnets from the subscription $ ctrlc sync azure networks - # Sync all AKS VPCs and subnets from a specific subscription + # Sync all Azure Virtual Networks and subnets from a specific subscription $ ctrlc sync azure networks --subscription-id 00000000-0000-0000-0000-000000000000 - # Sync all AKS VPCs and subnets every 5 minutes + # Sync all Azure Virtual Networks and subnets every 5 minutes $ ctrlc sync azure networks --interval 5m
193-200: Error handling stops processing other networks in the same resource group.When a single network fails to process, the entire goroutine returns, skipping all other networks in the same resource group. Consider continuing to process other networks even if one fails.
resources, err := processNetwork(ctx, network, resourceGroup, subscriptionID, tenantID) if err != nil { log.Error("Failed to process network", "name", *network.Name, "error", err) mu.Lock() syncErrors = append(syncErrors, fmt.Errorf("network %s: %w", *network.Name, err)) mu.Unlock() - return + continue // Continue with other networks instead of returning }
333-337: Redundant nil check before len() operation.In Go,
len()is safe to call on nil slices and will return 0, so the nil check is unnecessary.- if subnet.Properties != nil { - if subnet.Properties.PrivateEndpoints != nil && len(subnet.Properties.PrivateEndpoints) > 0 { + if subnet.Properties != nil { + if len(subnet.Properties.PrivateEndpoints) > 0 { privateAccess = true } }🧰 Tools
🪛 golangci-lint (1.64.8)
334-334: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/ctrlc/root/sync/aws/networks/networks.go(1 hunks)cmd/ctrlc/root/sync/azure/networks/networks.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctrlc/root/sync/aws/networks/networks.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/ctrlc/root/sync/azure/networks/networks.go (6)
cmd/ctrlc/root/sync/aws/networks/networks.go (1)
NewSyncNetworksCmd(22-43)internal/api/client.gen.go (1)
CreateResource(130-138)cmd/ctrlc/root/sync/azure/common/resourceGroup.go (2)
ResourceGroupInfo(10-13)GetResourceGroupInfo(15-42)internal/kinds/db.go (1)
CtrlplaneMetadataLinks(4-4)internal/api/client.go (1)
NewAPIKeyClientWithResponses(9-18)internal/api/resource_provider.go (1)
NewResourceProvider(11-50)
🪛 golangci-lint (1.64.8)
cmd/ctrlc/root/sync/azure/networks/networks.go
334-334: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
| "azure/status": func() string { | ||
| if network.Properties != nil { | ||
| return string(*subnet.Properties.ProvisioningState) | ||
| } | ||
| return "" | ||
| }(), |
There was a problem hiding this comment.
Fix potential nil pointer dereference in subnet status.
The code attempts to access subnet.Properties.ProvisioningState without checking if subnet.Properties is nil, which could cause a panic.
"azure/status": func() string {
- if network.Properties != nil {
+ if subnet != nil && subnet.Properties != nil && subnet.Properties.ProvisioningState != nil {
return string(*subnet.Properties.ProvisioningState)
}
return ""
}(),Note that the incorrect condition was checking network.Properties instead of subnet.Properties.
📝 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.
| "azure/status": func() string { | |
| if network.Properties != nil { | |
| return string(*subnet.Properties.ProvisioningState) | |
| } | |
| return "" | |
| }(), | |
| "azure/status": func() string { | |
| if subnet != nil && subnet.Properties != nil && subnet.Properties.ProvisioningState != nil { | |
| return string(*subnet.Properties.ProvisioningState) | |
| } | |
| return "" | |
| }(), |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
cmd/ctrlc/root/sync/azure/networks/networks.go (2)
224-236:⚠️ Potential issuePotential nil-pointer dereference when building the resource object
network.Name,network.ID, and other pointer fields are not guaranteed to be non-nil. Any of them beingnilwill cause a panic.Add defensive checks:
- networkName := network.Name + if network.Name == nil || network.ID == nil { + return nil, fmt.Errorf("network object missing Name or ID") + } + networkName := network.Name // ... - Name: *networkName, - Identifier: *network.ID, + Name: *networkName, + Identifier: *network.ID,
244-247:⚠️ Potential issuePotential nil-pointer dereference for network.Type and network.Location
Both
network.Typeandnetwork.Locationare pointers returned by the SDK and are not guaranteed to be non-nil.Add safety checks:
- "type": network.Type, - "region": network.Location, + "type": func() *string { + if network.Type != nil { + return network.Type + } + return nil + }(), + "region": func() *string { + if network.Location != nil { + return network.Location + } + return nil + }(),
🧹 Nitpick comments (5)
cmd/ctrlc/root/sync/azure/networks/networks.go (5)
334-335: Simplify nil slice checkThe
len()function for nil slices is defined as zero, so the nil check is redundant.- if subnet.Properties.PrivateEndpoints != nil && len(subnet.Properties.PrivateEndpoints) > 0 { + if len(subnet.Properties.PrivateEndpoints) > 0 { privateAccess = true }🧰 Tools
🪛 golangci-lint (1.64.8)
334-334: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
212-213: Fix logging messageThe warning message refers to "clusters" but this command syncs networks, not clusters.
- log.Warn("Some clusters failed to sync", "errors", len(syncErrors)) + log.Warn("Some networks failed to sync", "errors", len(syncErrors))
33-34: Fix typo in example documentationThe example documentation refers to "AKS VPCs" but this command syncs Virtual Networks.
- # Sync all AKS VPCs and subnets from the subscription + # Sync all Azure Virtual Networks and subnets from the subscription
36-37: Fix typo in example documentationSimilar typo in another example.
- # Sync all AKS VPCs and subnets from a specific subscription + # Sync all Azure Virtual Networks and subnets from a specific subscription
39-40: Fix typo in example documentationSimilar typo in the interval example.
- # Sync all AKS VPCs and subnets every 5 minutes + # Sync all Azure Virtual Networks and subnets every 5 minutes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/ctrlc/root/sync/azure/networks/networks.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/ctrlc/root/sync/azure/networks/networks.go (5)
internal/api/client.gen.go (1)
CreateResource(130-138)cmd/ctrlc/root/sync/azure/common/resourceGroup.go (2)
ResourceGroupInfo(10-13)GetResourceGroupInfo(15-42)internal/kinds/db.go (1)
CtrlplaneMetadataLinks(4-4)internal/api/client.go (1)
NewAPIKeyClientWithResponses(9-18)internal/api/resource_provider.go (1)
NewResourceProvider(11-50)
🪛 golangci-lint (1.64.8)
cmd/ctrlc/root/sync/azure/networks/networks.go
334-334: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return statement inside goroutine could lead to missed resources
Using return inside a goroutine will only exit that goroutine, not the entire function. This means if one network fails to process, all remaining networks in that resource group won't be processed either.
Consider continuing processing other networks in the resource group even when one fails:
- if err != nil {
- log.Error("Failed to process network", "name", *network.Name, "error", err)
- mu.Lock()
- syncErrors = append(syncErrors, fmt.Errorf("network %s: %w", *network.Name, err))
- mu.Unlock()
- return
- }
+ if err != nil {
+ log.Error("Failed to process network", "name", *network.Name, "error", err)
+ mu.Lock()
+ syncErrors = append(syncErrors, fmt.Errorf("network %s: %w", *network.Name, err))
+ mu.Unlock()
+ continue
+ }| networkName := network.Name | ||
| subnetName := subnet.Name | ||
|
|
||
| // Build console URL | ||
| consoleUrl := getSubnetConsoleUrl(resourceGroup, subscriptionID, *networkName) | ||
| metadata[kinds.CtrlplaneMetadataLinks] = fmt.Sprintf("{ \"Azure Portal\": \"%s\" }", consoleUrl) | ||
|
|
||
| return api.CreateResource{ | ||
| Version: "ctrlplane.dev/network/subnet/v1", | ||
| Kind: "AzureSubnet", | ||
| Name: *subnetName, | ||
| Identifier: *subnet.ID, |
There was a problem hiding this comment.
Missing nil checks for network.Name and subnet.Name
network.Name and subnet.Name are pointers that could be nil, potentially causing a panic.
Add safety checks:
+ if network.Name == nil || subnet.Name == nil || subnet.ID == nil {
+ return api.CreateResource{}, fmt.Errorf("subnet or network object missing required fields")
+ }
metadata := initSubnetMetadata(network, subnet, resourceGroup, subscriptionID, tenantID)
networkName := network.Name
subnetName := subnet.Name📝 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.
| networkName := network.Name | |
| subnetName := subnet.Name | |
| // Build console URL | |
| consoleUrl := getSubnetConsoleUrl(resourceGroup, subscriptionID, *networkName) | |
| metadata[kinds.CtrlplaneMetadataLinks] = fmt.Sprintf("{ \"Azure Portal\": \"%s\" }", consoleUrl) | |
| return api.CreateResource{ | |
| Version: "ctrlplane.dev/network/subnet/v1", | |
| Kind: "AzureSubnet", | |
| Name: *subnetName, | |
| Identifier: *subnet.ID, | |
| // Guard against missing required fields | |
| if network.Name == nil || subnet.Name == nil || subnet.ID == nil { | |
| return api.CreateResource{}, fmt.Errorf("subnet or network object missing required fields") | |
| } | |
| metadata := initSubnetMetadata(network, subnet, resourceGroup, subscriptionID, tenantID) | |
| networkName := network.Name | |
| subnetName := subnet.Name | |
| // Build console URL | |
| consoleUrl := getSubnetConsoleUrl(resourceGroup, subscriptionID, *networkName) | |
| metadata[kinds.CtrlplaneMetadataLinks] = fmt.Sprintf("{ \"Azure Portal\": \"%s\" }", consoleUrl) | |
| return api.CreateResource{ | |
| Version: "ctrlplane.dev/network/subnet/v1", | |
| Kind: "AzureSubnet", | |
| Name: *subnetName, | |
| Identifier: *subnet.ID, | |
| // ... other fields ... | |
| } |
| metadata := map[string]string{ | ||
| "network/type": "subnet", | ||
| "network/name": *subnet.Name, | ||
| "network/vpc": *network.Name, | ||
| "network/region": *network.Location, | ||
| "network/private-access": strconv.FormatBool(privateAccess), | ||
| "azure/subscription": subscriptionID, | ||
| "azure/tenant": tenantID, | ||
| "azure/resource-group": resourceGroup, | ||
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | ||
| "azure/location": *network.Location, | ||
| "azure/status": getSubnetState(subnet), | ||
| "azure/id": *subnet.ID, | ||
| "azure/console-url": getSubnetConsoleUrl(resourceGroup, subscriptionID, *network.Name), |
There was a problem hiding this comment.
Missing nil checks for subnet and network pointers in subnet metadata map
Similar to the network metadata, there are potential nil-pointer dereferences when building the subnet metadata.
Add safety checks for all pointer dereferences:
metadata := map[string]string{
"network/type": "subnet",
- "network/name": *subnet.Name,
- "network/vpc": *network.Name,
- "network/region": *network.Location,
+ "network/name": func() string {
+ if subnet.Name != nil {
+ return *subnet.Name
+ }
+ return ""
+ }(),
+ "network/vpc": func() string {
+ if network.Name != nil {
+ return *network.Name
+ }
+ return ""
+ }(),
+ "network/region": func() string {
+ if network.Location != nil {
+ return *network.Location
+ }
+ return ""
+ }(),
"network/private-access": strconv.FormatBool(privateAccess),
"azure/subscription": subscriptionID,
"azure/tenant": tenantID,
"azure/resource-group": resourceGroup,
"azure/resource-type": "Microsoft.Network/virtualNetworks/subnets",
- "azure/location": *network.Location,
+ "azure/location": func() string {
+ if network.Location != nil {
+ return *network.Location
+ }
+ return ""
+ }(),
"azure/status": getSubnetState(subnet),
- "azure/id": *subnet.ID,
+ "azure/id": func() string {
+ if subnet.ID != nil {
+ return *subnet.ID
+ }
+ return ""
+ }(),📝 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.
| metadata := map[string]string{ | |
| "network/type": "subnet", | |
| "network/name": *subnet.Name, | |
| "network/vpc": *network.Name, | |
| "network/region": *network.Location, | |
| "network/private-access": strconv.FormatBool(privateAccess), | |
| "azure/subscription": subscriptionID, | |
| "azure/tenant": tenantID, | |
| "azure/resource-group": resourceGroup, | |
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | |
| "azure/location": *network.Location, | |
| "azure/status": getSubnetState(subnet), | |
| "azure/id": *subnet.ID, | |
| "azure/console-url": getSubnetConsoleUrl(resourceGroup, subscriptionID, *network.Name), | |
| metadata := map[string]string{ | |
| "network/type": "subnet", | |
| "network/name": func() string { | |
| if subnet.Name != nil { | |
| return *subnet.Name | |
| } | |
| return "" | |
| }(), | |
| "network/vpc": func() string { | |
| if network.Name != nil { | |
| return *network.Name | |
| } | |
| return "" | |
| }(), | |
| "network/region": func() string { | |
| if network.Location != nil { | |
| return *network.Location | |
| } | |
| return "" | |
| }(), | |
| "network/private-access": strconv.FormatBool(privateAccess), | |
| "azure/subscription": subscriptionID, | |
| "azure/tenant": tenantID, | |
| "azure/resource-group": resourceGroup, | |
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | |
| "azure/location": func() string { | |
| if network.Location != nil { | |
| return *network.Location | |
| } | |
| return "" | |
| }(), | |
| "azure/status": getSubnetState(subnet), | |
| "azure/id": func() string { | |
| if subnet.ID != nil { | |
| return *subnet.ID | |
| } | |
| return "" | |
| }(), | |
| "azure/console-url": getSubnetConsoleUrl(resourceGroup, subscriptionID, *network.Name), | |
| } |
| metadata := map[string]string{ | ||
| "network/type": "vpc", | ||
| "network/name": *network.Name, | ||
| "network/subnet-count": strconv.Itoa(subnetCount), | ||
| "network/id": *network.ID, | ||
| "azure/subscription": subscriptionID, | ||
| "azure/tenant": tenantID, | ||
| "azure/resource-group": resourceGroup, | ||
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | ||
| "azure/location": *network.Location, | ||
| "azure/status": getNetworkState(network), | ||
| "azure/id": *network.ID, | ||
| "azure/console-url": getNetworkConsoleUrl(resourceGroup, subscriptionID, *network.Name), |
There was a problem hiding this comment.
Missing nil checks for network pointers in metadata map
Several network properties are dereferenced without nil checks when building the metadata map.
Add safety checks for all pointer dereferences:
metadata := map[string]string{
"network/type": "vpc",
- "network/name": *network.Name,
+ "network/name": func() string {
+ if network.Name != nil {
+ return *network.Name
+ }
+ return ""
+ }(),
"network/subnet-count": strconv.Itoa(subnetCount),
- "network/id": *network.ID,
+ "network/id": func() string {
+ if network.ID != nil {
+ return *network.ID
+ }
+ return ""
+ }(),
"azure/subscription": subscriptionID,
"azure/tenant": tenantID,
"azure/resource-group": resourceGroup,
"azure/resource-type": "Microsoft.Network/virtualNetworks/subnets",
- "azure/location": *network.Location,
+ "azure/location": func() string {
+ if network.Location != nil {
+ return *network.Location
+ }
+ return ""
+ }(),
"azure/status": getNetworkState(network),
- "azure/id": *network.ID,
+ "azure/id": func() string {
+ if network.ID != nil {
+ return *network.ID
+ }
+ return ""
+ }(),📝 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.
| metadata := map[string]string{ | |
| "network/type": "vpc", | |
| "network/name": *network.Name, | |
| "network/subnet-count": strconv.Itoa(subnetCount), | |
| "network/id": *network.ID, | |
| "azure/subscription": subscriptionID, | |
| "azure/tenant": tenantID, | |
| "azure/resource-group": resourceGroup, | |
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | |
| "azure/location": *network.Location, | |
| "azure/status": getNetworkState(network), | |
| "azure/id": *network.ID, | |
| "azure/console-url": getNetworkConsoleUrl(resourceGroup, subscriptionID, *network.Name), | |
| metadata := map[string]string{ | |
| "network/type": "vpc", | |
| "network/name": func() string { | |
| if network.Name != nil { | |
| return *network.Name | |
| } | |
| return "" | |
| }(), | |
| "network/subnet-count": strconv.Itoa(subnetCount), | |
| "network/id": func() string { | |
| if network.ID != nil { | |
| return *network.ID | |
| } | |
| return "" | |
| }(), | |
| "azure/subscription": subscriptionID, | |
| "azure/tenant": tenantID, | |
| "azure/resource-group": resourceGroup, | |
| "azure/resource-type": "Microsoft.Network/virtualNetworks/subnets", | |
| "azure/location": func() string { | |
| if network.Location != nil { | |
| return *network.Location | |
| } | |
| return "" | |
| }(), | |
| "azure/status": getNetworkState(network), | |
| "azure/id": func() string { | |
| if network.ID != nil { | |
| return *network.ID | |
| } | |
| return "" | |
| }(), | |
| "azure/console-url": getNetworkConsoleUrl(resourceGroup, subscriptionID, *network.Name), | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores