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.

Sign up for GitHub

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

Jump to bottom

Fix bug where broker container name can exceed 63 characters #14964

Merged
merged 4 commits into from
Oct 28, 2019
Merged

Fix bug where broker container name can exceed 63 characters #14964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8101022
Fix bug where broker container name can exceed 63 characters
amisevsk Oct 23, 2019
fd98a46
Fixup: Extract name generation to method and add tests
amisevsk Oct 24, 2019
8790908
fixup: Move some documentation around
amisevsk Oct 24, 2019
edec4e9
fixup: formatting
amisevsk Oct 25, 2019
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
4 changes: 2 additions & 2 deletions ...s/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ public static String machineName(ObjectMeta podMeta, Container container) {
if (containerName != null && containerName.length() > MAX_CONTAINER_NAME_LENGTH) {
throw new IllegalArgumentException(
format(
"The container name exceeds the allowed limit of %s characters.",
MAX_CONTAINER_NAME_LENGTH));
"The container name '%s' exceeds the allowed limit of %s characters.",
containerName, MAX_CONTAINER_NAME_LENGTH));
}

if (annotations != null
Expand Down
13 changes: 12 additions & 1 deletion .../workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,25 @@ private String generateUniqueName(String suffix) {
return NameGenerator.generate(suffix, 6);
}

/**
* Generate a container name from an image reference. Since full image references can be over 63
* characters, we need to strip registry and organization from the image reference to limit name
* length.
*/
@VisibleForTesting
protected String generateContainerNameFromImageRef(String image) {
return image.toLowerCase().replaceAll("[^/]*/", "").replaceAll("[^\\d\\w-]", "-");
}

private Container newContainer(
RuntimeIdentity runtimeId,
List<EnvVar> envVars,
String image,
@Nullable String brokerVolumeName) {
String containerName = generateContainerNameFromImageRef(image);
final ContainerBuilder cb =
new ContainerBuilder()
.withName(image.toLowerCase().replaceAll("[^\\d\\w-]", "-"))
.withName(containerName)
.withImage(image)
.withArgs(
"-push-endpoint",
Expand Down
21 changes: 21 additions & 0 deletions ...kspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

Expand Down Expand Up @@ -238,4 +239,24 @@ public void shouldCreateConfigMapWithPluginFQNs() throws Exception {
"Missing field from serialized config: expected '%s' in '%s'", expect, config));
}
}

@Test(dataProvider = "imageRefs")
public void testImageToContainerNameConversion(Object image, Object expected) {
String actual = factory.generateContainerNameFromImageRef((String) image);
assertEquals(
actual,
expected,
String.format("Should remove registry and organization from image '%s'.", image));
}

@DataProvider(name = "imageRefs")
public Object[][] imageRefs() {
return new Object[][] {
{"quay.io/eclipse/che-unified-plugin-broker:v0.20", "che-unified-plugin-broker-v0-20"},
{"very-long-registry-hostname-url.service/eclipse/image:tag", "image-tag"},
{"eclipse/che-unified-plugin-broker:v0.20", "che-unified-plugin-broker-v0-20"},
{"very-long-organization.name-eclipse-che/image:tag", "image-tag"},
{"very-long-registry-hostname-url.service/very-long-organization/image:tag", "image-tag"}
Copy link
Contributor

Choose a reason for hiding this comment

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

different images from different registries or users will have same name

It looks odd when you debug if there is no easy way to know from where it could come

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could always get the full image ref from the pod or env var though. There's no way to include registry and organization in the name, since it very easily goes over the 63 char limit (the length of just the broker pod name + version is 32 chars already).

Copy link
Contributor

Choose a reason for hiding this comment

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

I never talked on including the full registry or org name
but I think it's like some logger are doing for packages
instead of seeing org.apache.catalina.something you see o.a.c.something

In logger of Tomcat I see for example o.a.c.startup.VersionLoggerListener
so I know more from where is coming the VersionLoggerListener class than if only VersionLoggerListener was displayed

So I would say you could do something similar, it's for UX.

and of course I could go to openshift webconsole or somewhere else but it's definitely not UX

Copy link
Contributor

Choose a reason for hiding this comment

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

quay.io/my-organization/che-unified-plugin-broker:v0.20 --> q-m-che-unified-plugin-broker-v0-20

docker.io/my-organization/che-unified-plugin-broker:v0.20 --> d-m-che-unified-plugin-broker-v0-20

docker.io/other-organization/che-unified-plugin-broker:v0.20 --> d-o-che-unified-plugin-broker-v0-20

Copy link
Member

Choose a reason for hiding this comment

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

@benoitf this sounds like a relevant improvement, but is it really that valuable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think q-m-che-unified-broker-image-v0-20 and d-m-che-unified-plugin-broker-v0-20 are less readable than che-unified-plugin-broker-v0-20. Even with the truncation, it's an external reference (whereas logger trimming is referring to internal classes), so you'll still have to ask "what image is this/where is it hosted/what is it built from?" A single m character doesn't tell us anything real about the actual organization it's pushed to. I care more about what job the broker has (init vs unified), and what version it comes from (since e.g. v0.19 brokers don't support offline).

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems i'm the only one to care UX or that it's just a hot fix of removing repo/org and not fixing the 63 limit

If we know that we can't reach more than 63 values should never return more than 63 characters

@amisevk. You may use other prefix it was an example to illustrate that you may care repo/org more than full image name as well

I won't block but to me it's not a fix of the issue. Just hot fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Also speaking about UX I think issue is that we're giving each image name independently and not a set of images so the logic can work only on a limited data while with all images it could dedup in a better format all images

};
}
}