marking crd fields as required #345

shawkins · 2021-05-13T13:10:49Z

This is for https://issues.redhat.com/browse/MGDSTRM-3288

Based upon our usage, this appears the set of fields / objects that are required. In some cases not having the field will produce NPEs - such as when the spec is missing. In other cases there's just no explicit null check and a null value could be used directly, like in the oauth properties.

Please review this so that we can identify which fields should have default handling and/or aren't actually required.

Note that due to fabric8io/kubernetes-client#3096 we can't yet mark the spec as required.

cc @rareddy @ppatierno @k-wall

api/src/main/java/org/bf2/operator/resources/v1alpha1/ObservabilityConfiguration.java

MikeEdgar · 2021-05-13T13:55:16Z

Does it make sense to use @NotBlank for fields that require a non-empty, non-whitespace value or would that be a future task?

shawkins · 2021-05-13T14:00:22Z

Does it make sense to use @notblank for fields that require a non-empty, non-whitespace value or would that be a future task?

It could from an api standpoint, but it will have no effect on the generated crd nor the json/yaml parsing I believe.

rareddy

lgtm

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAgentSpec.java

shawkins · 2021-05-13T17:56:50Z

Keep in mind that if we mark a field as required and a cr already exists that does not have that field, it won't update properly - as it will fail validation.

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java

api/src/main/java/org/bf2/operator/resources/v1alpha1/Versions.java

shawkins · 2021-05-14T13:12:13Z

This next commit has a refinement of the spec and spec capacity handling - where not null is enforced in the api, rather than the crd. This requires no changes in changes in our existing logic which is not appropriately checking for null. That is another route to consider - but without additional logic it does mean some oddities in how things would look when created via the api:

ManagedKafka mk = new ... // without a spec
client...create(mk);

that would actually serialize to

spec:
  capacity: {}

Rather than omitting it.

shawkins · 2021-05-18T13:56:11Z

This next commit further refines things - it removes the not null from most of the oauth fields and converts the config kafkacluster config properties to be based upon the cr instead.

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java

MikeEdgar

Looks good to me

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java

shawkins · 2021-06-01T15:20:32Z

Tbh based on the chat conversation I would leave all auth fields not required but fixing the potential NPEs.

Since that now has potential conflicts are you suggesting that we continue to wait on this pr until after you merge. Or can this be merged and then we fix up everything in your pr?

The Auth part has already caused a lot of problems to us and marking them required when they are not (because not clear) could cause more. Wdyt?

I'm fine with any resolution...

ppatierno · 2021-06-01T15:29:44Z

where do you see conflicts with my PR? I see just a "logical" one about tls trusted cert that is NOT required.

shawkins · 2021-06-01T15:36:15Z

There is a conflict with SecretSecurityManager.

ppatierno · 2021-06-01T15:40:03Z

Since that now has potential conflicts are you suggesting that we continue to wait on this pr until after you merge.

No you can merge after approvals, I can fix conflicts on my side but my point was about removing the @NotNull from the tls trusted certs. It's not related to the conflict.

shawkins · 2021-06-01T15:44:45Z

No you can merge after approvals, I can fix conflicts on my side but my point was about removing the @NotNull from the tls trusted certs. It's not related to the conflict.

I'm not trying to be difficult, but I would leave this pr as self-consistent. If I remove the NotNull from tls, that's based upon a change that has not been merged. If this pr is merged first, you can remove the NotNull. If yours is merged first, I'll remove the NotNull here. There's no need to proactively intermingle the changes even more.

ppatierno · 2021-06-01T15:50:25Z

I'm not trying to be difficult

Same here :-) ... but why adding a NotNull that hasn't been there since today and that we already know will be removed tomorrow?
Anyway I can remove it of course if this will be merged first.

ppatierno · 2021-06-01T16:56:51Z

@shawkins I have just merged my PR, I think we now agree we need to remove @NotNull on the TLS trusted certificates. Sorry, I rushed because tomorrow I am on PTO ;-)

shawkins · 2021-06-01T17:06:17Z

I have just merged my PR, I think we now agree we need to remove @NotNull on the TLS trusted certificates. Sorry, I rushed because tomorrow I am on PTO ;-)

In general we need to make sure that we don't let prs sit like this - it's bad for productivity. I think in the future if concerns aren't actionable within a couple of days, we should assume that we can move forward based upon 2 other approvals.

ppatierno · 2021-06-01T17:13:27Z

I agree, but we spent more time waiting for the people knowing the oauth business more than us, even if in the end the information wasn't so useful :-(

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafka.java

api/src/main/java/org/bf2/operator/resources/v1alpha1/Versions.java

operator/src/main/java/org/bf2/operator/secrets/SecuritySecretManager.java

ppatierno

LGTM. Let's just wait for my doubt/question for @tomncooper

generally this guards against unexpected npes due to a fabric8 bug, we can't mark the specs as requires, so they will always be there based upon the api switching kafka.authentication.enabled and kafka.external.certificate.enabled to be based upon the cr, rather than the config

MikeEdgar · 2021-06-04T15:07:33Z

operator/src/main/java/org/bf2/operator/secrets/SecuritySecretManager.java

-    @ConfigProperty(name = "kafka.authentication.enabled", defaultValue = "false")
-    private boolean isKafkaAuthenticationEnabled;
+    public static boolean isKafkaAuthenticationEnabled(ManagedKafka managedKafka) {
+        return managedKafka.getSpec().getOauth() != null;
+    }

-    @ConfigProperty(name = "kafka.external.certificate.enabled", defaultValue = "false")
-    private boolean isKafkaExternalCertificateEnabled;
+    public static boolean isKafkaExternalCertificateEnabled(ManagedKafka managedKafka) {
+        return managedKafka.getSpec().getEndpoint().getTls() != null;
+    }


@shawkins , @k-wall - it looks like this change is going to give us a short-term headache with the development installer script. Fleet manager always sends the tls entry, but with dummy values in the dev installation process. I was relying on the config property to force TLS to be disabled.

shawkins added the DO NOT MERGE label May 13, 2021

rareddy reviewed May 13, 2021

View reviewed changes

api/src/main/java/org/bf2/operator/resources/v1alpha1/ObservabilityConfiguration.java Show resolved Hide resolved

rareddy approved these changes May 13, 2021

View reviewed changes

ppatierno reviewed May 13, 2021

View reviewed changes

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAgentSpec.java Show resolved Hide resolved

ppatierno reviewed May 14, 2021

View reviewed changes

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java Outdated Show resolved Hide resolved

api/src/main/java/org/bf2/operator/resources/v1alpha1/Versions.java Show resolved Hide resolved

shawkins requested a review from ppatierno May 18, 2021 14:05

shawkins removed the DO NOT MERGE label May 18, 2021

k-wall reviewed May 18, 2021

View reviewed changes

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java Show resolved Hide resolved

MikeEdgar approved these changes May 19, 2021

View reviewed changes

ppatierno reviewed Jun 1, 2021

View reviewed changes

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java Outdated Show resolved Hide resolved

api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafkaAuthenticationOAuth.java Show resolved Hide resolved

ppatierno added this to the 0.5.0 milestone Jun 1, 2021

shawkins requested a review from k-wall June 1, 2021 17:38

ppatierno requested changes Jun 3, 2021

View reviewed changes

ppatierno approved these changes Jun 3, 2021

View reviewed changes

shawkins merged commit 35d9011 into bf2fc6cc711aee1a0c2a:main Jun 3, 2021

MikeEdgar reviewed Jun 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

marking crd fields as required #345

marking crd fields as required #345

shawkins commented May 13, 2021

MikeEdgar commented May 13, 2021

shawkins commented May 13, 2021

rareddy left a comment

shawkins commented May 13, 2021

shawkins commented May 14, 2021

shawkins commented May 18, 2021

MikeEdgar left a comment

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

ppatierno left a comment

MikeEdgar Jun 4, 2021

marking crd fields as required #345

marking crd fields as required #345

Conversation

shawkins commented May 13, 2021

MikeEdgar commented May 13, 2021

shawkins commented May 13, 2021

rareddy left a comment

Choose a reason for hiding this comment

shawkins commented May 13, 2021

shawkins commented May 14, 2021

shawkins commented May 18, 2021

MikeEdgar left a comment

Choose a reason for hiding this comment

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

ppatierno commented Jun 1, 2021

shawkins commented Jun 1, 2021

ppatierno commented Jun 1, 2021

ppatierno left a comment

Choose a reason for hiding this comment

MikeEdgar Jun 4, 2021

Choose a reason for hiding this comment