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

Connect My Computer: Agent compatibility fixes #32477

Merged
merged 10 commits into from
Sep 27, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -24,45 +24,53 @@ import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvi
import { MockWorkspaceContextProvider } from 'teleterm/ui/fixtures/MockWorkspaceContextProvider';
import { MockAppContext } from 'teleterm/ui/fixtures/mocks';

import { isAgentCompatible, CompatibilityError } from './CompatibilityPromise';
import {
checkAgentCompatibility,
CompatibilityError,
} from './CompatibilityPromise';

describe('isAgentCompatible', () => {
const testCases = [
{
agentVersion: '2.0.0',
proxyVersion: '2.0.0',
isCompatible: true,
expected: 'compatible',
},
{
agentVersion: '2.1.0',
proxyVersion: '2.0.0',
isCompatible: true,
expected: 'compatible',
},
{
agentVersion: '3.0.0',
proxyVersion: '2.0.0',
isCompatible: false,
expected: 'incompatible',
},
{
agentVersion: '2.0.0',
proxyVersion: '3.0.0',
isCompatible: true,
expected: 'compatible',
},
{
agentVersion: '2.0.0',
proxyVersion: '4.0.0',
isCompatible: false,
expected: 'incompatible',
},
{
agentVersion: '2.0.0',
proxyVersion: '',
expected: 'unknown',
},
];
test.each(testCases)(
'should agent $agentVersion and cluster $proxyVersion be compatible? $isCompatible',
({ agentVersion, proxyVersion, isCompatible }) => {
'should agent $agentVersion and cluster $proxyVersion be compatible? $expected',
({ agentVersion, proxyVersion, expected }) => {
expect(
isAgentCompatible(
checkAgentCompatibility(
proxyVersion,
makeRuntimeSettings({ appVersion: agentVersion })
)
).toBe(isCompatible);
).toBe(expected);
}
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,24 @@ import { RuntimeSettings } from 'teleterm/mainProcess/types';
const CONNECT_MY_COMPUTER_RELEASE_VERSION = '14.1.0';
const CONNECT_MY_COMPUTER_RELEASE_MAJOR_VERSION = 14;

export function isAgentCompatible(
export type AgentCompatibility = 'unknown' | 'compatible' | 'incompatible';

export function checkAgentCompatibility(
proxyVersion: string,
runtimeSettings: Pick<RuntimeSettings, 'appVersion' | 'isLocalBuild'>
): boolean {
if (proxyVersion === '') {
return false;
): AgentCompatibility {
if (!proxyVersion) {
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
return 'unknown';
}
if (runtimeSettings.isLocalBuild) {
return true;
return 'compatible';
}
const majorAppVersion = getMajorVersion(runtimeSettings.appVersion);
const majorClusterVersion = getMajorVersion(proxyVersion);
return (
majorAppVersion === majorClusterVersion ||
return majorAppVersion === majorClusterVersion ||
majorAppVersion === majorClusterVersion - 1 // app one major version behind the cluster
);
? 'compatible'
: 'incompatible';
}

export function CompatibilityError(): JSX.Element {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ export function DocumentConnectMyComputerSetup() {
function Information(props: { onSetUpAgentClick(): void }) {
const { systemUsername, hostname, roleName, clusterName } =
useAgentProperties();
const { isAgentCompatible } = useConnectMyComputerContext();
const { agentCompatibility } = useConnectMyComputerContext();
const isAgentIncompatible = agentCompatibility === 'incompatible';
const isAgentIncompatibleOrUnknown =
agentCompatibility === 'incompatible' || agentCompatibility === 'unknown';

return (
<>
{!isAgentCompatible && (
{isAgentIncompatible && (
<>
<CompatibilityError />
<Separator mt={3} mb={2} />
Expand All @@ -94,7 +97,7 @@ function Information(props: { onSetUpAgentClick(): void }) {
css={`
display: block;
`}
disabled={!isAgentCompatible}
disabled={isAgentIncompatibleOrUnknown}
onClick={props.onSetUpAgentClick}
data-testid="start-setup"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,14 @@ export function DocumentConnectMyComputerStatus(
isAgentConfiguredAttempt,
markAgentAsNotConfigured,
removeAgent,
isAgentCompatible,
agentCompatibility,
} = useConnectMyComputerContext();
const { rootClusterUri } = useWorkspaceContext();
const { roleName, systemUsername, hostname } = useAgentProperties();
const { proxyVersion, appVersion, isLocalBuild } = useVersions();
const isAgentIncompatible = agentCompatibility === 'incompatible';
const isAgentIncompatibleOrUnknown =
agentCompatibility === 'incompatible' || agentCompatibility === 'unknown';

const prettyCurrentAction = prettifyCurrentAction(currentAction);

Expand Down Expand Up @@ -135,7 +138,11 @@ export function DocumentConnectMyComputerStatus(
const showConnectAndStopAgentButtons = isRunning || isKilling;
const disableConnectAndStopAgentButtons = isKilling;
const disableStartAgentButton =
isDownloading || isStarting || isRemoving || isRemoved;
isDownloading ||
isStarting ||
isRemoving ||
isRemoved ||
isAgentIncompatibleOrUnknown;

return (
<Box maxWidth="680px" mx="auto" mt="4" px="5" width="100%">
Expand Down Expand Up @@ -240,7 +247,7 @@ export function DocumentConnectMyComputerStatus(
)}
{prettyCurrentAction.logs && <Logs logs={prettyCurrentAction.logs} />}

{!isAgentCompatible ? (
{isAgentIncompatible ? (
<CompatibilityError />
) : (
<>
Expand Down
49 changes: 38 additions & 11 deletions web/packages/teleterm/src/ui/ConnectMyComputer/NavigationMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
useConnectMyComputerContext,
} from './connectMyComputerContext';

import type { AgentCompatibility } from './CompatibilityPromise';

/**
* IndicatorStatus combines a couple of different states into a single enum which dictates the
* decorative look of NavigationMenu.
Expand All @@ -39,12 +41,16 @@ export function NavigationMenu() {
const iconRef = useRef();
const [isMenuOpened, setIsMenuOpened] = useState(false);
const { documentsService, rootClusterUri } = useWorkspaceContext();
const { isAgentConfiguredAttempt, isAgentCompatible, currentAction, canUse } =
useConnectMyComputerContext();
const {
isAgentConfiguredAttempt,
agentCompatibility,
currentAction,
canUse,
} = useConnectMyComputerContext();
const indicatorStatus = getIndicatorStatus(
currentAction,
isAgentConfiguredAttempt,
isAgentCompatible
agentCompatibility
);

if (!canUse) {
Expand Down Expand Up @@ -114,7 +120,7 @@ export function NavigationMenu() {
function getIndicatorStatus(
currentAction: CurrentAction,
isAgentConfiguredAttempt: Attempt<boolean>,
isAgentCompatible: boolean
agentCompatibility: AgentCompatibility
): IndicatorStatus {
if (isAgentConfiguredAttempt.status === 'error') {
return 'error';
Expand All @@ -123,15 +129,16 @@ function getIndicatorStatus(
const isAgentConfigured =
isAgentConfiguredAttempt.status === 'success' &&
isAgentConfiguredAttempt.data;
const isAgentCompatibilityUnkown = agentCompatibility === 'unknown';

if (!isAgentConfigured) {
if (!isAgentConfigured || isAgentCompatibilityUnkown) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason behind the check for agent compatibility here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, we don't need it.

BTW, I realized that this condition !isAgentConfigured causes the indicator to be not shown during the setup.
I assume this is expected? I'm asking because I remember that I used your patch for this.

Copy link
Member

Choose a reason for hiding this comment

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

I made a draft PR with a reply to this comment. #32638

I also added one improvement there to flip autoStart to false if the auto start fails.

Let me know what you think and I'll merge it to this branch.

return '';
}

if (currentAction.kind === 'observe-process') {
switch (currentAction.agentProcessState.status) {
case 'not-started': {
if (!isAgentCompatible) {
if (agentCompatibility === 'incompatible') {
return 'error';
}

Expand Down Expand Up @@ -179,14 +186,18 @@ export const MenuIcon = forwardRef<HTMLDivElement, MenuIconProps>(
title="Open Connect My Computer"
>
<Laptop size="medium" />
{indicatorStatusToStyledStatus(props.indicatorStatus)}
{props.indicatorStatus === 'error' ? (
<StyledWarning />
) : (
indicatorStatusToStyledStatus(props.indicatorStatus)
)}
</StyledButton>
);
}
);

const indicatorStatusToStyledStatus = (
indicatorStatus: IndicatorStatus
indicatorStatus: '' | 'processing' | 'success'
): JSX.Element => {
return (
<StyledStatus
Expand Down Expand Up @@ -217,13 +228,14 @@ const indicatorStatusToStyledStatus = (
);
};

function getIndicatorColor(status: IndicatorStatus, theme: any): string {
function getIndicatorColor(
status: 'processing' | 'success',
theme: any
): string {
switch (status) {
case 'processing':
case 'success':
return theme.colors.success;
case 'error':
return theme.colors.error.main;
}
}

Expand All @@ -245,3 +257,18 @@ const StyledStatus = styled(Box)`
border-radius: 50%;
box-shadow: 0 4px 8px rgba(0, 0, 0, 0.1);
`;

const StyledWarning = styled(Warning).attrs({
size: 'small',
color: 'error.main',
})`
position: absolute;
top: -6px;
right: -6px;
z-index: 1;

> svg {
width: 14px;
height: 14px;
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { compareSemVers } from 'shared/utils/semVer';

import { RuntimeSettings } from 'teleterm/mainProcess/types';

import { isAgentCompatible } from './CompatibilityPromise';
import { checkAgentCompatibility } from './CompatibilityPromise';

export function shouldShowAgentUpgradeSuggestion(
proxyVersion: string,
Expand All @@ -32,7 +32,7 @@ export function shouldShowAgentUpgradeSuggestion(
const isClusterOnOlderVersion =
compareSemVers(proxyVersion, appVersion) === 1;
return (
isAgentCompatible(proxyVersion, runtimeSettings) &&
checkAgentCompatibility(proxyVersion, runtimeSettings) === 'compatible' &&
isClusterOnOlderVersion &&
!isLocalBuild
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ import { assertUnreachable, retryWithRelogin } from '../utils';

import { hasConnectMyComputerPermissions } from './permissions';

import { isAgentCompatible as checkIfAgentIsComptabile } from './CompatibilityPromise';
import {
checkAgentCompatibility,
AgentCompatibility,
} from './CompatibilityPromise';

import type {
AgentProcessState,
Expand Down Expand Up @@ -88,7 +91,7 @@ export interface ConnectMyComputerContext {
isAgentConfiguredAttempt: Attempt<boolean>;
markAgentAsConfigured(): void;
markAgentAsNotConfigured(): void;
isAgentCompatible: boolean;
agentCompatibility: AgentCompatibility;
}

const ConnectMyComputerContext = createContext<ConnectMyComputerContext>(null);
Expand Down Expand Up @@ -135,9 +138,9 @@ export const ConnectMyComputerContextProvider: FC<{
// https://github.com/gravitational/teleport/blob/master/rfd/0133-connect-my-computer.md#access-to-ui-and-autostart
return isFeatureFlagEnabled && (hasPermissions || isAgentConfigured);
}, [configService, isAgentConfigured, mainProcessClient, rootCluster]);
const isAgentCompatible = useMemo(
const agentCompatibility = useMemo(
() =>
checkIfAgentIsComptabile(
checkAgentCompatibility(
rootCluster.proxyVersion,
mainProcessClient.getRuntimeSettings()
),
Expand Down Expand Up @@ -352,6 +355,7 @@ export const ConnectMyComputerContextProvider: FC<{
const agentIsNotStarted =
currentAction.kind === 'observe-process' &&
currentAction.agentProcessState.status === 'not-started';
const isAgentCompatible = agentCompatibility === 'compatible';

useEffect(() => {
const shouldAutoStartAgent =
Expand Down Expand Up @@ -390,7 +394,7 @@ export const ConnectMyComputerContextProvider: FC<{
markAgentAsNotConfigured,
isAgentConfiguredAttempt,
removeAgent,
isAgentCompatible,
agentCompatibility,
}}
children={children}
/>
Expand Down