Update the GCP provider to allow users to skip firewall actions

[barbacbd]

cluster:
Update the scripts to include the new variables

providers/gce:

Update the config to include the new ManageFirewallRules boolean setting. This variable will allow users to skip the creation, deletion, and updates to firewall rules when set to false. Users may not want or have the ability to add the permissions to perform these actions on their service account. When this is the case the firewall rules should be pre created and managed by someone with permissions to achieve the same goal.

[k8s-ci-robot]

This issue is currently awaiting triage.

If the repository mantainers determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[k8s-ci-robot]

Hi @barbacbd. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

[theobarberbany]

/ok-to-test

[bowei]

/assign

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: barbacbd
Once this PR has been reviewed and has the lgtm label, please ask for approval from bowei. For more information see the 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:

• cluster/OWNERS
• providers/gce/OWNERS

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

[JoelSpeed]

/lgtm

[theobarberbany]

/lgtm

[patrickdillon]

I think it would be helpful to include the event in this case, like we do below in the XPN case:

			g.raiseFirewallChangeNeededEvent(svc, FirewallToGCloudCreateCmd(firewall, g.NetworkProjectID()))

[JoelSpeed]

/lgtm

New event change makes sense to me

[barbacbd]

@bowei Can you please take another look and approve or comment on this PR? Thank you.

[damdo]

/assign @bowei @cheftako

Thanks!

[bowei]

/assign @swetharepakula @mmamczur

Please take a look at this PR -- it will add a field to gce.conf.

[mmamczur]

why not use just a flag for the binary so that you can configure your deployment that way?

it looks like when unset this will maintain the old behavior so it should work fine for existing configurations and gke

[patrickdillon]

why not use just a flag for the binary so that you can configure your deployment that way?

it looks like when unset this will maintain the old behavior so it should work fine for existing configurations and gke

@mmamczur like these flags?

cloud-provider-gcp/cmd/cloud-controller-manager/main.go

Lines 84 to 87 in 25e7d16

	cloudProviderFS := fss.FlagSet("GCE Cloud Provider")
	cloudProviderFS.BoolVar(&enableMultiProject, "enable-multi-project", false, "Enables project selection from Node providerID for GCE API calls. CAUTION: Only enable if Node providerID is configured by a trusted source.")
	cloudProviderFS.BoolVar(&enableDiscretePortForwarding, "enable-discrete-port-forwarding", false, "Enables forwarding of individual ports instead of port ranges for GCE external load balancers.")
	cloudProviderFS.BoolVar(&enableRBSDefaultForL4NetLB, "enable-rbs-default-l4-netlb", false, "Enables RBS defaulting for GCE L4 NetLB")

Based on those, it's not clear to me when we would choose a flag vs the cloud config.

Personally, I would not expose an API for this but make the behavior default, like it is on XPN. So we could just remove the g.OnXPN() condition from code like:

cloud-provider-gcp/providers/gce/gce_loadbalancer_internal.go

Lines 410 to 413 in 25e7d16

	if isForbidden(err) && g.OnXPN() {
	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): could not delete traffic firewall on XPN cluster. Raising event.", loadBalancerName)
	g.raiseFirewallChangeNeededEvent(svc, FirewallToGCloudDeleteCmd(fwName, g.NetworkProjectID()))
	return nil

[JoelSpeed]

I thought generally as a community Kubernetes was moving away from long lists of flag options to config file options with structure and versionability. Certainly this aligns with other additions of late to cloud providers as a config file option over a flag.

Certainly with my SIG Cloud and API reviewer hats on, I think config file is the correct choice for a switch on behaviour here over a flag

[aojea]

I thought generally as a community Kubernetes was moving away from long lists of flag options to config file options with structure and versionability.

this was an intent that never materialized, kube-apiserver uses flags, kubelet moved to config but still has a lot of complex combinations between flags and configs and kube-proxy is in a similar limbo than kubelet, with a considerable use of flags vs config, kube-controller-manager is flag based IIRC , no idea about kube-scheduler and kube-controller-manager but my anecdotical experience is I only see them configured via flags

[mmamczur]

in that case the config file is ok.

I don't know the history behind this but writing this file sounds like a lot of hassle vs just providing flags in the manifest

[patrickdillon]

Does it make sense to expose an API and make this functionality configurable?

This functionality is already enabled by default on XPN; making non-XPN consistent with XPN would reduce the complexity managed in code.

Here's my understanding of what this PR and adding the API enables:

Scenario	FW Perms in Network Project	Firewall Management	Result
non-XPN	True	Enabled	provider creates fw
non-XPN	False	Enabled	provider throws error
non-XPN	True	Disabled	provider creates event
non-XPN	False	Disabled	provider creates event
XPN	True	Enabled	provider creates fw
XPN	False	Enabled	provider creates event
XPN	True	Disabled	provider creates event
XPN	False	Disabled	provider creates event

If we remove the API and make non-XPN consistent with XPN, the behavior becomes:

FW Perms in Network Project	Result
True	provider creates fw
False	provider creates event (we should also log this scenario)

There's something to be said for capturing user intent, but the only difference AFAICT in how we act on that intent is whether the provider throws an error in a specific case.