Make default ingress tls and annotations congurable in the helm config

[Tom-Newton]

Purpose of this PR

Support helm chart level defaults for ingress TLS and annotations, so they don't need to be configured on every SparkApplication.
Closes: #2460

Proposed changes:

• Add CLI arguments --default-ingress-tls and --default-ingress-annotations and use them when TLS/annotations are not provided in the SparkApplication's spec. These are provided as JSON strings in the CLI.
• Expose these via the helm config. The user configures them as normal in the helm yaml and they get serialised to JSON, so they can be read by the controller.
• Wrote unit tests.

Change Category

• Bugfix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that could affect existing functionality)
• Documentation update

Rationale

Checklist

• I have conducted a self-review of my own code.
• I have updated documentation accordingly.
• I have added tests that prove my changes are effective or that my feature works.
• Existing unit tests pass locally with my changes.

Additional Notes

I considered adding an e2e test to cover the helm and CLI configuration. However, I couldn't see a way to run the e2e tests with multiple helm configs. I could have changed all the e2e tests to use these new configs, but then we loose test coverage for when the new configs are not used.

I'm a golang noob.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mwielgus for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[google-oss-prow]

@thetaiter-swf: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Tom-Newton]

@ChenYi015 sorry for the direct ping, but you helped me get a couple of other things merged. I was hoping you could review this, when you get a chance.

[nabuskey]

I wonder if defining default TLS settings with JSON is the way to go. Probably not a bad experience if using Helm but I don't really like to deal with escaping JSON strings in general.

[Tom-Newton]

I wonder if defining default TLS settings with JSON is the way to go. Probably not a bad experience if using Helm but I don't really like to deal with escaping JSON strings in general.

Thanks for reviewing. I think that is a fair assesment, but I can't think of any better options... Do you have any suggestions?

[nabuskey]

I wonder if defining default TLS settings with JSON is the way to go. Probably not a bad experience if using Helm but I don't really like to deal with escaping JSON strings in general.

Thanks for reviewing. I think that is a fair assesment, but I can't think of any better options... Do you have any suggestions?

Hmm maybe read from a file instead, likely from CMs. I'd say adding support YAML and JSON is a good idea too.

I'd like to hear others' opinions on this though. I do love this feature proposed here and would love to see it implemented.

[Tom-Newton]

Hmm maybe read from a file instead, likely from CMs. I'd say adding support YAML and JSON is a good idea too.

I've been thinking a bit about how to implement this. It does add a bit of complexity and I'm not decided how to organise things. It feels a bit strange to mix with some config provided via config maps and some via CLI arguments.

I can implement it with config maps if anyone has strong opinions, but I think my preference would be to leave it as is for now. If we want to change to using config maps, I would propose that as a seperate PR to do a full migration to config maps.

[nabuskey]

It feels a bit strange to mix with some config provided via config maps and some via CLI arguments.

Agreed on this but I honestly I am not sure if escaped json string is good either. I'd love to hear others' opinions and get this implemented.

@ChenYi015 @jacobsalway @vara-bonthu Opinions on this?