Skip to content

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

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

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 4 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
30 changes: 24 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 @@ -9,6 +9,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicInteger;

import io.kubernetes.client.openapi.models.V1ConfigMapVolumeSource;
import io.kubernetes.client.openapi.models.V1Container;
Expand Down Expand Up @@ -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;
private final AtomicInteger volumeIndex = new AtomicInteger(1);
private final AtomicInteger mountIndex = new AtomicInteger(1);
public static final String VOLUME_NAME_SUFFIX = "-volume";
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(getOverrideSecretVolumeSource(secretName)));
}

private void addConfigOverrideVolume(V1PodSpec podSpec, String configOverrides) {
podSpec.addVolumesItem(
new V1Volume()
.name(configOverrides + "-volume")
.name(configOverrides + VOLUME_NAME_SUFFIX)
Copy link
Member

@rjeberhard rjeberhard Mar 11, 2021

Choose a reason for hiding this comment

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

In the spirit of fixing all of the related problems, I think we should make the same change to volume items that include the name of a config map as well. Config maps are another example of a resource that can have names longer than 63 characters.

Let's please look at PodHelper/PodStepContext too to see if we automatically generate volume/volumeMount names based on resource names.

Copy link
Member Author

@ankedia ankedia Mar 11, 2021

Choose a reason for hiding this comment

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

Done. I looked PodHelper/PodStepContext but didn't find any place where we automatically generate volume/volumeMount names based on resource names.

.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(configMapName + VOLUME_NAME_SUFFIX)
.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(getConfigOverrides() + VOLUME_NAME_SUFFIX, OVERRIDES_CM_MOUNT_PATH));
}

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

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

private String getVolumeName(String secretName) {
return getName(secretName, volumeIndex) + VOLUME_NAME_SUFFIX;
}

private String getVolumeMountName(String secretName) {
return getName(secretName, mountIndex) + VOLUME_NAME_SUFFIX;
}

private String getName(String secretName, AtomicInteger index) {
return secretName.length() > (MAX_ALLOWED_VOLUME_NAME_LENGTH - VOLUME_NAME_SUFFIX.length())
? "long-secret-name-" + index.getAndIncrement() : secretName;
Copy link
Member

@rjeberhard rjeberhard Mar 11, 2021

Choose a reason for hiding this comment

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

Instead of "long-secret-name-" can we please use something simpler like "volume-" or as much of the secretName that we have room for (eliminating the end of the name).

Copy link
Member Author

@ankedia ankedia Mar 11, 2021

Choose a reason for hiding this comment

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

We suffix the volume names with "-volume" and I thought to keep the same suffix for the secrets volume names where the name exceeds 63 characters limit (for consistency). I will take the first 50 characters of the secret name and then leave the remaining characters for the ordinal/index of the volume in case there are multiple secrets having the same first 50 characters.

Copy link
Contributor

@russgold russgold Mar 11, 2021

Choose a reason for hiding this comment

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

And you have to make sure that modifying the domain doesn't create a different set up mappings based on the order in which the secret/volume names are shortened. One way to do that might be to compute a checksum for the original name, and then use that as part of the generated name, rather than an index.

Copy link
Member Author

@ankedia ankedia Mar 11, 2021

Choose a reason for hiding this comment

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

I have made the change to compute a checksum for the original name and use that as part of the generated name.

}

protected String getContainerName() {
return getJobName();
}
Expand Down
68 changes: 68 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,6 +27,7 @@
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.JobAwaiterStepFactory;
import oracle.kubernetes.operator.LabelConstants;
Expand Down Expand Up @@ -60,6 +61,7 @@
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 @@ -85,13 +87,20 @@
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_SECRET_NAME
= "very-long-secret-name-very-long-secret-name-abcdefghijklm";
protected static final String SECOND_LONG_SECRET_NAME
= "second-very-long-secret-name-very-long-secret-name-very-long-secret-name";

/**
* 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 VOLUME_NAME_FOR_LONG_SECRET_NAME = "long-secret-name-1-volume";
public static final String VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME = "long-secret-name-2-volume";
public static final int DEFAULT_MODE = 420;
private Method getDomainSpec;
private final Domain domain = createTestDomain();
private final DomainPresenceInfo domainPresenceInfo = createDomainPresenceInfo(domain);
Expand Down Expand Up @@ -462,6 +471,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 +517,57 @@ public void whenDomainHasAdditionalVolumesWithCustomVariablesInvalidValue_jobNot
assertThat(job, is(nullValue()));
}

@Test
public void whenDomainHasConfigOverrideSecretsWithLongName_volumeCreatedWithShorterName() {
resourceLookup.defineResource(LONG_SECRET_NAME, KubernetesResourceType.Secret, NS);

configureDomain()
.withConfigOverrideSecrets(LONG_SECRET_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_LONG_SECRET_NAME, LONG_SECRET_NAME, DEFAULT_MODE));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + LONG_SECRET_NAME, true));
}

@Test
public void whenDomainHasMultipleConfigOverrideSecretsWithLongNames_volumeCreatedWithShorterNames() {
resourceLookup.defineResource(LONG_SECRET_NAME, KubernetesResourceType.Secret, NS);
resourceLookup.defineResource(SECOND_LONG_SECRET_NAME, KubernetesResourceType.Secret, NS);
Copy link
Member

@jshum2479 jshum2479 Mar 11, 2021

Choose a reason for hiding this comment

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

Probably want to add multiple long named secrets where the first 63 characters are the same.

Copy link
Member Author

@ankedia ankedia Mar 11, 2021

Choose a reason for hiding this comment

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

Done


configureDomain()
.withConfigOverrideSecrets(LONG_SECRET_NAME, SECOND_LONG_SECRET_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_LONG_SECRET_NAME, LONG_SECRET_NAME, DEFAULT_MODE));
assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME,
SECOND_LONG_SECRET_NAME, DEFAULT_MODE));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + LONG_SECRET_NAME, true));
assertThat(getJobVolumeMounts(), hasVolumeMount(VOLUME_NAME_FOR_SECOND_LONG_SECRET_NAME,
"/weblogic-operator/config-overrides-secrets/" + SECOND_LONG_SECRET_NAME, true));
}

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

configureDomain()
.withConfigOverrideSecrets(SECRET_NAME, LONG_SECRET_NAME);

runCreateJob();

assertThat(getJobVolumes(), hasSecretVolume(SECRET_NAME + "-volume", SECRET_NAME, DEFAULT_MODE));
assertThat(getJobVolumes(), hasSecretVolume(VOLUME_NAME_FOR_LONG_SECRET_NAME, LONG_SECRET_NAME, DEFAULT_MODE));
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_SECRET_NAME, true));
}

@Test
public void verify_introspectorPodSpec_activeDeadlineSeconds_initial_values() {
V1JobSpec jobSpec = createJobSpec();
Expand Down
10 changes: 10 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,19 @@ 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>> hasPvClaimVolume(String name, String claimName) {
return hasItem(new V1Volume().name(name).persistentVolumeClaim(
new V1PersistentVolumeClaimVolumeSource().claimName(claimName)));
Expand Down