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

controller-gen interprets path as numeric literal and fails #734

Open
Jean-Daniel opened this issue Nov 21, 2022 · 9 comments
Open

controller-gen interprets path as numeric literal and fails #734

Jean-Daniel opened this issue Nov 21, 2022 · 9 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.

Comments

@Jean-Daniel
Copy link

When trying to run controller-gen, it fails with the following error, just because the path contains a dir with a name starting by 0.

Updating generated CRD deep-copy API code ...
Error: unable to parse option "object:headerFile=/var/folders/xt/087bdd_s6vd1j_jkk244w5bm0000gn/T/tmp.daYAU3yd": [invalid digit '8' in octal literal (at <input>:1:28)]
@SgtCoDFish
Copy link

SgtCoDFish commented Nov 22, 2022

We've just seen a similar (but slightly different) issue when testing cert-manager, where a standard tmpdir was interpreted as a hexadecimal floating point number:

$ /home/prow/go/src/github.com/cert-manager/cert-manager/_bin/tools/controller-gen \
	schemapatch:manifests=./deploy/crds \
	output:dir=/tmp/tmp.4paHkdEcpK \
	paths=./pkg/apis/...
Error: unable to parse option "output:dir=/tmp/tmp.4paHkdEcpK": ['p' exponent requires hexadecimal mantissa (at <input>:1:9) exponent has no digits (at <input>:1:9)]

The tmpdir was generated by a simple mktemp -d here.

edit: This was using controller-gen v0.10.0 installed via go install sigs.k8s.io/controller-tools/cmd/[email protected]

@camilamacedo86
Copy link
Member

Hi @SgtCoDFish,

What changes in the spec were made for the error began to be faced?
Could you let us know how is possible to reproduce this scenario outside of the github.com/cert-manager?

We need to ensure that it is not a bug/issue introduced in the github.com/cert-manager project.

@SgtCoDFish
Copy link

I can reliably reproduce using these steps:

# latest checkout of cert-manager
git clone [email protected]:cert-manager/cert-manager.git && cd cert-manager

# install controller-gen v0.10.0 using "go install" - the make command is complicated but it's just running "go install"
make _bin/downloaded/tools/[email protected]_linux_amd64

# create tmpdir which caused the failure
mkdir -p /tmp/tmp.4paHkdEcpK

# run command as above
 ./_bin/downloaded/tools/[email protected]_linux_amd64 schemapatch:manifests=./deploy/crds \
        output:dir=/tmp/tmp.4paHkdEcpK \
        paths=./pkg/apis/...

This is a completely unmodified controller-gen at v0.10.0.

The next test run worked successfully with the same command, same files, just a differently named tmpdir:

/home/prow/go/src/github.com/cert-manager/cert-manager/_bin/tools/controller-gen \
	schemapatch:manifests=./deploy/crds \
	output:dir=/tmp/tmp.SgrxkiqbO4 \
	paths=./pkg/apis/...

We run controller-gen in this way on every PR raised against cert-manager, and I haven't personally seen this error before but I'd bet it's reliably common with certain named tmpdirs.

@Jean-Daniel
Copy link
Author

We need to ensure that it is not a bug/issue introduced in the github.com/cert-manager project.

In my case, this is not related to cert-manager at all, as this is a locally developed controller.

@camilamacedo86
Copy link
Member

Hi @Jean-Daniel,

So the problem here is that controller-gen is unable to generate the files when the output is like /tmp/tmp.SgrxkiqbO4 (it has in the name dots). Just for we are able to confirm: can you try without a name that does not have dot and number like test to ensure that works?

@SgtCoDFish
Copy link

SgtCoDFish commented Nov 23, 2022

I can reproduce the issue with /var/folders/xt/087bdd_s6vd1j_jkk244w5bm0000gn/T/tmp.daYAU3yd that @Jean-Daniel reported using the cert-manager codebase as well:

git clone [email protected]:cert-manager/cert-manager.git && cd cert-manager
make _bin/downloaded/tools/[email protected]_linux_amd64

mkdir -p /tmp/087bdd_s6vd1j_jkk244w5bm0000g

 ./_bin/downloaded/tools/[email protected]_linux_amd64 schemapatch:manifests=./deploy/crds \
        output:dir=/tmp/087bdd_s6vd1j_jkk244w5bm0000g \
        paths=./pkg/apis/...

Output:

Error: unable to parse option "output:dir=/tmp/087bdd_s6vd1j_jkk244w5bm0000gn": [invalid digit '8' in octal literal (at <input>:1:6)]

The cert-manager job works as expected if I use a tmpdir called /tmp/tmp.daYAU3yd - the problem is with the 087bdd_s6vd1j_jkk244w5bm0000gn being treated as an octal literal.

@Jean-Daniel
Copy link
Author

Jean-Daniel commented Nov 23, 2022

Hi @Jean-Daniel,

So the problem here is that controller-gen is unable to generate the files when the output is like /tmp/tmp.SgrxkiqbO4 (it has in the name dots). Just for we are able to confirm: can you try without a name that does not have dot and number like test to ensure that works?

controller-gen, for some reason, tries to interpret each path component as a number literal. If a path component starts with 0, it assumes this is an octal literal, and crashes if it contains a 8 (which is invalid in octal literal). In the case of SgtCoDFish, it tries to parse tmp.4paHkdEcpK as a floating point literal, and obviously fails.

controller-gen MUST NOT try to interpret file names as some numeric literal.

@camilamacedo86
Copy link
Member

Would you like to working on this change? If so, please feel free to push a PR.

@allenhaozi
Copy link

I have the same problem

kubebuilder init --domain 1pq.io --repo tutorial.kubebuilder.io/project
kubebuilder create api --group core --version v1alpha1 --kind CronJob
make manifests

got the following errors:

/Users/mahao/go/src/test/project/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: 'p' exponent requires hexadecimal mantissa (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: exponent has no digits (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: 'p' exponent requires hexadecimal mantissa (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: exponent has no digits (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: 'p' exponent requires hexadecimal mantissa (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: exponent has no digits (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: 'p' exponent requires hexadecimal mantissa (at <input>:1:5)
/Users/mahao/go/src/test/project/api/v1alpha1/groupversion_info.go:19:1: exponent has no digits (at <input>:1:5)
/Users/mahao/go/src/test/project/controllers/cronjob_controller.go:36:1: 'p' exponent requires hexadecimal mantissa (at <input>:1:12)
/Users/mahao/go/src/test/project/controllers/cronjob_controller.go:36:1: exponent has no digits (at <input>:1:12)

The pattern of problems seems to be {number}p

SgtCoDFish added a commit to SgtCoDFish/cert-manager that referenced this issue Jan 3, 2023
Due to a bug in controller-gen[1] certain paths are incorrectly split
and part of these paths can be interpreted as a numeric literal, which
will cause controller-gen to fail. We observe this as occasional test
flakes in the "verify-crds" target, when the tmpdir starts with a zero,
such as in "/tmp/tmp.0PFqFSHBID"

This commit attempts to avoid this bug by specifying a template for the
tmpdir we generate when verifying CRDs which doesn't include any "."
characters, which seem to be being split incorrectly.

[1] kubernetes-sigs/controller-tools#734

Signed-off-by: Ashley Davis <[email protected]>
jetstack-bot pushed a commit to jetstack-bot/cert-manager that referenced this issue Jan 3, 2023
Due to a bug in controller-gen[1] certain paths are incorrectly split
and part of these paths can be interpreted as a numeric literal, which
will cause controller-gen to fail. We observe this as occasional test
flakes in the "verify-crds" target, when the tmpdir starts with a zero,
such as in "/tmp/tmp.0PFqFSHBID"

This commit attempts to avoid this bug by specifying a template for the
tmpdir we generate when verifying CRDs which doesn't include any "."
characters, which seem to be being split incorrectly.

[1] kubernetes-sigs/controller-tools#734

Signed-off-by: Ashley Davis <[email protected]>
jetstack-bot pushed a commit to jetstack-bot/cert-manager that referenced this issue Jan 3, 2023
Due to a bug in controller-gen[1] certain paths are incorrectly split
and part of these paths can be interpreted as a numeric literal, which
will cause controller-gen to fail. We observe this as occasional test
flakes in the "verify-crds" target, when the tmpdir starts with a zero,
such as in "/tmp/tmp.0PFqFSHBID"

This commit attempts to avoid this bug by specifying a template for the
tmpdir we generate when verifying CRDs which doesn't include any "."
characters, which seem to be being split incorrectly.

[1] kubernetes-sigs/controller-tools#734

Signed-off-by: Ashley Davis <[email protected]>
@camilamacedo86 camilamacedo86 added kind/bug Categorizes issue or PR as related to a bug. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 31, 2023
skriss added a commit to skriss/contour that referenced this issue Feb 23, 2023
Workaround for the following controller-gen issue:
kubernetes-sigs/controller-tools#734

Signed-off-by: Steve Kriss <[email protected]>
skriss added a commit to projectcontour/contour that referenced this issue Feb 23, 2023
Workaround for the following controller-gen issue:
kubernetes-sigs/controller-tools#734

Signed-off-by: Steve Kriss <[email protected]>
vmw-yingy pushed a commit to vmw-yingy/contour that referenced this issue Feb 28, 2023
Workaround for the following controller-gen issue:
kubernetes-sigs/controller-tools#734

Signed-off-by: Steve Kriss <[email protected]>
kaworu added a commit to kaworu/cilium that referenced this issue Apr 28, 2023
Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
pchaigno pushed a commit to cilium/cilium that referenced this issue May 1, 2023
Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
joamaki pushed a commit to joamaki/cilium that referenced this issue May 2, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
joestringer pushed a commit to cilium/cilium that referenced this issue May 4, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
joamaki pushed a commit to joamaki/cilium that referenced this issue May 5, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
joamaki pushed a commit to joamaki/cilium that referenced this issue May 5, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
michi-covalent pushed a commit to cilium/cilium that referenced this issue May 10, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
YutaroHayakawa pushed a commit to cilium/cilium that referenced this issue May 10, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Yutaro Hayakawa <[email protected]>
YutaroHayakawa pushed a commit to YutaroHayakawa/cilium that referenced this issue May 11, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Yutaro Hayakawa <[email protected]>
youngnick pushed a commit to cilium/cilium that referenced this issue May 11, 2023
[ upstream commit db3e015 ]

Before this patch, we would hit a controller-gen[1] bug when the
temporary file would be of the form tmp.0oXXXXXX.

This patch uses a custom mktemp template that will not trigger the bug.

[1]: kubernetes-sigs/controller-tools#734

Signed-off-by: Alexandre Perrin <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Yutaro Hayakawa <[email protected]>
logand22 pushed a commit to gravitational/cert-manager that referenced this issue Mar 8, 2024
Due to a bug in controller-gen[1] certain paths are incorrectly split
and part of these paths can be interpreted as a numeric literal, which
will cause controller-gen to fail. We observe this as occasional test
flakes in the "verify-crds" target, when the tmpdir starts with a zero,
such as in "/tmp/tmp.0PFqFSHBID"

This commit attempts to avoid this bug by specifying a template for the
tmpdir we generate when verifying CRDs which doesn't include any "."
characters, which seem to be being split incorrectly.

[1] kubernetes-sigs/controller-tools#734

Signed-off-by: Ashley Davis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

4 participants