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

Add handling for license requests in OSS #9963

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Feb 3, 2021

This changes the OSS response for GET /v1/operator/license from 501 to 204 to close #9827. It preserves the 1 status code when running nomad operator license. Here’s me interacting with it locally:

image

This means people won’t see what appears to be an error in their logs for a known situation, as the UI checks this endpoint to decide whether to show enterprise features.

This is quite naïve but it’s working for me locally!
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This mostly looks like a solid approach.

command/license_get_test.go is where a test for this happens, but it’s expecting an error code, should it still?

It should be changed to expect the new success code but it should probably still report that same message to the user.

I couldn’t find a test for API endpoint vs the command

Typically we don't have separate HTTP API tests; most of the logic is either in the RPC that the HTTP API calls, or in the CLI. So it's not a lot of payoff for the effort. This endpoint has so little logic it's probably safe to leave out a test so long as it compiles.

is 204 really the answer? with an empty string response?

Should it be an empty JSON body instead? Not sure what the UI typically expects.

will it break when enterprise tries to handle the endpoint instead?

Yeah, this isn't quite going to work. But if you move the new LicenseRequest method into a operator_endpoint_oss.go file with the directive // +build !ent at the top, that will work. Take a look at operator_endpoint_ent.go in the enterprise repo for an example of the opposite.

@backspace
Copy link
Contributor Author

Thanks for the input, @tgross!

re the error code, I believe the test is looking for the exit code vs the HTTP status code. So to preserve expectations around that maybe it should still be 1? CLI on enterprise vs free:

image

But I don’t know about whether keeping Error getting license: EOF is necessary 🤔

Because of this line in 204 documentation

Although this status code is intended to describe a response with no body, servers may erroneously include data following the headers.

I’m now leaning toward just 200 with {} as the body. In this case the UI doesn’t care what’s returned but an empty object seems clear to me.

@tgross
Copy link
Member

tgross commented Feb 4, 2021

re the error code, I believe the test is looking for the exit code vs the HTTP status code. So to preserve expectations around that maybe it should still be 1?

Ah yes it should just check for exit code 1.

But I don’t know about whether keeping Error getting license: EOF is necessary

I think the EOF is only going to show if it's not getting a body in the response. It's probably worth special-casing getting the 204 back to return a friendly message and then if we do get an EOF to return the existing message so that we're handling actual errors communicating with the server.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks @backspace!

@backspace backspace merged commit 288e50a into master Feb 8, 2021
@backspace backspace deleted the oss-license-endpoint branch February 8, 2021 18:53
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OSS Nomad spam Enterprise-only related errors in logs
2 participants