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.

Sign up for GitHub

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

Jump to bottom

Impossible to create labels with a number as value #337

Closed
max-soe opened this issue Mar 22, 2023 · 1 comment
Closed

Impossible to create labels with a number as value #337

max-soe opened this issue Mar 22, 2023 · 1 comment

Comments

@max-soe
Copy link

max-soe commented Mar 22, 2023 •
edited
Loading

GCP not allow that keys of a label starts with a number. But the CPI currently also change values that starting with a number.

Coding:

bosh-google-cpi-release/src/bosh-google-cpi/google/instance_service/labels.go

Line 48 in a089393

if numFirstRe.MatchString(s) {

A key value pair like: index:1 is changed to index:n1. But it's possible to set the label index:1 via GCP API call.
@ramonskie
Copy link
Contributor

as it seems that only the values are going trough safe labels and never the keys (which is weird)
see https://github.com/cloudfoundry/bosh-google-cpi-release/blob/master/src/bosh-google-cpi/google/instance_service/google_instance_set_metadata.go#L66

i think we can safly remove that line.

altough i still think we should look in to why its only checking the values and not the keys
as the keys are the one that have restrictions see https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing#adding_tag_values

@beyhan beyhan closed this as completed in 9ac86b3 Apr 13, 2023
beyhan added a commit that referenced this issue Apr 13, 2023
@beyhan
Merge pull request #338 from cloudfoundry/337
2003b38 
remove the check for values begining with numbers will fix #337
klakin-pivotal added a commit that referenced this issue Apr 13, 2023
@klakin-pivotal
Revert "remove the check for values begining with numbers will fix #337"
555a1fc 
This reverts commit 9ac86b3.
klakin-pivotal added a commit that referenced this issue Apr 13, 2023
@klakin-pivotal
Revert "remove the check for values begining with numbers will fix #337"
729f446 
This reverts commit 9ac86b3.

PR 337 changed some code that is used for sanity-checking both Tag and
Label names to be too permissive. The rules for Label names are more
restrictive than the rules for Tag names, and the change made causes
some Labels to be rejected by GCP, resulting in integration test
failures like the following:

```
------------------------------
STEP: creating a VM
STEP: locating the VM
• Failure [23.037 seconds]
VM
/tmp/build/e797b25d/bosh-cpi-src/src/bosh-google-cpi/integration/vm_test.go:13
  executes the VM lifecycle [It]
  /tmp/build/e797b25d/bosh-cpi-src/src/bosh-google-cpi/integration/vm_test.go:104

  Expected
      <map[string]string | len:6>: {
          "director": "val-that-is-definitely-for-sure-absolutely-longer-than-the-al",
          "job": "job-name",
          "label-1-key": "label-1-value",
          "label-2-key": "label-2-value",
          "name": "val-with-underscores-ending-in-dash",
          "deployment": "deployment-name",
      }
  to be equivalent to
      <map[string]string | len:7>: {
          "director": "val-that-is-definitely-for-sure-absolutely-longer-than-the-al",
          "name": "val-with-underscores-ending-in-dash",
          "deployment": "deployment-name",
          "job": "job-name",
          "index": "n0",
          "label-1-key": "label-1-value",
          "label-2-key": "label-2-value",
      }

  /tmp/build/e797b25d/bosh-cpi-src/src/bosh-google-cpi/integration/vm_test.go:185
------------------------------
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Foundational Infrastructure Working Group
Archived in project
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ramonskie @max-soe