[Federation][Kubefed] Bug fix to enable disabling federation controllers through override args

[irfanurrehman]

Targets #42761

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

Special notes for your reviewer:
@marun @perotinus @nikhiljindal

Release note:

NONE 

[k8s-reviewable]

This change is 

[irfanurrehman]

@k8s-bot cvm gce e2e test this

[perotinus]

I'm not super enthused that this could break people's scripted usages of kubefed, since it's now in beta. That said, I'm not sure that I can see a better way to do this, except revamping the whole system to use some sort of configuration file rather than command-line parameters. Which, incidentally, is how kubeadm does this: https://kubernetes.io/docs/admin/kubeadm/#config-file

I'm wondering if we should move from a flag to doing this the way kubeadm does. Of course, this is complicated by the fact that kubefed is in beta now.

[marun]

Is this potential complexity worth it? Or might it be preferable to expose any multivariate configuration option for apiserver or controllermanager as a first-class argument to kubefed init?

[madhusudancs]

I'm not super enthused that this could break people's scripted usages of kubefed, since it's now in beta. That said, I'm not sure that I can see a better way to do this, except revamping the whole system to use some sort of configuration file rather than command-line parameters. Which, incidentally, is how kubeadm does this: https://kubernetes.io/docs/admin/kubeadm/#config-file

I'm wondering if we should move from a flag to doing this the way kubeadm does. Of course, this is complicated by the fact that kubefed is in beta now.

Yes. We have discussed this before. Someone should bite the bullet. But note that we want to go the ComponentConfig route. SIG-node and SIG-scheduling are working on that. So it might be worth discussing with them. I had a chat with @mtaufen long back. I don't know the current status though.

Is this potential complexity worth it? Or might it be preferable to expose any multivariate configuration option for apiserver or controllermanager as a first-class argument to kubefed init?

How is that less complex?

[irfanurrehman]

Should I limit this PR to only update the marshaling of arguments such that = in the values don't break us?
It would at least be possible to disable one controller in near term?

[madhusudancs]

Yeah, I think we should do that to begin with and cherry-pick that to v1.6.x branch before we resolve the other issues with syntax changes for 1.7

[irfanurrehman]

updated, PTAL!

[marun]

How is that less complex?

More complex in terms of UX, not implementation. I think it would be simpler for the user to provide a first-class argument to kubefed vs having to remember kubefed-specific formatting for multivalued arguments.

[madhusudancs]

More complex in terms of UX, not implementation. I think it would be simpler for the user to provide a first-class argument to kubefed vs having to remember kubefed-specific formatting for multivalued arguments.

There are pros and cons. There are the advantages that you mention here. But on the other hand

1. Every user, even the ones who don't care about these flags, has to sift through the ton of documentation that is generated for these flags.
2. Each such flag has to be dealt on a case-by-case basis. If we miss a flag in a release, there will be a window where users will have to wait for that flag to be available in kubefed.
3. Adds to the confusion. There are now multiple ways of providing override args.
4. Name collision. You might have a flag with a same name for different components that mean different things. Now you need to namespace them or qualify them with the component name. In either case those names are not exactly the same as the one taken by the components and users need to learn this.

[perotinus]

What's here LGTM, for the issue with marshaling multiple =. Perhaps we should continue any discussion about the higher-level problem in #42761?

[madhusudancs]

Perhaps we should continue any discussion about the higher-level problem in #42761?

Agreed.

[madhusudancs]

/lgtm
/approve

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, madhusudancs

Needs approval from an approver in each of these OWNERS Files:

• federation/OWNERS [madhusudancs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[madhusudancs]

@irfanurrehman please also cherry-pick this to release-1.6 branch.

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 44286, 44209)