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

Error out when mfT user surpassed seat limit #2809

Closed
eyalb181 opened this issue Oct 9, 2024 · 4 comments
Closed

Error out when mfT user surpassed seat limit #2809

eyalb181 opened this issue Oct 9, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@eyalb181
Copy link
Member

eyalb181 commented Oct 9, 2024

Currently, unless operator:true or the user tried to use a paid feature, we fall back to OSS, which can be confusing.

@eyalb181 eyalb181 added the enhancement New feature or request label Oct 9, 2024
Copy link

linear bot commented Oct 9, 2024

@meowjesty
Copy link
Member

For the issue in the title, we have a check in the operator that gives the user an error if they've reached the max seat count:

x mirrord exec
    ✓ running on latest (3.121.1)!
    x preparing to launch process
      ✓ layer extracted
      x checking operator
        ✓ operator version compatible
        ✓ operator license valid
        ✓ user credentials prepared
        x starting session
          x connecting to the target
Error:   × mirrord operator rejected creating a websocket connection: Maximum seat count (1) reached, session aborted
  help: If the problem refers to mirrord operator license, visit https://app.metalbear.co to manage or renew your license.

        - If you're still stuck:

This happens whether you set operator: true or if you omit the operator from mirrord.json.

I believe the only time we fallback now, is when checking license validity on the mirrord side:

match api.check_license_validity(&license_subtask) {
Ok(()) => license_subtask.success(Some("operator license valid")),
Err(error) => {
license_subtask.failure(Some("operator license expired"));
if config.operator == Some(true) {
return Err(error.into());
} else {
operator_subtask.failure(Some("proceeding without operator"));
return Ok(None);
}
}
}

Where this check is just looking at date stuff:

pub fn check_license_validity<P>(&self, progress: &P) -> OperatorApiResult<()>
where
P: Progress,
{
let Some(days_until_expiration) =
self.operator.spec.license.expire_at.days_until_expiration()
else {
let no_license_message = "No valid license found for mirrord for Teams. Visit https://app.metalbear.co to purchase or renew your license";
progress.warning(no_license_message);
tracing::warn!(no_license_message);
return Err(OperatorApiError::NoLicense);
};
let expires_soon =
days_until_expiration <= <DateTime<Utc> as LicenseValidity>::CLOSE_TO_EXPIRATION_DAYS;
let is_trial = self.operator.spec.license.name.contains("(Trial)");
if is_trial && expires_soon {
let expiring_soon = if days_until_expiration > 0 {
format!(
"soon, in {days_until_expiration} day{}",
if days_until_expiration > 1 { "s" } else { "" }
)
} else {
"today".to_string()
};
let message = format!("Operator license will expire {expiring_soon}!",);
progress.warning(&message);
} else if is_trial {
let message =
format!("Operator license is valid for {days_until_expiration} more days.");
progress.info(&message);
}
Ok(())
}

One thing I'm seeing there, however, is that we're not checking the license validity on mirrord side (we're not calling the license.is_good() method which checks today against expiration_date). When implementing this stuff initially, I left it like this to prevent potential time mismatches between the user's system time, and the operator's system time (maybe the user has set their computer date to the future). Do we want to be more strict with license validity checking there? Or just avoid falling back to non-operator mirrord (give an error, instead of Ok(None))?

@eyalb181
Copy link
Member Author

Not sure I understand the edge case from the user's perspective. Assuming seat limit has been surpassed and operator is set to true or omitted, when would they receive an error? When would they fall back to OSS?

@meowjesty
Copy link
Member

meowjesty commented Oct 25, 2024

I tested this scenario by changing our check in the operator here to always return the max seats reached error.

You'll get the error when we reach the "connecting to target" part, with omitted or operator: true:

let connection = api
.connect_in_new_session(target, config, &session_subtask)
.await?;

It should not fallback to OSS for this case.

@eyalb181 eyalb181 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants