[improvement] : allow passing vpcids and subnetids to ccm #423

rahulait · 2025-07-22T14:37:40Z

General:

This PR adds two new flags to CCM (--vpc-ids and --subnet-ids). This will allow someone to specify ids instead of names when starting CCM.

Key changes:
Added --vpc-ids and --subnet-ids command-line flags as IntSlice types
Changed existing --vpc-names and --subnet-names flags from string to StringSlice types
Added validation logic to ensure only one approach (names or IDs) is used at a time

Have you removed all sensitive information, including but not limited to access keys and passwords?
Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

Does your submission pass tests?
Have you added tests?
Are you addressing a single feature in this PR?
Are your commits atomic, addressing one change per commit?
Are you following the conventions of the language?
Have you saved your large formatting changes for a different PR, so we can focus on your work?
Have you explained your rationale for why this feature is needed?
Have you linked your PR to an open issue

codecov · 2025-07-22T14:39:11Z

Codecov Report

Attention: Patch coverage is 93.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 75.83%. Comparing base (557cb5d) to head (3747f45).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
cloud/linode/vpc.go	96.05%	2 Missing and 1 partial ⚠️
cloud/linode/cloud.go	33.33%	1 Missing and 1 partial ⚠️
cloud/linode/loadbalancers.go	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
+ Coverage   75.42%   75.83%   +0.40%     
==========================================
  Files          16       16              
  Lines        2987     3054      +67     
==========================================
+ Hits         2253     2316      +63     
- Misses        545      547       +2     
- Partials      189      191       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR adds new command-line flags (--vpc-ids and --subnet-ids) to the CCM, allowing users to specify IDs instead of names when configuring VPC routing. The changes maintain backward compatibility with existing name-based flags while adding validation to ensure only one approach is used at a time.

Key changes:

Added --vpc-ids and --subnet-ids flags as IntSlice types alongside existing name-based flags
Converted existing string flags to StringSlice types for consistency
Added comprehensive validation logic to prevent conflicting flag combinations
Updated Helm chart templates and documentation to reflect the new options

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
main.go	Updated flag definitions to support both ID and name-based VPC/subnet specification
cloud/linode/vpc.go	Added validation functions and ID-to-name resolution logic
cloud/linode/vpc_test.go	Added comprehensive tests for new validation and resolution functions
docs/configuration/environment.md	Updated documentation to include new flag options
deploy/chart/values.yaml	Added configuration examples for new ID-based flags
deploy/chart/templates/daemonset.yaml	Added Helm template validation and flag passing logic
Multiple test files	Updated test data to use new StringSlice format

cloud/linode/vpc.go

scmeggitt · 2025-07-22T21:13:34Z

cloud/linode/vpc.go

+func resolveSubnetNames(client client.Client, vpcID int) ([]string, error) {
+	subnetNames := []string{}
+	for _, subnetID := range Options.SubnetIDs {
+		subnet, err := client.GetVPCSubnet(context.TODO(), vpcID, subnetID)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get subnet %d for VPC %d: %w", subnetID, vpcID, err)
+		}
+
+		subnetIDs[subnet.Label] = subnet.ID
+		subnetNames = append(subnetNames, subnet.Label)
+	}


won't this still lead to the same problem if the subnet name is changed? Since we are just grabbing the label at one point in time?

Nope. We store everything in the cache and then it always looks into the cache.
vpcIDs and subnetIDs are the two caches it builds at startup and then uses going forward.

A ccm restart will cause that cache to be purged. Would that be an issue?

Nope. If someone updates vpcname/subnetname and CCM is restarted, it will again try to get the label for the passed vpcid/subnetid and store the updated name in its cache and continue working. When logging, it will log the vpc/subnet names instead of ids which are more readable/human friendly than the ids.

ahh ok I missed the vpcID logic since that was already there. Makes a lot more sense to me now.

Only other comment would be why can't you use ids and names?

You mean flipping the logic and using vpcids/subnetids instead of vpcnames/subnetnames? Or you meant combination of vpcname+subnetid?
I didn't flip the logic as that would mean all existing tests would start failing and that would be a much bigger change and touch most of the functionality of CCM.

the combination, but it really isn't that important. One or the other is fine with me.

It increases complexity and would be more error prone. I would prefer just using either names or ids but not both.

docs/configuration/environment.md

allow passing vpcids and subnetids to ccm

e0eaa41

github-actions bot added the improvement for improvements in existing functionality in the changelog. label Jul 22, 2025

rahulait temporarily deployed to prod July 22, 2025 14:38 — with GitHub Actions Inactive

rahulait had a problem deploying to prod July 22, 2025 14:38 — with GitHub Actions Failure

fix linting failures

b5bdc78

rahulait temporarily deployed to prod July 22, 2025 15:47 — with GitHub Actions Inactive

rahulait marked this pull request as ready for review July 22, 2025 18:48

rahulait temporarily deployed to prod July 22, 2025 18:48 — with GitHub Actions Inactive

rahulait had a problem deploying to prod July 22, 2025 18:48 — with GitHub Actions Error

add unittests

f640b63

rahulait force-pushed the accept-vpc-subnet-ids branch from ebdb773 to f640b63 Compare July 22, 2025 18:51

rahulait temporarily deployed to prod July 22, 2025 18:52 — with GitHub Actions Inactive

handle default value for subnet-names, add docs

9aa8eb4

rahulait requested a review from Copilot July 22, 2025 20:26

rahulait temporarily deployed to prod July 22, 2025 20:26 — with GitHub Actions Inactive

rahulait had a problem deploying to prod July 22, 2025 20:26 — with GitHub Actions Error

This comment was marked as outdated.

Sign in to view

resolve comments

d515402

rahulait requested a review from Copilot July 22, 2025 20:50

rahulait temporarily deployed to prod July 22, 2025 20:50 — with GitHub Actions Inactive

Copilot AI reviewed Jul 22, 2025

View reviewed changes

cloud/linode/vpc.go Show resolved Hide resolved

cloud/linode/vpc.go Show resolved Hide resolved

scmeggitt reviewed Jul 22, 2025

View reviewed changes

scmeggitt approved these changes Jul 23, 2025

View reviewed changes

AshleyDumaine reviewed Jul 23, 2025

View reviewed changes

docs/configuration/environment.md Outdated Show resolved Hide resolved

update docs to avoid showing slice in example

3747f45

rahulait had a problem deploying to prod July 23, 2025 14:50 — with GitHub Actions Failure

rahulait temporarily deployed to prod July 23, 2025 14:50 — with GitHub Actions Inactive

AshleyDumaine approved these changes Jul 23, 2025

View reviewed changes

komer3 approved these changes Jul 23, 2025

View reviewed changes

rahulait merged commit f1b64ce into main Jul 23, 2025
10 of 11 checks passed

AshleyDumaine deleted the accept-vpc-subnet-ids branch July 23, 2025 18:23

[improvement] : allow passing vpcids and subnetids to ccm #423

[improvement] : allow passing vpcids and subnetids to ccm #423

Uh oh!

Conversation

rahulait commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General:

Pull Request Guidelines:

Uh oh!

codecov bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

scmeggitt Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rahulait Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eljohnson92 Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rahulait Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

scmeggitt Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rahulait Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scmeggitt Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rahulait Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rahulait commented Jul 22, 2025 •

edited

Loading

codecov bot commented Jul 22, 2025 •

edited

Loading

rahulait Jul 22, 2025 •

edited

Loading

rahulait Jul 22, 2025 •

edited

Loading