Make the webhook server port configurable via flag

[joonas]

/kind feature

Describe the solution you'd like

It was pointed out that in some cases the default value (443) for the webhook server port can be problematic, so we should should make this configurable via a flag.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• Cluster-api-provider-aws version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[joonas]

/help
/good-first-issue
/priority important-longterm

[k8s-ci-robot]

@joonas:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/help
/good-first-issue
/priority important-longterm

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.

[luigi-riefolo]

@joonas I'd like to work on this. Could you please provide an example of the desired functionality and/or how to run it?

[joonas]

@luigi-riefolo Great!

I believe what we should look at doing is adding a new flag (say webhook-port) that you can pass in to the CAPA manager (see how existing flags are setup), which then subsequently gets passed down as the Port option when initializing the controller-runtime manager.

Note, we should default the new flag with the existing default value.

Is that enough context to get you started?

[tahsinrahman]

@joonas should we set default value to 443? then it'll always fail if we don't define value via flag

[detiber]

@tahsinrahman I think having the default be 443 for the binary would be good, then we can add a kustomize patch to config/default/ to override the argument to a more appropriate port for our use case.

[ncdc]

Wouldn't it make more sense to use a higher port so it doesn't require root? I'd go with 8443 for the default (assuming no conflicts with e.g. the metrics server or something else).

[vincepri]

+1 on having a higher port and use non-root user

[wfernandes]

@luigi-riefolo Could you /assign and /lifecycle active this issue if you are actively working on it?

[luigi-riefolo]

/assign
/lifecycle active

[luigi-riefolo]

@joonas it is

[luigi-riefolo]

@joonas which version should I use as base branch for the PR?
master: does not contain clusterctl, therefore make create-cluster-management fails
release-0.3: does not contain main.go with all the flags you were referring to

[joonas]

@luigi-riefolo I would recommend working against the master branch (for the current v1alpha2 release).

clusterctl has been moved from the provider-specific code to core cluster-api instead, so you would have to first get a copy of it from the cluster-api repository.

@detiber @ncdc @vincepri thoughts on having a script in this repo that would check your path for clusterctl and fetch it if missing for the Makefile commands before attempting to use it?

[vincepri]

While we don't ship clusterctl as part of the release, the Makefile builds it in a local directory before using it https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/Makefile#L109-L110

[luigi-riefolo]

@vincepri the above make target generates this error:

go build -o bin/clusterctl sigs.k8s.io/cluster-api/cmd/clusterctl
build sigs.k8s.io/cluster-api/cmd/clusterctl: cannot load github.com/Azure/go-autorest/autorest: ambiguous import: found github.com/Azure/go-autorest/autorest in multiple modules:
	github.com/Azure/go-autorest v11.1.2+incompatible (/home/luigi/go/pkg/mod/github.com/!azure/go-autorest@v11.1.2+incompatible/autorest)
	github.com/Azure/go-autorest/autorest v0.9.0 (/home/luigi/go/pkg/mod/github.com/!azure/go-autorest/autorest@v0.9.0)
Makefile:110: recipe for target 'bin/clusterctl' failed

is this a known issue?

[vincepri]

That's unfortunate, can you file a different issue? Seems something broke somewhere in modules

[vincepri]

Filed #1169