Add external custom yaml for manifests

[pidah]

What this PR does / why we need it:
This PR adds an option to pass external custom YAML for manifest files

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #3906

[codecov]

Codecov Report

Merging #4092 into master will decrease coverage by 0.18%.
The diff coverage is 93.57%.

@@            Coverage Diff             @@
##           master    #4092      +/-   ##
==========================================
- Coverage   50.64%   50.45%   -0.19%     
==========================================
  Files         109      109              
  Lines       17020    16969      -51     
==========================================
- Hits         8619     8561      -58     
- Misses       7605     7611       +6     
- Partials      796      797       +1

[tariq1890]

@pidah Thanks so much for the PR.

Could you tell us why you'd like gzipped B64 data as opposed to just B64 (which is being done for Custom add-on scripts)?

Since we are enabling custom scripts for Manifests and Addons, we might as well include the K8s artifact files

[tariq1890]

A lot of duplication here. We can create a private method that performs the gzip and B64 validation and call that multiple times.

[tariq1890]

Can we also add unit tests?

[pidah]

thanks @tariq1890. I couldn't get B64 data to work with either the custom addon scripts or manifests. Perhaps I am missing something.

Yes I can create a private method and add unit tests.

[tariq1890]

Could you let us know the error you got when you tried custom add-on scripts? Feel free to open an issue for that as well.

[pidah]

Yes, I will build a cluster with current master and raise an issue with the error.

[pidah]

@tariq1890 I have raised an issue here: #4102

[tariq1890]

On second thought. Let's do away with this validation. This is an experimental feature and we are yet to actually decide a formalized UX for this.

[tariq1890]

Let's just use kubernetesFeatureSettings instead. Delete the kubernetesAddonSetting struct and add the rawScript field to kubernetesFeatureSettings.

[tariq1890]

I don't think we need this. We already have psp YAML files with placeholders. They should suffice as defaults

[tariq1890]

After we remove kubernetesAddonSetting we can just go back to using substituteConfigString. So the code to use the rawScript directly should be in substituteConfigString.

[tariq1890]

NIT: YAML* :P

[tariq1890]

@pidah Thanks again for the PR. I've added comments. Looks good so far :)

[pidah]

thanks @tariq1890. I have made the changes suggested.

[tariq1890]

@pidah Thanks once again for the PR. Looks good :). I've also add tests to validate the changes. They seem to be passing

[tariq1890]

@jackfrancis Requesting review

[pidah]

@tariq1890 thanks 👍

[jackfrancis]

@pidah Thanks! Sorry to add more work, but here's a suggestion: instead of a specific key under KubernetesConfig let's copy the existing addons interface and do something like the following:

"manifests": [
	        {
	          "name": "PodSecurityPolicy",
	          "data" : <base64 encoded data string>
	        }
	      ]

Each manifest that we enable in this way would be represented by a reserved string value (i.e., PodSecurityPolicy in the example above) and could be processed in the same way that addons are: if the api model has the manifest key w/ data in it, then use that, otherwise use the default provided under parts/k8s/manifests. We shouldn't have to change any of the last mile sed replacement foo, as it's effectively a no-op if the files don't match the substitution pattern.

@tariq1890 thoughts?

[pidah]

Hi @jackfrancis ! its do-able but I wonder if the API model might become a bit confusing. e.g for kube-scheduler manifest we could have something like:

"manifests": [ { "name": "Scheduler", "data" : <base64 encoded data string> } ]

and we currently set arbitrary scheduler config atm like this:

"kubernetesConfig": { "SchedulerConfig": { "data": <base64 encoded data string>, "xxx": "xxx" } }
just my 2cents

[jackfrancis]

@pidah It's true. I think what I'm suggesting is that the data input becomes a kind of "catch-all override". It's a higher order of precedence, in other words.

Because this is a kind of experimental feature (super easy to break your cluster) I think that's appropriate. The obvious use case (to me) for something like this is a way of performing brute force tests against a cluster; eventually those data overrides would be better implemented as targeted, higher level configuration, but it's a useful tool for iterating on novel configurations that don't aren't (yet) able to be represented conveniently in the api model.

[pidah]

hi @tariq1890 , any thoughts ?

[tariq1890]

@jackfrancis I am leaning more towards @pidah 's approach for now. I find it more intuitive to attach the raw script data directly to the SchedulerConfig struct as opposed to maintaining a separate struct of override scripts.

[jackfrancis]

I think I understand now. Does this PR support a data override of all currently supported component configs?

[tariq1890]

No. It does not support an override of all components (i.e. manifests and artifacts).

[jackfrancis]

The generic key/val config objects that already exist in kubernetesConfig are:

• kubeletConfig
• controllerManagerConfig
• cloudControllerManagerConfig
• apiServerConfig
• schedulerConfig

[tariq1890]

Yes. In this PR the podSecurityPolicyConfig has been added to the K8sConfig struct as that was not supported earlier.

[jackfrancis]

/lgtm pending E2E

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, pidah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jackfrancis]

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

[jackfrancis]

rebased to attempt to get rid of Windows E2E flakes