[#981] Avoid using JAVA_ARGS_APPEND for internal configuration

[gaohoward]

This PR add tests on how to properly append to JAVA_ARGS_APPEND env var

[gaohoward]

(Test is supposed to fail at the moment until the dependent PR is merged)

[gtully]

In some cases we want to split, but in general the intent is that we use JDK_JAVA_ARGS and we append to any existing value, and JAVA_ARGS_APPEND is a way to override as a duplicate command line argument. They are env vars that are at either end of the java command line.

The user should always have control to override if necessary, it is like a build in escape hatch.

But the append depends on env values. However we can combine, b/c env vars can be dependent.

If the user wants an env var from source, that can be separate and included in the env vars we care about.

I tried a little test of this:

crd.Spec.Env = []corev1.EnvVar{
				{
					Name: "ENV_FROM_X",
					ValueFrom: &corev1.EnvVarSource{
						SecretKeyRef: &corev1.SecretKeySelector{
							LocalObjectReference: corev1.LocalObjectReference{Name: secret.Name},
							Key:                  "envKey",
						},
					}},
				{Name: "JAVA_ARGS_APPEND", Value: "-Dhawtio.realm=console $(ENV_FROM_X)"},
			}

and it works as expected, $(ENV_FROM_X) gets expanded into the value of the envKey from the secret just fine.

[gaohoward]

In some cases we want to split, but in general the intent is that we use JDK_JAVA_ARGS and we append to any existing value, and JAVA_ARGS_APPEND is a way to override as a duplicate command line argument. They are env vars that are at either end of the java command line.

The user should always have control to override if necessary, it is like a build in escape hatch.

But the append depends on env values. However we can combine, b/c env vars can be dependent.

If the user wants an env var from source, that can be separate and included in the env vars we care about.

I tried a little test of this:

crd.Spec.Env = []corev1.EnvVar{
				{
					Name: "ENV_FROM_X",
					ValueFrom: &corev1.EnvVarSource{
						SecretKeyRef: &corev1.SecretKeySelector{
							LocalObjectReference: corev1.LocalObjectReference{Name: secret.Name},
							Key:                  "envKey",
						},
					}},
				{Name: "JAVA_ARGS_APPEND", Value: "-Dhawtio.realm=console $(ENV_FROM_X)"},
			}

and it works as expected, $(ENV_FROM_X) gets expanded into the value of the envKey from the secret just fine.

That makes sense. Thanks!

[gaohoward]

@gtully test updated. please review. Thanks!

[gtully]

I think this is the right approach.

Did you consider doing this check at the validation phase?
I imagine if valueFrom and value are defined that creating the stateful set will fail/error which is very similar. ie: a deployment failure.
If we identify the env vars we modify, in some internal collection - we already use createorappend for those in the code, then we can trap a validation error if any env var matches that set and has valueFrom set.
if that set ever gets out of sync, the stateful set error will be a fallback.
I think it makes sense for us to document or callout the env vars we modify or want to modify. The set should be small.
in the error message we should point to using dependent variables as the desired approach. It can be very specific, and can be very specific, in a validation failure condition/error.

[gaohoward]

validation phase?

I think this is the right approach.

Did you consider doing this check at the validation phase? I imagine if valueFrom and value are defined that creating the stateful set will fail/error which is very similar. ie: a deployment failure. If we identify the env vars we modify, in some internal collection - we already use createorappend for those in the code, then we can trap a validation error if any env var matches that set and has valueFrom set. if that set ever gets out of sync, the stateful set error will be a fallback. I think it makes sense for us to document or callout the env vars we modify or want to modify. The set should be small. in the error message we should point to using dependent variables as the desired approach. It can be very specific, and can be very specific, in a validation failure condition/error.

Yeah I think doing it at validation makes sense. I'll update and add a document about the env vars the operator uses internally. Thanks!

[gaohoward]

@gtully PR updated. Now

• check valueFrom at validation phase
• document internal used env vars