Skip to content
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

fix: set max firewall name to 36 #1645

Merged

Conversation

NissesSenap
Copy link
Contributor

This, to be able to have longer and unique names.
The firewall API supports 63 charters
Solves #1527

https://cloud.google.com/compute/docs/reference/rest/v1/firewalls

# The longest name with the extra chars added by the module
echo gke--intra-cluster-egress |wc -c
26
# the max ammount of char - the number of chars the module creates
expr 63 - 26
37
# I picked 36 since it looks better then 37 :)

@NissesSenap NissesSenap requested review from a team, Jberlinsky and ericyz as code owners May 25, 2023 08:13
@NissesSenap NissesSenap changed the title Set max firewall name to 36 fix: set max firewall name to 36 May 25, 2023
@bharathkkb
Copy link
Member

/gcbrun

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @NissesSenap

Looks like some of our tests may need updating

gcloud.Runf(t, "compute firewall-rules --project %s describe gke-%s-intra-cluster-egress", projectId, clusterName[:25])
gcloud.Runf(t, "compute firewall-rules --project %s describe gke-%s-webhooks", projectId, clusterName[:25])

This to be able to have longer and unique names.
The firewall API supports 64 charters
Solves terraform-google-modules#1527

Signed-off-by: Edvin Norling <edvin.norling@kognic.com>
Signed-off-by: Edvin Norling <edvin.norling@kognic.com>
@NissesSenap
Copy link
Contributor Author

@bharathkkb I updated the tests as you pointed out

@bharathkkb
Copy link
Member

/gcbrun

@bharathkkb
Copy link
Member

bharathkkb commented Jun 14, 2023

Looks like 36 is OOB since the test cluster name is smaller but I think we can just skip truncating since it's a static size from example.

 --- FAIL: TestSaferCluster (30.46s)
 panic: runtime error: slice bounds out of range [:36] with length 26 [recovered]
 	panic: runtime error: slice bounds out of range [:36] with length 26

@bharathkkb
Copy link
Member

/gcbrun

@bharathkkb bharathkkb merged commit 29d9259 into terraform-google-modules:master Jun 15, 2023
4 checks passed
@NissesSenap NissesSenap deleted the 1527_traunc_fw branch June 15, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants