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

API Go module #5449

Merged
merged 21 commits into from
Feb 23, 2021
Merged

API Go module #5449

merged 21 commits into from
Feb 23, 2021

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Feb 1, 2021

Changes:

  • Initialize the api module and import it directly into the main teleport module.
  • Update make update-vendor to delete api from vendor, and add a symlink in its place.

@Joerger
Copy link
Contributor Author

Joerger commented Feb 1, 2021

@russjones @awly I'm not sure exactly what our vendoring strategy is. How do normally update dependencies, and are the upgrades here an issue?

Copy link
Contributor

@a-palchikov a-palchikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding upgrades - these are better done intentionally, e.g. when specific issues are targeted. But regular upgrades are also possible, I guess with the caveat that things need to continue working afterwards.

api/go.mod Show resolved Hide resolved
api/go.mod Outdated Show resolved Hide resolved
@russjones
Copy link
Contributor

@Joerger I'm not sure if we should vendor api here. @awly what do you think?

Copy link
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See golang/go#33789 for the vendoring problem.
Can you try adding some post-processing steps in make update-vendor to remove the vendored copy of api/?

go.mod Outdated Show resolved Hide resolved
@awly
Copy link
Contributor

awly commented Feb 4, 2021

@Joerger Joerger force-pushed the joerger/api-client-go-mod branch from b2e00b1 to 2ab25aa Compare February 4, 2021 21:37
@Joerger
Copy link
Contributor Author

Joerger commented Feb 4, 2021

See golang/go#33789 for the vendoring problem.
Can you try adding some post-processing steps in make update-vendor to remove the vendored copy of api/?

Post-processing does not seem to be possible in go 1.15, it leads to an "inconsistent vendoring error":

go: inconsistent vendoring in /home/bjoerger/gravitational/teleport:
	github.com/gravitational/teleport/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/gravitational/teleport/api: is replaced in go.mod, but not marked as replaced in vendor/modules.txt

See how consul does this: https://github.com/hashicorp/consul/blob/master/GNUmakefile#L225-L228

If you upgrade Consol to go 1.14+ you get the same issue:

go: inconsistent vendoring in /home/bjoerger/examples/consul:
	github.com/hashicorp/consul/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/hashicorp/consul/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/hashicorp/consul/api: is replaced in go.mod, but not marked as replaced in vendor/modules.txt
	github.com/hashicorp/consul/sdk: is replaced in go.mod, but not marked as replaced in vendor/modules.txt
	github.com/gophercloud/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod

I spent a while trying to find a way around this in go 1.15, but it seems that partial vendoring of modules just isn't supported, and manual modification of vendor doesn't seem intended. I couldn't find any other way around vendoring api.

@awly
Copy link
Contributor

awly commented Feb 4, 2021

@Joerger I got this working locally with a symlink:

.PHONY: update-vendor
update-vendor:
	go mod tidy
	go mod vendor
	rm -r vendor/github.com/gravitational/teleport/api
	ln -s $(shell readlink -f api) vendor/github.com/gravitational/teleport

The problem with keeping api vendored, is that the compiler looks at the version in vendor/. So if you make a change in api/ and forget to re-vendor, the change won't be picked up.

@Joerger Joerger force-pushed the joerger/api-client-go-mod branch from bc411a7 to 2ab25aa Compare February 5, 2021 00:37
Joerger and others added 12 commits February 8, 2021 17:48
This change removes the need for users to manually install u2f-host.
It also enables us to do U2F authentication with multiple devices.
Install `mingw-w64` cross-compiler toolchain in the buildbox and pass
magic flags to `go build` to use it.
* Increment to version 0.0.10

* Add kubernetes_service option config
After adding several U2F tokens with `tsh mfa add`, you can now `tsh
login` using any of those tokens.

Two caveats:

1. The MFA method you get prompted for on login depends on the
`second_factor` config field on the auth server. There isn't yet an
option to require _either_ TOTP or U2F yet, even if you have both kinds
registered.

2. Web logins still need updating.

Also a few small unrelated changes:
- remove u2f-host binary presence check and docs
- hide `tsh mfa` commands until the feature is complete
@Joerger Joerger force-pushed the joerger/api-client-go-mod branch from 1c0fa65 to 37bab12 Compare February 9, 2021 19:24
@Joerger Joerger requested review from awly and russjones February 9, 2021 23:20
@Joerger
Copy link
Contributor Author

Joerger commented Feb 9, 2021

@Joerger I got this working locally with a symlink

I changed this to a relative symlink to get it to work in docker without relinking.

lib/services/authentication.go Outdated Show resolved Hide resolved
api/types/authentication.go Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot.

@Joerger Joerger merged commit 427bafe into master Feb 23, 2021
@Joerger Joerger deleted the joerger/api-client-go-mod branch February 23, 2021 00:20
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.

9 participants