readOnly for ConfigMaps

[AdheipSingh]

Description

• Currently configMaps mounted for common, runtime are read-write (default).
• readOnly when set to true, cm shall be mounted as readOnly

---

This PR has:

• [x ] been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
• [x ] been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• [x ] added documentation for new or modified features or behaviors.

---

Key changed/added files in this PR

• handler.go
• types.go

[AdheipSingh]

@himanshug Can we run the CI and test on microk8s in travis. Maybe use helm to bootstrap the CR and Operator.

[himanshug]

I think we should just make them "always read-only" ... that option doesn't need to exist. please remove the option and make it always read-only unless you know of a use case that requires writing into mounted config, I think we should never need to write into that.

[himanshug]

@himanshug Can we run the CI and test on microk8s in travis. Maybe use helm to bootstrap the CR and Operator.

can you please create a proposal around the benefits of doing that, then feel free to work on it. only request is to retain the existing things that build does.
I'm not sure how stable microk8s and travis integration is, but no harm in trying out.

[AdheipSingh]

yeah sure i ll create proposal around the travis, and send a workaround propsal. Plus its great to have readonly set to true.

[AdheipSingh]

@himanshug i am looking forward for some help around fixing the Unit tests. Any suggestion

[himanshug]

you definitely need to update broker-statefulset.yaml , however I tried this patch with that file updated and test still failed. at this point it looks to me like reflect.DeepEquals(..) has some serious bug but I will check more later.

[AdheipSingh]

@himanshug
I was looking at these units tests, plan to use them in locally. Its uses a fake client.
Plus i like to test a POC kind of workflow for CI-CD, were test this operator creation against actual k8s cluster. Will check that soon.

Do tell your thoughts on this unit tests.
https://sdk.operatorframework.io/docs/golang/unit-testing/

[AdheipSingh]

you definitely need to update broker-statefulset.yaml , however I tried this patch with that file updated and test still failed. at this point it looks to me like reflect.DeepEquals(..) has some serious bug but I will check more later.

reflect.DeepEquals is not the issue, the way the k8s object is coming out is the issue as i see. Its a notion of a three way merge original, current, and updated.
ill look for a different approach to fix it.

[AdheipSingh]

i guess we need to use this library. Its addressing the same issue.

@himanshug

[himanshug]

I am still not sure what causes reflect.DeepEquals to fail. Sounds like you understood what exactly is the problem, can you describe that?

no problems using the proposed library as long as we understand the problem with reflect.DeepEquals.

[AdheipSingh]

so if i log the actual, existing objects the only difference i found was in the hash annotation.

If i pass in existed.Spec and actual.Spec they pass without any error. So the only difference was in the hash object. I dont know how was it passing the test earlier.

Regarding the libraries, i searched kubernetes issues and found some identical problems during spec matching
kubernetes-sigs/kubebuilder#592 (comment)
and the main motivation for this library describes the same issue
https://github.com/banzaicloud/k8s-objectmatcher#how-does-it-work

plus if i remove the empty interface{} in the assertion function and pass *appsv1.StatefulSet it will still pass without any error.

i had faced the same issues when i earlier tried to map the difference in objects, so would map each spec, template, like here
https://github.com/BinaryOmen/druid-operator/blob/master/pkg/sync/sync.go#L11 , empty interface{} dint work here too

I am still confused though how was it working earlier, we never had the same hash. :(
@himanshug

[AdheipSingh]

@himanshug once you approve here, ill do changes #52 . Then cut out a new release.

Thanks