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

fix: Simplification of the 'canCreateNamespace' logic #431

Merged
merged 1 commit into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012-2022 Red Hat, Inc.
* Copyright (c) 2012-2023 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
Expand Down Expand Up @@ -265,33 +265,11 @@ private KubernetesNamespaceMeta asNamespaceMeta(Namespace namespace) {
* Tells the caller whether the namespace that is being prepared for the provided workspace
* runtime identity can be created or is expected to already be present.
*
* <p>Note that this method cannot be reduced to merely checking if user-defined namespaces are
* allowed or not (and depending on prior validation using the {@link
* #checkIfNamespaceIsAllowed(String)} method during the workspace creation) because workspace
* start is a) async from workspace creation and the underlying namespaces might have disappeared
* and b) can be called during workspace recovery, where we don't even have the current user in
* the context.
*
* @param identity the identity of the workspace runtime
* @param userName the user name
* @return true if the namespace can be created, false if the namespace is expected to already
* exist
* @throws InfrastructureException on failure
*/
protected boolean canCreateNamespace(RuntimeIdentity identity, String userName)
throws InfrastructureException {
if (!namespaceCreationAllowed) {
return false;
}

String requiredNamespace = identity.getInfrastructureNamespace();

NamespaceResolutionContext resolutionContext =
new NamespaceResolutionContext(identity.getWorkspaceId(), identity.getOwnerId(), userName);

String resolvedDefaultNamespace = evaluateNamespaceName(resolutionContext);

return resolvedDefaultNamespace.equals(requiredNamespace);
protected boolean canCreateNamespace() {
return namespaceCreationAllowed;
}

/**
Expand All @@ -316,7 +294,7 @@ public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws Infrastr
evaluateAnnotationPlaceholders(resolutionCtx);

namespace.prepare(
canCreateNamespace(identity, userName),
canCreateNamespace(),
labelNamespaces ? namespaceLabels : emptyMap(),
annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap());

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012-2021 Red Hat, Inc.
* Copyright (c) 2012-2023 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
Expand Down Expand Up @@ -505,8 +505,6 @@ public void shouldCreateCredentialsSecretIfNotExists() throws Exception {
when(k8sClient.secrets()).thenReturn(mixedOperation);
when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation);
when(namespaceResource.get()).thenReturn(null);
when(cheClientFactory.create()).thenReturn(k8sClient);
when(clientFactory.create()).thenReturn(k8sClient);

// when
RuntimeIdentity identity =
Expand Down Expand Up @@ -753,7 +751,7 @@ public void shouldRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDef

// then
assertEquals(toReturnNamespace, namespace);
verify(toReturnNamespace).prepare(eq(false), any(), any());
verify(toReturnNamespace).prepare(eq(true), any(), any());
}

@Test
Expand Down Expand Up @@ -853,7 +851,6 @@ public void shouldBindToAllConfiguredClusterRoles() throws Exception {
when(toReturnNamespace.getName()).thenReturn("workspace123");
doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
when(k8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(true);
when(cheClientFactory.create()).thenReturn(k8sClient);
when(clientFactory.create(any())).thenReturn(k8sClient);

// pre-create the cluster roles
Expand Down Expand Up @@ -930,7 +927,6 @@ public void shouldCreateAndBindCredentialsSecretRole() throws Exception {
when(toReturnNamespace.getName()).thenReturn("workspace123");
doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
when(clientFactory.create(any())).thenReturn(k8sClient);
when(cheClientFactory.create()).thenReturn(k8sClient);

// when
RuntimeIdentity identity =
Expand Down Expand Up @@ -978,7 +974,6 @@ public void shouldCreateExecAndViewRolesAndBindings() throws Exception {
doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
when(k8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(true);
when(clientFactory.create(any())).thenReturn(k8sClient);
when(cheClientFactory.create()).thenReturn(k8sClient);

// when
RuntimeIdentity identity =
Expand Down Expand Up @@ -1452,7 +1447,7 @@ public void testUsernamePlaceholderInAnnotationsIsEvaluated() throws Infrastruct
// then
assertEquals(toReturnNamespace, namespace);
verify(toReturnNamespace)
.prepare(eq(false), any(), eq(Map.of("try_placeholder_here", "jondoe")));
.prepare(eq(true), any(), eq(Map.of("try_placeholder_here", "jondoe")));
}

@Test(dataProvider = "invalidUsernames")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012-2022 Red Hat, Inc.
* Copyright (c) 2012-2023 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
Expand Down Expand Up @@ -104,7 +104,6 @@ public OpenShiftProjectFactory(

public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws InfrastructureException {
OpenShiftProject osProject = get(identity);

var subject = EnvironmentContext.getCurrent().getSubject();
var userName = subject.getUserName();
NamespaceResolutionContext resolutionCtx =
Expand All @@ -113,7 +112,7 @@ public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws Infrastruct
evaluateAnnotationPlaceholders(resolutionCtx);

osProject.prepare(
canCreateNamespace(identity, userName),
canCreateNamespace(),
initWithCheServerSa && !isNullOrEmpty(oAuthIdentityProvider),
labelNamespaces ? namespaceLabels : emptyMap(),
annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012-2021 Red Hat, Inc.
* Copyright (c) 2012-2023 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
Expand Down Expand Up @@ -535,7 +535,7 @@ public void shouldRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDef

// then
assertEquals(toReturnProject, project);
verify(toReturnProject).prepare(eq(false), eq(false), any(), any());
verify(toReturnProject).prepare(eq(true), eq(false), any(), any());
}

@Test
Expand Down Expand Up @@ -854,7 +854,7 @@ public void testUsernamePlaceholderInAnnotationsIsEvaluated() throws Infrastruct
// then
assertEquals(toReturnProject, project);
verify(toReturnProject)
.prepare(eq(false), eq(false), any(), eq(Map.of("try_placeholder_here", "jondoe")));
.prepare(eq(true), eq(false), any(), eq(Map.of("try_placeholder_here", "jondoe")));
}

@Test
Expand Down