go.mod: Revert version bump of CEL

[francislavoie]

Workaround for #4331, for now

Unfortunately, bumping CEL will cause all Caddy plugins to no longer build with the latest version of Caddy, if they have an earlier version of Caddy in their go.mod.

I don't know what our long-term solution will be, so this will require more research before we can safely bump CEL to a newer version.

[mholt]

That's so weird. The new commits of CEL supposedly fixed that issue. Is it because plugins refer to older versions of Caddy?

[francislavoie]

Yes, it's because plugins require an older version of Caddy, which requires an older version of CEL which requires the wrong version of antlr. The Go compiler complains because there's two different antlr (ambiguous) in the build and it explodes.

I don't really understand why this issue happens, it seems like a Go module problem.

But if we do bump the CEL version for v2.5.0, I don't think any plugins will build with v2.5.0 unless the plugins also update their go.mod to Caddy v2.5.0.

We'll need to do more research, get help from experts on Go modules to figure out a solution that won't cause a hard line in the sand for plugins. Because that would really, really suck.

[mholt]

I just saw your message in Slack. Yeah, that makes sense. This smells like MVS to me. (Minimal Version Selection).

It looks like cel-go created an invalid module after v0.7.3. I don't fully understand the behavior of the Go tooling here, but I don't know if this can ever be fixed.

I think if cel-go released a v2 of their module, this might be fixable. Plugins that still refer to old versions of Caddy will use the pre-v1 module, and Caddy will use the new /v2 module, so MVS might not apply (or is applied differently, I suspect, as /v2 is viewed as a separate module, given the addition of /v2 to the package path). This is just a hunch/hope, I don't know if this is actually true though.

I'm not sure it's a good idea to keep using a dependency we cannot upgrade. It might be best long-term to either remove the CEL functionality or create an entirely new module with a clean history (no invalid versions) and use that instead.

But in the meantime, downgrading is acceptable so we don't block the release of v2.5.

[mholt]

Unfortunate, but apparently necessary. Thanks.

[TristonianJones]

@mholt @francislavoie At some point Antlr introduced a proper go.mod and people who were using it and the legacy directory that cel-go pointed at had issues. CEL moved to the official ANTLR go module to address the issue, but as I understand it, this is now causing problems for Caddy?

[francislavoie]

Unfortunately, yes 😢

The problem is plugins for Caddy reference an older version of Caddy in their go.mod so both the latest and the older versions of Caddy are pulled into the build, which means both versions of CEL as well, which means both ANTLR, and that triggers a built error. 😬

I'm not sure what the solution is. I think maybe both ANTLR and CEL would need a v2 or something to unbreak it. But I realize that might be too big an ask. I'm not sure what to do.

[TristonianJones]

Could you give me some more context on what's required for a v2 of the plugin? I'm happy to upgrade it if needed. I realized I missed your earlier reply to me about ways to better integrate CEL @francislavoie, so I just happened to spot this. I think there was some desire for additional functionality, so a v2 makes sense to me.

Also, we just did a ton of upgrades for cel-go v0.10.0 for Kubernetes, so it's probably a good time to bump.

[TristonianJones]

In talking with the team, we're going to go mod vendor ANTLR into cel-go which should remove it from the dependency list and bypass any issues with conflicting modules (I hope)

[francislavoie]

I'll be honest, I don't truly understand what considerations Go's module system makes that causes the problem, so I'm not certain what to expect as the outcome of changes to the upstream packages.

Vendoring sounds like an interesting plan though, if that would avoid the ambiguity problem.

If you need a reproduce case to help figure this out, I set up a branch https://github.com/caddyserver/caddy/compare/cel-upgrade which would let you use xcaddy to try to make a build of Caddy which includes a plugin that lists an earlier version of Caddy in its go.mod, which is where the problem was triggered.

You can run this to see the error happen:

xcaddy build cel-upgrade --with github.com/mastercactapus/caddy2-proxyprotocol

Using https://github.com/caddyserver/xcaddy which is our build tool (basically just a wrapper over go commands to make it easier to use plugins -- you can set XCADDY_SKIP_CLEANUP=1 env to inspect the temp build directory (i.e. go mod init etc) to run go build yourself if you need.

[francislavoie]

Btw maybe we should move the discussion to #4331 which is the actual open issue for this 😅

[TristonianJones]

@francislavoie Thanks for the pointer to the branch. I have vendoring staged locally and am curious if it addresses the upgrade issue.

[francislavoie]

If you have a branch of cel I can put in the go.mod to try it out, I could confirm

[TristonianJones]

@francislavoie It looks like the vendoring works out and makes the cel dependencies hermetic. I was able to successfully run the xcaddy command against my local copy of the cel-upgrade branch with the vendored cel-go being used via a go mod replace directive:

xcaddy build  --with github.com/mastercactapus/caddy2-proxyprotocol

I could be mistaken as I'm not super familiar with the xcaddy tool, so if you'd like to confirm my copy of the source with the vendored dependencies is located here: https://github.com/TristonianJones/cel-go/tree/vendor-antlr

[francislavoie]

I updated the cel-upgrade branch to have a replace line, but it's still giving me an error with xcaddy when building against cel-upgrade. 🤔

$ xcaddy.exe build cel-upgrade --with github.com/mastercactapus/caddy2-proxyprotocol
...
caddy imports
        github.com/caddyserver/caddy/v2/modules/standard imports
        github.com/caddyserver/caddy/v2/modules/caddyhttp/standard imports
        github.com/caddyserver/caddy/v2/modules/caddyhttp imports
        github.com/google/cel-go/cel imports
        github.com/google/cel-go/parser imports
        github.com/antlr/antlr4/runtime/Go/antlr: ambiguous import: found package github.com/antlr/antlr4/runtime/Go/antlr in multiple modules:
        github.com/antlr/antlr4 v0.0.0-20200503195918-621b933c7a7f (C:\Users\lavof\go\pkg\mod\github.com\antlr\antlr4@v0.0.0-20200503195918-621b933c7a7f\runtime\Go\antlr)
        github.com/antlr/antlr4/runtime/Go/antlr v0.0.0-20210826220005-b48c857c3a0e (C:\Users\lavof\go\pkg\mod\github.com\antlr\antlr4\runtime\!go\antlr@v0.0.0-20210826220005-b48c857c3a0e)

[francislavoie]

Oh, I think the replace might be ignored when building against a target branch, for some reason.

[francislavoie]

Trying this doesn't work either:

$ xcaddy.exe build cel-upgrade --with github.com/mastercactapus/caddy2-proxyprotocol --with github.com/google/cel-go=github.com/TristonianJones/cel-go@vendor-antlr
...
go: finding module for package github.com/google/cel-go
caddy imports
        github.com/google/cel-go: module github.com/google/cel-go@latest found (v0.10.1, replaced by github.com/TristonianJones/cel-go@v0.0.0-20220313061635-0a41511c52b7), but does not contain package github.com/google/cel-go

[francislavoie]

Anyways to clarify, running xcaddy build without another argument doesn't look at your current directory at all, it will try to build from the latest tagged release of Caddy instead. So what you tried built successfully cause it just built v2.4.6 basically.

[TristonianJones]

@francislavoie looks like the error is complaining about the GitHub repo being my fork. I'll put up a PR to vendor dependencies, and will try again with a pre-release candidate once someone from my team has had a look.

[francislavoie]

@TristonianJones are you able to make a branch on the cel-go repo? Might work better to try

[TristonianJones]

I finally moved the commentary over to the issue, but I've noticed that we can probably solve the ANTLR upgrade path with a patch into xcaddy to do a default replacement.