gaborgsomogyi commented on PR #689: URL: https://github.com/apache/flink-kubernetes-operator/pull/689#issuecomment-1795575344
Please see my comments inline: > So the problem is that user declared flinkdeployments could specify the cert directory wherever they want to, I didn't want to lock down the directory to a known location. As this location could be anywhere, just using the same location is therefore not possible. Also as our flink operator containers are root read-only mounted I decided to put them in /tmp and provide a naming convention to ensure multiple flinkdeployments would not overwrite the same file. I think the operator can use a single root CA as truststore generated per k8s cluster (or some tiny changes in the granularity) and the workloads can use this root CA as signing CA for keystores. > I doubt the operator would be managing 100's of deployments let another orders of magnitude greater than that. I also wasn't sure about permanently persisting them as if the cert is rotated I would need a mechanism to renew the cert, with this method you just need to roll the pod to refresh the certs Doubt or not at big companies the operator is handling extreme amount of jobs so I'm not convinced with the actual proposal in this area. > I only wanted to change the config if I have correctly found and created the file, hence setting it in the createLocalFile method. This way if users are doing a different way to mount their certs I wouldn't be overriding them I'm speaking about structural code patterns and not concrete things. To be crystal clear when I see a function called `createLocalFile` then I expect such function to do exactly one thing, which is creating a local file based on parameters and nothing else. I know why the config is changed but that must be done in another place of the code or the function can be renamed. This applies to all code parts. > The operator will need to use the restclient for many of its functions i.e. listJobs, cancelJobs, triggerSavepoints e.t.c. without the correct ssl the operator won't be able to do this. I have provided a sample, why don't you try it on the current codebase unless I'm misunderstanding your question Please provide me the line where your new test code asserts on that the REST client is using SSL and then we're fine. > I will tidy up the tests but would like confirmation that the methodology I'm using is acceptable to the community Not sure what you mean `methodology I'm using`. What I suggest is to add automated tests for each added functionality including positive and negative test cases too. I've not checked the coverage provided by the tests but I'm pretty sure copy-paste is a no go. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
