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 Does Not Respect GOFLAGS (-tags) During Build #1158

Open
camilamacedo86 opened this issue Mar 4, 2025 · 12 comments · May be fixed by #1160 or #1161
Open

controller-gen Does Not Respect GOFLAGS (-tags) During Build #1158

camilamacedo86 opened this issue Mar 4, 2025 · 12 comments · May be fixed by #1160 or #1161

Comments

@camilamacedo86
Copy link
Member

controller-gen currently does not respect/support Go build tags (-tags) passed via the GOFLAGS environment variable. This creates issues when working with code that relies on build constraints.

When trying to build using:

export GOFLAGS="-tags=mymodule"
controller-gen object paths=./apis/...

For example, the tags provided are not passed to the build.
Therefore, failures might occur for projects that rely on C-based packages.

Expected Behavior

  • controller-gen should respect the GOFLAGS environment variable.
  • Build tags should be properly applied when processing Go files.

Also, it seems related to: #871

@sbueringer
Copy link
Member

For example, the tags provided are not passed to the build.

Which build? :)

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Mar 5, 2025

Hi @sbueringer

The issue arises when working with Go Build Cache Optimization (GCO) enabled, where the controller-gen command fails due to missing build tags. Specifically, it does not respect the GOFLAGS environment variable that includes -tags, which is matter for projects that rely on conditional compilation.

For example, when running:

/home/runner/go/bin/controller-gen-v0.17.2 object:headerFile="hack/boilerplate.go.txt" paths="./..."
It results in the following error:

-: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/proglottis/[email protected]
Error: not all generators ran successfully
run `controller-gen object:headerFile=hack/boilerplate.go.txt paths=./... -w` to see all available markers, or `controller-gen object:headerFile=hack/boilerplate.go.txt paths=./... -h` for usage
make: *** [Makefile:151: generate] Error 1

This issue occurs because controller-gen does not propagate build tags to the Go toolchain when invoked. As a result, projects that depend on C-based dependencies, such as containers/image, fail to compile when required build constraints are not met.

To resolve this, I need to pass a specific build tag (GO_BUILD_TAGS), similar to how it can be done with golangci-lint:

golangci-lint run --build-tags $(GO_BUILD_TAGS)

Expected Behavior:

  • controller-gen should respect the GOFLAGS environment variable and correctly apply build tags.
  • It should function similarly to other tools like golangci-lint, which properly recognize and pass -tags to the Go compiler.

Let me know if any additional clarification is needed!

@sbueringer
Copy link
Member

Thx for the context. If someone has an idea on how to fix it, feel free to open a PR

@perdasilva
Copy link

perdasilva commented Mar 6, 2025

@sbueringer I think the way to fix it would be to pass configuration to LoadRootsWithConfig by modifying ForRoots around here. I'd be happy to put together the PR. What I'd like to know first is what would be the preferred way to instruct controller-gen to do that. Would something like what @camilamacedo86 suggested:

controller-gen --build-tags <values> ... be the right way?

Alternatives might be to consider GOFLAGS or take input from some env var like GO_BUILD_TAGS. But these options might have a large blast radius or sudden onset of unexpected behavior in some projects.

@sbueringer
Copy link
Member

sbueringer commented Mar 6, 2025

I think at this point of the project it would make sense that folks have to explicitly opt-in.

Regarding the exact way to do that, would be good in general to be consistent with how similar tools are doing that.

@JoelSpeed opinions from your side?

@JoelSpeed
Copy link
Contributor

I think at this point of the project it would make sense that folks have to explicitly opt-in.

Agreed

Regarding the exact way to do that, would be good in general to be consistent with how similar tools are doing that.

As far as I can tell, you'd have exactly the same issue with any Gengo based generator. The Parser loads with build tags passed into it based on this configuration.

But if you check the way this is done in, for example, deepcopy-gen they just pass the standard ignore_autogenerated tag, and there's no way for you to override the build tags.

We may want to raise the issue with the gengo folks and get their opinion as well

I could see us following a build-tags pattern, but we probably want to explore getting the same into gengo based generators simultaneously. @camilamacedo86 Do you happen to have tried any of the other gengo based generators to see if this works for them?

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Mar 6, 2025

Hi @JoelSpeed

That should occur with all. :(

/home/runner/go/bin/controller-gen-v0.17.2 crd paths="./api/v1/..." output:crd:artifacts:config=crd_work_dir
/home/runner/go/bin/controller-gen-v0.17.2 rbac:roleName=manager-role paths="./internal/operator-controller/..." output:rbac:artifacts:config=config/base/operator-controller/rbac
-: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/proglottis/[email protected]
run `controller-gen rbac:roleName=manager-role paths=./internal/operator-controller/... output:rbac:artifacts:config=config/base/operator-controller/rbac -w` to see all available markers, or `controller-gen rbac:roleName=manager-role paths=./internal/operator-controller/... output:rbac:artifacts:config=config/base/operator-controller/rbac -h` for usage

In our case, we found two possible workarounds to overcome this issue:

  • Isolate all markers into separate files so they do not cause issues.
    OR
  • Downgrade [email protected] since it's related to our specific case, replacing it with an older version.

What about we just ensure that controller-gen supports GOFLAGS? Or if we do not use the GOFLAGS, allow the user to explicitly say it for controller-gen via a specific ENV VAR like GO_BUILD_CONTROLLER_TOOLS_FLAGS?

This would allow users to dynamically pass build tags like containers_image_openpgp without modifying code or structuring files differently. Something like:

        // Default build tags
	buildTags := []string{"ignore_autogenerated"}

	// Read GOFLAGS for additional tags
	if goFlags := os.Getenv("GOFLAGS"); goFlags != "" {
		for _, flag := range strings.Fields(goFlags) {
			if strings.HasPrefix(flag, "-tags=") {
				// Extract and append custom tags
				tags := strings.TrimPrefix(flag, "-tags=")
				buildTags = append(buildTags, strings.Split(tags, ",")...)
			}
		}
	}

	// Add "-tags" argument to BuildFlags
	l.cfg.BuildFlags = append(l.cfg.BuildFlags, "-tags", strings.Join(buildTags, ","))

camilamacedo86 added a commit to camilamacedo86/operator-controller that referenced this issue Mar 6, 2025
Bumps [github.com/containers/image/v5](https://github.com/containers/image) from 5.33.1 to 5.34.0.
- [Release notes](https://github.com/containers/image/releases)
- [Commits](containers/[email protected])

---
updated-dependencies:
- dependency-name: github.com/containers/image/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

And downgrade :
replace github.com/proglottis/gpgme => github.com/proglottis/gpgme v0.1.3
Due: kubernetes-sigs/controller-tools#1158

Upgrade with Signed-off-by: Joe Lanford <[email protected]>
camilamacedo86 added a commit to camilamacedo86/operator-controller that referenced this issue Mar 6, 2025
Bumps [github.com/containers/image/v5](https://github.com/containers/image) from 5.33.1 to 5.34.0.
- [Release notes](https://github.com/containers/image/releases)
- [Commits](containers/[email protected])

---
updated-dependencies:
- dependency-name: github.com/containers/image/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

And downgrade :
replace github.com/proglottis/gpgme => github.com/proglottis/gpgme v0.1.3
Due: kubernetes-sigs/controller-tools#1158

Upgrade with Signed-off-by: Joe Lanford <[email protected]>
camilamacedo86 added a commit to camilamacedo86/operator-controller that referenced this issue Mar 6, 2025
Bumps [github.com/containers/image/v5](https://github.com/containers/image) from 5.33.1 to 5.34.0.
- [Release notes](https://github.com/containers/image/releases)
- [Commits](containers/[email protected])

---
updated-dependencies:
- dependency-name: github.com/containers/image/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

And downgrade :
replace github.com/proglottis/gpgme => github.com/proglottis/gpgme v0.1.3
Due: kubernetes-sigs/controller-tools#1158

Upgrade with Signed-off-by: Joe Lanford <[email protected]>
camilamacedo86 added a commit to camilamacedo86/operator-controller that referenced this issue Mar 6, 2025
Bumps [github.com/containers/image/v5](https://github.com/containers/image) from 5.33.1 to 5.34.0.
- [Release notes](https://github.com/containers/image/releases)
- [Commits](containers/[email protected])

---
updated-dependencies:
- dependency-name: github.com/containers/image/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

And downgrade :
replace github.com/proglottis/gpgme => github.com/proglottis/gpgme v0.1.3
Due: kubernetes-sigs/controller-tools#1158

Co-Author: Joe Lanford <[email protected]>
@joelanford
Copy link
Member

My first instinct if we want to preserve existing functionality would be to add a --build-tags flag (or whatever style suits controller-gen's semi-esoteric CLI structure).

Would we want it to be a global setting, or something passed individually to each generator? My feeling there is to make it a global setting only, which seems like it would cover basically all of the uses cases of this, especially given that controller-gen has been around for years with no one running into build tags issues before.

And if it turns out that we are asked to support per-generator build tags, that's always something that could be considered later.

@tmshort tmshort linked a pull request Mar 7, 2025 that will close this issue
@joelanford joelanford linked a pull request Mar 7, 2025 that will close this issue
@joelanford
Copy link
Member

@tmshort and I put each put together PRs that add support in two different ways.

Opinions?

@JoelSpeed
Copy link
Contributor

I'm leaning towards a flagged approach.

Using a marker has the issue that the marker could itself, be excluded from the build process by virtue of being within a file that has a particular build tag, right? IIUC, we have to parse the environment first to find the files marked, and then re-parse to load the correct build tags? Does the default parse include all files, or only those without build tags?

If we were generating multiple different versions (which is the common use of build tags right?), I'm not sure how I would be able to say "generate this version and then generate this version" when the input is a file within the code being generated, where I could easily see how to vary the flag inputs

@joelanford
Copy link
Member

joelanford commented Mar 7, 2025

@JoelSpeed I might be misunderstanding Todd's PR, but I think marker in that context is a command line marker, not a file-based comment marker.

So with my PR, one would invoke this way:

controller-gen --build-tags=tag1,tag2 crd ...

And Todd's PR, one would invoke something like this (@tmshort, correct me if I got the syntax wrong):

controller-gen crd ... buildTag=tag1,tag2

@tmshort
Copy link

tmshort commented Mar 7, 2025

@joelanford's invocation is correct, although there can be multiple buildTags supported:

controller-gen crd buildTag=tag1 buildTag=tag2 ...

Yes, code refers to these things as "markers", and are registered (code-wise) in the same way, AFAICT.

But it's probably best to think of it as a generic option closer to the path option than generators (e.g. crd) or additional "markers".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants