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

Owls 87956 - Generate shorter volume name when override secret name is too long #2257

Merged
merged 8 commits into from
Mar 12, 2021
Merged

Owls 87956 - Generate shorter volume name when override secret name is too long #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
49c2acb
OWLS-87956 - Generate shorter volume name when override secret name i…
ankedia Mar 10, 2021
3f53a3a
Minor changes
ankedia Mar 10, 2021
e641782
Introduce constant for default mode.
ankedia Mar 10, 2021
0fb1663
Minor refactoring.
ankedia Mar 11, 2021
f634779
Address PR review comments to use a new volume naming scheme with che…
ankedia Mar 11, 2021
63810e3
Use different suffix for generated volume names for secret and config…
ankedia Mar 11, 2021
73984f2
Minor change.
ankedia Mar 11, 2021
c1ccff8
Remove unnecessary method.
ankedia Mar 12, 2021
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
33 changes: 27 additions & 6 deletions operator/src/main/java/oracle/kubernetes/operator/helpers/JobStepContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import oracle.kubernetes.operator.calls.UnrecoverableErrorBuilder;
import oracle.kubernetes.operator.logging.LoggingFacade;
import oracle.kubernetes.operator.logging.LoggingFactory;
import oracle.kubernetes.operator.utils.ChecksumUtils;
import oracle.kubernetes.operator.work.NextAction;
import oracle.kubernetes.operator.work.Packet;
import oracle.kubernetes.operator.work.Step;
Expand All @@ -44,6 +45,10 @@ public abstract class JobStepContext extends BasePodStepContext {
private static final LoggingFacade LOGGER = LoggingFactory.getLogger("Operator", "Operator");
private static final String WEBLOGIC_OPERATOR_SCRIPTS_INTROSPECT_DOMAIN_SH =
"/weblogic-operator/scripts/introspectDomain.sh";
private static final int MAX_ALLOWED_VOLUME_NAME_LENGTH = 63;
public static final String VOLUME_NAME_SUFFIX = "-volume";
public static final String CONFIGMAP_TYPE = "cm";
public static final String SECRET_TYPE = "st";
private V1Job jobModel;

JobStepContext(Packet packet) {
Expand Down Expand Up @@ -313,14 +318,14 @@ protected V1PodSpec createPodSpec(TuningParameters tuningParameters) {
private void addConfigOverrideSecretVolume(V1PodSpec podSpec, String secretName) {
podSpec.addVolumesItem(
new V1Volume()
.name(secretName + "-volume")
.name(getVolumeName(secretName, SECRET_TYPE))
.secret(getOverrideSecretVolumeSource(secretName)));
}

private void addConfigOverrideVolume(V1PodSpec podSpec, String configOverrides) {
podSpec.addVolumesItem(
new V1Volume()
.name(configOverrides + "-volume")
.name(getVolumeName(configOverrides, CONFIGMAP_TYPE))
.configMap(getOverridesVolumeSource(configOverrides)));
}

Expand All @@ -331,7 +336,7 @@ private boolean isSourceWdt() {
private void addWdtConfigMapVolume(V1PodSpec podSpec, String configMapName) {
podSpec.addVolumesItem(
new V1Volume()
.name(configMapName + "-volume")
.name(getVolumeName(configMapName, CONFIGMAP_TYPE))
.configMap(getWdtConfigMapVolumeSource(configMapName)));
}

Expand Down Expand Up @@ -365,20 +370,20 @@ protected V1Container createPrimaryContainer(TuningParameters tuningParameters)

if (getConfigOverrides() != null && getConfigOverrides().length() > 0) {
container.addVolumeMountsItem(
readOnlyVolumeMount(getConfigOverrides() + "-volume", OVERRIDES_CM_MOUNT_PATH));
readOnlyVolumeMount(getVolumeName(getConfigOverrides(), CONFIGMAP_TYPE), OVERRIDES_CM_MOUNT_PATH));
}

List<String> configOverrideSecrets = getConfigOverrideSecrets();
for (String secretName : configOverrideSecrets) {
container.addVolumeMountsItem(
readOnlyVolumeMount(
secretName + "-volume", OVERRIDE_SECRETS_MOUNT_PATH + '/' + secretName));
getVolumeName(secretName, SECRET_TYPE), OVERRIDE_SECRETS_MOUNT_PATH + '/' + secretName));
}

if (isSourceWdt()) {
if (getWdtConfigMap() != null) {
container.addVolumeMountsItem(
readOnlyVolumeMount(getWdtConfigMap() + "-volume", WDTCONFIGMAP_MOUNT_PATH));
readOnlyVolumeMount(getVolumeName(getWdtConfigMap(), CONFIGMAP_TYPE), WDTCONFIGMAP_MOUNT_PATH));
}
container.addVolumeMountsItem(
readOnlyVolumeMount(RUNTIME_ENCRYPTION_SECRET_VOLUME,
Expand All @@ -389,6 +394,22 @@ protected V1Container createPrimaryContainer(TuningParameters tuningParameters)
return container;
}

private String getVolumeName(String resourceName, String type) {
Copy link
Member

Choose a reason for hiding this comment

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

If these methods do the same thing, why do you need two of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was leftover from the previous implementation and I thought having methods with different names might provide better readability. I went ahead and removed it.

return getName(resourceName, type);
}

private String getName(String resourceName, String type) {
return resourceName.length() > (MAX_ALLOWED_VOLUME_NAME_LENGTH - VOLUME_NAME_SUFFIX.length())
? getShortName(resourceName, type)
: resourceName + VOLUME_NAME_SUFFIX;
}

private String getShortName(String resourceName, String type) {
String volumeSuffix = VOLUME_NAME_SUFFIX + "-" + type + "-"
+ Optional.ofNullable(ChecksumUtils.getMD5Hash(resourceName)).orElse("");
Copy link
Member

Choose a reason for hiding this comment

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

When would the result of getMD5Hash be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be null when MessageDigest.getInstance("MD5") method (in ChecksumUtils) throws NoSuchAlgorithmException or String.getBytes() method throws UnsupportedEncodingException.

return resourceName.substring(0, MAX_ALLOWED_VOLUME_NAME_LENGTH - volumeSuffix.length()) + volumeSuffix;
}

protected String getContainerName() {
return getJobName();
}
Expand Down
34 changes: 34 additions & 0 deletions operator/src/main/java/oracle/kubernetes/operator/utils/ChecksumUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) 2021, Oracle and/or its affiliates.
// Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl.

package oracle.kubernetes.operator.utils;

import java.security.MessageDigest;
import javax.xml.bind.DatatypeConverter;

import oracle.kubernetes.operator.logging.LoggingFacade;
import oracle.kubernetes.operator.logging.LoggingFactory;
import oracle.kubernetes.operator.logging.MessageKeys;

public class ChecksumUtils {
private static final LoggingFacade LOGGER = LoggingFactory.getLogger("Operator", "Operator");

/**
* Gets the MD5 hash of a string.
*
* @param data input string
* @return MD5 hash value of the data, null in case of an exception.
*/
public static String getMD5Hash(String data) {
try {
return bytesToHex(MessageDigest.getInstance("MD5").digest(data.getBytes("UTF-8")));
} catch (Exception ex) {
LOGGER.severe(MessageKeys.EXCEPTION, ex);
return null;
}
}

private static String bytesToHex(byte[] hash) {
return DatatypeConverter.printHexBinary(hash).toLowerCase();
}
}
95 changes: 95 additions & 0 deletions operator/src/test/java/oracle/kubernetes/operator/helpers/JobHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
import io.kubernetes.client.openapi.models.V1PodTemplateSpec;
import io.kubernetes.client.openapi.models.V1SecurityContext;
import io.kubernetes.client.openapi.models.V1Toleration;
import io.kubernetes.client.openapi.models.V1Volume;
import io.kubernetes.client.openapi.models.V1VolumeMount;
import oracle.kubernetes.operator.DomainSourceType;
import oracle.kubernetes.operator.JobAwaiterStepFactory;
import oracle.kubernetes.operator.LabelConstants;
import oracle.kubernetes.operator.ProcessingConstants;
Expand Down Expand Up @@ -57,9 +59,11 @@
import static oracle.kubernetes.operator.DomainProcessorTestSetup.createTestDomain;
import static oracle.kubernetes.operator.ProcessingConstants.DOMAIN_TOPOLOGY;
import static oracle.kubernetes.operator.ProcessingConstants.JOBWATCHER_COMPONENT_NAME;
import static oracle.kubernetes.operator.helpers.Matchers.hasConfigMapVolume;
import static oracle.kubernetes.operator.helpers.Matchers.hasContainer;
import static oracle.kubernetes.operator.helpers.Matchers.hasEnvVar;
import static oracle.kubernetes.operator.helpers.Matchers.hasEnvVarRegEx;
import static oracle.kubernetes.operator.helpers.Matchers.hasSecretVolume;
import static oracle.kubernetes.operator.helpers.Matchers.hasVolumeMount;
import static oracle.kubernetes.operator.helpers.PodHelperTestBase.createAffinity;
import static oracle.kubernetes.operator.helpers.PodHelperTestBase.createConfigMapKeyRefEnvVar;
Expand All @@ -69,6 +73,7 @@
import static oracle.kubernetes.operator.helpers.PodHelperTestBase.createSecretKeyRefEnvVar;
import static oracle.kubernetes.operator.helpers.PodHelperTestBase.createSecurityContext;
import static oracle.kubernetes.operator.helpers.PodHelperTestBase.createToleration;
import static oracle.kubernetes.operator.utils.ChecksumUtils.getMD5Hash;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.allOf;
Expand All @@ -85,13 +90,29 @@
public class JobHelperTest extends DomainValidationBaseTest {
private static final String RAW_VALUE_1 = "find uid1 at $(DOMAIN_HOME)";
private static final String END_VALUE_1 = "find uid1 at /u01/oracle/user_projects/domains";
protected static final String LONG_RESOURCE_NAME
= "very-long-resource-name-very-long-resource-name-abcdefghi";
protected static final String SECOND_LONG_RESOURCE_NAME
= "very-long-resource-name-very-long-resource-name-abcdefghijklmnopqrstuvwxyz";

/**
* OEVN is the name of an env var that contains a comma-separated list of oper supplied env var names.
* It's used by the Model in Image introspector job to detect env var differences from the last
* time the job ran.
*/
private static final String OEVN = "OPERATOR_ENVVAR_NAMES";
public static final String SECRET_VOLUME_SUFFIX1 = "-volume-st-" + getMD5Hash(LONG_RESOURCE_NAME);
public static final String SECRET_VOLUME_SUFFIX2 = "-volume-st-" + getMD5Hash(SECOND_LONG_RESOURCE_NAME);
public static final String CM_VOLUME_SUFFIX1 = "-volume-cm-" + getMD5Hash(LONG_RESOURCE_NAME);
public static final int MAX_ALLOWED_VOLUME_NAME_LENGTH = 63;
public static final String VOLUME_NAME_FOR_LONG_SECRET_NAME = LONG_RESOURCE_NAME
.substring(0, MAX_ALLOWED_VOLUME_NAME_LENGTH - SECRET_VOLUME_SUFFIX1.length()) + SECRET_VOLUME_SUFFIX1;
public static final String VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME = SECOND_LONG_RESOURCE_NAME
.substring(0, MAX_ALLOWED_VOLUME_NAME_LENGTH - SECRET_VOLUME_SUFFIX2.length()) + SECRET_VOLUME_SUFFIX2;
public static final String VOLUME_NAME_FOR_LONG_CONFIG_MAP_NAME = LONG_RESOURCE_NAME
.substring(0, MAX_ALLOWED_VOLUME_NAME_LENGTH - SECRET_VOLUME_SUFFIX1.length()) + CM_VOLUME_SUFFIX1;
public static final int MODE_420 = 420;
public static final int MODE_365 = 365;
private Method getDomainSpec;
private final Domain domain = createTestDomain();
private final DomainPresenceInfo domainPresenceInfo = createDomainPresenceInfo(domain);
Expand Down Expand Up @@ -462,6 +483,14 @@ private List<V1VolumeMount> getJobVolumeMounts() {
.getVolumeMounts();
}

private List<V1Volume> getJobVolumes() {
return Optional.ofNullable(job.getSpec())
.map(V1JobSpec::getTemplate)
.map(V1PodTemplateSpec::getSpec)
.map(V1PodSpec::getVolumes)
.orElseThrow();
}

@Test
public void whenDomainHasAdditionalVolumesWithCustomVariables_createIntrospectorPodWithSubstitutions() {
resourceLookup.defineResource(SECRET_NAME, KubernetesResourceType.Secret, NS);
Expand Down Expand Up @@ -500,6 +529,72 @@ public void whenDomainHasAdditionalVolumesWithCustomVariablesInvalidValue_jobNot
assertThat(job, is(nullValue()));
}

@Test
public void whenDomainHasMultipleConfigOverrideSecretsWithLongNames_volumesCreatedWithShorterNames() {
resourceLookup.defineResource(LONG_RESOURCE_NAME, KubernetesResourceType.Secret, NS);
resourceLookup.defineResource(SECOND_LONG_RESOURCE_NAME, KubernetesResourceType.Secret, NS);

configureDomain()
.withConfigOverrideSecrets(LONG_RESOURCE_NAME, SECOND_LONG_RESOURCE_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_LONG_SECRET_NAME, LONG_RESOURCE_NAME, MODE_420));
assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME,
SECOND_LONG_RESOURCE_NAME, MODE_420));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + LONG_RESOURCE_NAME, true));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + SECOND_LONG_RESOURCE_NAME, true));
}

@Test
public void whenDomainHasConfigMapOverrideWithLongConfigMapName_volumeCreatedWithShorterName() {
resourceLookup.defineResource(LONG_RESOURCE_NAME, KubernetesResourceType.ConfigMap, NS);

configureDomain()
.withConfigOverrides(LONG_RESOURCE_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasConfigMapVolume(VOLUME_NAME_FOR_LONG_CONFIG_MAP_NAME, LONG_RESOURCE_NAME, MODE_365));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_CONFIG_MAP_NAME,
"/weblogic-operator/config-overrides", true));
}

@Test
public void whenDomainHasModelConfigMapOverrideWithLongModelCMName_volumeCreatedWithShorterName() {
resourceLookup.defineResource(LONG_RESOURCE_NAME, KubernetesResourceType.ConfigMap, NS);

configureDomain()
.withDomainHomeSourceType(DomainSourceType.FromModel)
.withModelConfigMap(LONG_RESOURCE_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasConfigMapVolume(VOLUME_NAME_FOR_LONG_CONFIG_MAP_NAME, LONG_RESOURCE_NAME, MODE_365));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_CONFIG_MAP_NAME,
"/weblogic-operator/wdt-config-map", true));
}

@Test
public void whenDomainHasMultipleConfigOverrideSecretsWithLongAndShortNames_volumeCreatedWithCorrectNames() {
resourceLookup.defineResource(SECRET_NAME, KubernetesResourceType.Secret, NS);
resourceLookup.defineResource(LONG_RESOURCE_NAME, KubernetesResourceType.Secret, NS);

configureDomain()
.withConfigOverrideSecrets(SECRET_NAME, LONG_RESOURCE_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasSecretVolume(SECRET_NAME + "-volume", SECRET_NAME, MODE_420));
assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_LONG_SECRET_NAME, LONG_RESOURCE_NAME, MODE_420));
assertThat(getJobVolumeMounts(), hasVolumeMount(SECRET_NAME + "-volume",
"/weblogic-operator/config-overrides-secrets/" + SECRET_NAME, true));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + LONG_RESOURCE_NAME, true));
}

@Test
public void verify_introspectorPodSpec_activeDeadlineSeconds_initial_values() {
V1JobSpec jobSpec = createJobSpec();
Expand Down
15 changes: 15 additions & 0 deletions operator/src/test/java/oracle/kubernetes/operator/helpers/Matchers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.kubernetes.client.openapi.models.V1HostPathVolumeSource;
import io.kubernetes.client.openapi.models.V1PersistentVolumeClaimVolumeSource;
import io.kubernetes.client.openapi.models.V1Probe;
import io.kubernetes.client.openapi.models.V1SecretVolumeSource;
import io.kubernetes.client.openapi.models.V1Volume;
import io.kubernetes.client.openapi.models.V1VolumeMount;
import org.hamcrest.Description;
Expand Down Expand Up @@ -68,10 +69,24 @@ static Matcher<Iterable<? super V1VolumeMount>> hasVolumeMount(String name, Stri
return hasItem(new V1VolumeMount().name(name).mountPath(path));
}

static Matcher<Iterable<? super V1VolumeMount>> hasVolumeMount(String name, String path, boolean readOnly) {
return hasItem(new V1VolumeMount().name(name).mountPath(path).readOnly(readOnly));
}

static Matcher<Iterable<? super V1Volume>> hasVolume(String name, String path) {
return hasItem(new V1Volume().name(name).hostPath(new V1HostPathVolumeSource().path(path)));
}

static Matcher<Iterable<? super V1Volume>> hasSecretVolume(String name, String secretName, Integer defaultMode) {
return hasItem(new V1Volume().name(name).secret(new V1SecretVolumeSource()
.secretName(secretName).defaultMode(defaultMode)));
}

static Matcher<Iterable<? super V1Volume>> hasConfigMapVolume(String name, String cmName, Integer defaultMode) {
return hasItem(new V1Volume().name(name).configMap(new V1ConfigMapVolumeSource().name(cmName)
.defaultMode(defaultMode)));
}

static Matcher<Iterable<? super V1Volume>> hasPvClaimVolume(String name, String claimName) {
return hasItem(new V1Volume().name(name).persistentVolumeClaim(
new V1PersistentVolumeClaimVolumeSource().claimName(claimName)));
Expand Down