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

Extend operator headers #2550

Merged
merged 14 commits into from
Jul 2, 2024
Merged

Conversation

Razz4780
Copy link
Contributor

@Razz4780 Razz4780 commented Jun 26, 2024

Closes #2466

CLI version and user name/hostname headers are sent with all operator requests.
Client certificate header:

  1. mirrord operator status: we don't know operator license fingerprint before getting the resource, so header is not sent.
  2. mirrord exec: as soon as we get the operator resource, we prepare the client cert header. If we fail, command aborts.
  3. mirrord ls / mirrord operator session: as soon as we get the operator resource, we try to prepare the client cert header. If we fail, we continue without it

I kind of went full aggro on the operator client code and made more changes than necessary :V

  1. Copied to mirrord ls the extra discovery step present in mirrord exec (Improve operator discovery UX (CLI) #2487)
  2. Moved some parts of operator client code to new separate submodules (errors, http upgrade request logic, connection wrapper, operator discovery)
  3. Unified format of progress messages

@aviramha
Copy link
Member

We don't want a license to be needed for the non actual usage APIs. It should be fallible and optional.

@Razz4780
Copy link
Contributor Author

We don't want a license to be needed for the non actual usage APIs. It should be fallible and optional.

Do you mean operator license or user license?

@aviramha
Copy link
Member

We don't want a license to be needed for the non actual usage APIs. It should be fallible and optional.

Do you mean operator license or user license?

I meant user license but we shouldn't check the operator license for getting it's status anyway

@Razz4780
Copy link
Contributor Author

Razz4780 commented Jun 26, 2024

We don't want a license to be needed for the non actual usage APIs. It should be fallible and optional.

Do you mean operator license or user license?

I meant user license but we shouldn't check the operator license for getting it's status anyway

So, summarizing:

  1. operator status -> no check at all, plain GET with version/name/hostname headers
  2. target connection -> operator license validity check, user license required
  3. session management or listing targets -> no operator license validity check, fallible and optional user license

Correct?

@aviramha
Copy link
Member

I'd add the user license to the status too, just make it optional.

@Razz4780 Razz4780 requested a review from aviramha June 28, 2024 12:17
@Razz4780 Razz4780 removed the request for review from aviramha June 28, 2024 14:43
@Razz4780 Razz4780 assigned Razz4780 and DmitryDodzin and unassigned Razz4780 Jul 1, 2024
@Razz4780 Razz4780 requested a review from DmitryDodzin July 1, 2024 17:21
Copy link
Member

@DmitryDodzin DmitryDodzin left a comment

Choose a reason for hiding this comment

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

I think operator api can be a changed bit to prevent any issues that may happen post future changes

mirrord/cli/src/main.rs Outdated Show resolved Hide resolved
mirrord/operator/src/client.rs Outdated Show resolved Hide resolved
mirrord/operator/src/client.rs Outdated Show resolved Hide resolved
Copy link
Member

@DmitryDodzin DmitryDodzin left a comment

Choose a reason for hiding this comment

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

I would double check that the progress bar isn't a bit much but other than that looks good =]

@Razz4780 Razz4780 added this pull request to the merge queue Jul 2, 2024
Merged via the queue into metalbear-co:main with commit 63b3eed Jul 2, 2024
16 checks passed
@Razz4780 Razz4780 deleted the extend-operator-headers branch July 2, 2024 15:34
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.

Add client certificate header to all operator APIs
3 participants