Skip to content

Conversation

@AGWA
Copy link
Contributor

@AGWA AGWA commented May 29, 2020

As discussed in #971.

Before releasing this, you should update auth/kerberos/go.mod as follows:

  • Remove the replace directive.
  • Increase the version of the github.com/lib/pq dependency. (Moving forward this will need to be increased any time github.com/lib/pq has a new release.)

Thanks for working on pq!

AGWA added 2 commits May 29, 2020 14:39
Henceforth to use Kerberos authentication, you must add this import
statement somewhere in your program:

import _ "github.com/lib/pq/auth/kerberos"

Moving Kerberos to its own module reduces the number of dependencies
for those users who don't need Kerberos.

Closes: lib#971
@AGWA
Copy link
Contributor Author

AGWA commented May 29, 2020

The CI failures appear to be unrelated, and caused by a change in SSL certificate validation in Go master.

@madelynnblue
Copy link
Collaborator

Maybe looks good except for the forced version bumping. I think it's circular and can't work. Say I want to make a new release. I update go.mod to contain the future (but non-existent) tag. I now need to update go.sum, but I haven't pushed the tag, so the go proxy can't download it yet. It seems liked we'd have to refer to the previous lib/pq version, or else make 2 releases every time? I don't like any of these options.

I think we should instead have the krb package not auto-register itself with lib/pq. This would remove the dependency on lib/pq from krb. It would add one other line for GSS-users, but that's fine. It'd be like:

import "github.com/lib/pq/auth/krb"

func init() { pq.RegisterNewGSSFunc(krb.NewGSS) }

Am I correct? I haven't tried any of this so I could be wrong about some details. Or maybe there are better ways to avoid the forced go.sum bump thing?

@AGWA
Copy link
Contributor Author

AGWA commented Jun 1, 2020

I can't think of a way to avoid a two-step release process, I'm afraid.

I can rework this to require explicit registration, but it will have to look like this:

import "github.com/lib/pq/auth/krb"

func init() { pq.RegisterNewGSSFunc(func() (pq.Gss, error) { return krb.NewGSS() }) }

That's because RegisterNewGSSFunc expects a function returning an interface, but to avoid the dependency on pq, NewGSS will need to return a pointer to a concrete type instead.

@madelynnblue
Copy link
Collaborator

Yes that's fine. I can't see a better way either.

@ggilley
Copy link

ggilley commented Jun 2, 2020

I came here to write a ticket to suggest doing this. Thanks for being on top of it. I pulled the latest pq and was surprised at all the extra dependencies (especially the gorilla sessions and securecookie)

This removes the dependency from the kerberos module to the pq module,
which would have complicated releases.
@AGWA
Copy link
Contributor Author

AGWA commented Jun 2, 2020

I've reworked the patch to remove the dependency and require explicit registration.

@madelynnblue madelynnblue merged commit 65babff into lib:master Jun 8, 2020
xhit pushed a commit to xhit/pq that referenced this pull request Jul 16, 2020
Move Kerberos support to separate module
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.

3 participants