Preserve pod SHA256 values

[russgold]

This changes verifies some cases of SHA256 hash computations and ensures that they don't change over time. The hash values used as references are those generated by operator version 3.1.0.

[doxiao]

This line may cause a NPE on unboxing a null Integer if both getListenPort() and getSslPort() is null in getMetricsPort().

[russgold]

True - of course, such a domain would be pretty worthless. IMO the logic is wrong in any case, but I didn't want to change it here - all I have done is refactored the existing logic.

[doxiao]

Operator definitely hit this problem without Johnny's fix in PR #2256 with secure mode enabled but SSL is not configured. Johnny's changes tire up the loose ends in some of the configuration permutations. I am not sure if there will be any configuration that may hit this even with PR #2256.

[doxiao]

I don't like adding defensive code for invalid configurations, but the ugly part of this NPE is that when it happens it is hard to figure out what is going on. It would be helpful if we could at least detect the situation and log a warning message, instead of letting NPE occur.

[russgold]

I've added a change that should catch that.

[doxiao]

I am not 100% sure if the NPE will happen on return or on the method that calls this, although I would think it is latter. It is probably safer to catch the exception on the caller, or assign the port to a variable in this method, and check that.

[russgold]

It should happen on the unboxing - so I should ensure that happens here.

[russgold]

No, even that doesn't help. That kind of domain error causes problems all over the place. If you catch it here, it pops up elsewhere. This PR is not the place to fix that preexisting problem, I think. The code here does not cause the problem, and fixing it will require more effort and is out of scope. I am going to revert my last change.

[doxiao]

I wrote a simple class to test this, and unboxing occurred on the method call where null Integer value is assigned to an int parameter. In other words it does not happen in the return statement here and the try/catch should be on the calling method.

[doxiao]

public class Test {
public static void main(String[] args) {
try {
printValue(getValue());
} catch (Throwable t) {
System.out.println("t2 = " + t);
}
}

static Integer getValue() {
Integer value = null;
try {
return value;
} catch (Throwable t) {
System.out.println("t = " + t);
throw t;
}
}

static void printValue(int i) {
System.out.println("i = " + i);
}
}

[doxiao]

And the output is

t2 = java.lang.NullPointerException

[doxiao]

It is probably better to use a local variable to check the value. If it is null, log and return zero.

[russgold]

I can reproduce the problem. I can also reproduce it before this change. I can prevent getMetricsPort() from returning a null, which causes the problem later - and then the NPE happens in getLocalAdminProtocolChannelPort.

I agree that there is a problem - but that problem has nothing to do with the PR, and should be addressed elsewhere. Could you please file a bug report or a GitHub issue so we can track it?

The basic problem is the logic by which we select the port for admin protocol. It should be done in one place, and it is done in multiple places. Worse, a change was made in 3.1.4 to the logic now served by getMetricsPort which cannot be changed, as it would cause pods to roll on upgrade if we do.

The best thing would probably be to catch the error in the domain long before we get here.

[doxiao]

I agree with you that the logic of choosing a port to use is not clean and needs to be revisited.

I was glad that this particular section was removed by a previous PR, and was just concerned that this code is now added back (to prevent pods to roll) so the issue came back.

We can certainly handle this in a different jira.

[doxiao]

Just to complete the current discussion. It is pretty rare that getLocalAdminProtocolChannelPort returns null since it looks into every port that is configured (admin port, listen port, SSL port, and NAPs). But getMetricsPort only looks for listen port and SSL port.

[doxiao]

LGTM. We can track the NPE on unboxing and revisiting the logic of choosing a port for Prometheus in a different jira task.