Enable most admission controllers for the control plane tests #77
Conversation
zen-dog
requested review from
gerred,
jbarrick-mesosphere and
kensipe
as code owners
Summary: we used to be very permissive with admission controllers for the control plane tests by using `--admission-control=AlwaysAdmit` option. It is deprecated and it effectively disables all admission controllers. This PR introduces a much narrower setup by disabling only two admission controllers: `--disable-admission-plugins=ServiceAccount,NamespaceLifecycle` which are still needed for some tests. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
| @@ -69,7 +69,7 @@ var APIServerDefaultArgs = []string{ | |||
| "--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}", | |||
| "--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}", | |||
| "--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}", | |||
| "--admission-control=AlwaysAdmit", | |||
| "--disable-admission-plugins=ServiceAccount,NamespaceLifecycle", | |||
There was a problem hiding this comment.
This is the main change. I also thought about exposing APIServerDefaultArgs to the kuttl users (different tests might require different admission controller set) but wrapping and wiring these through kuttl turned out to be tedious so I just edited it here. Especially, because these are only applied for the controller plane (and to kind or others) tests.
There was a problem hiding this comment.
it seems that you captured the concerns
- only affects mock control plane
- and I also thought exposing thru testsuite makes sense... but it seems fine to capture that in another PR if we gather consensus around it.
I'm concerned with making sure that many use cases are supported... for which I think expose to end user is necessary. perhaps we can take the route we take with kind config... where the full APIServerDefaultArgs can be provided in a file and can be reference as an override in testsuite and a flag. for expert users.
There was a problem hiding this comment.
💯 valid points, which you already addressed in #78 Let's continue the discussion there 👍
| break | ||
| } | ||
| } | ||
| env.Config, err = env.Environment.Start() |
There was a problem hiding this comment.
I don't think this loop makes sense anymore. Envtest already has logic to retry starting the controller plane and a mechanism to suggest free ports
zen-dog
force-pushed
the
ad/admission-controllers
branch
from
6dbfa41
to
e470e54
Compare
| h.T.Log("started test environment (kube-apiserver and etcd) in", time.Since(started)) | ||
| h.T.Logf("started test environment (kube-apiserver and etcd) in %v, with following options:\n%s", | ||
| time.Since(started), | ||
| strings.Join(testenv.Environment.KubeAPIServerFlags, "\n")) |
There was a problem hiding this comment.
output for other reviewers...
kuttl: harness.go:195: started test environment (kube-apiserver and etcd) in 4.597689123s, with following options:
--advertise-address=127.0.0.1
--etcd-servers={{ if .EtcdURL }}{{ .EtcdURL.String }}{{ end }}
--cert-dir={{ .CertDir }}
--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}
--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}
--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}
--disable-admission-plugins=ServiceAccount,NamespaceLifecycle
--service-cluster-ip-range=10.0.0.0/24
--advertise-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}
| @@ -69,7 +69,7 @@ var APIServerDefaultArgs = []string{ | |||
| "--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}", | |||
| "--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}", | |||
| "--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}", | |||
| "--admission-control=AlwaysAdmit", | |||
| "--disable-admission-plugins=ServiceAccount,NamespaceLifecycle", | |||
There was a problem hiding this comment.
it seems that you captured the concerns
- only affects mock control plane
- and I also thought exposing thru testsuite makes sense... but it seems fine to capture that in another PR if we gather consensus around it.
I'm concerned with making sure that many use cases are supported... for which I think expose to end user is necessary. perhaps we can take the route we take with kind config... where the full APIServerDefaultArgs can be provided in a file and can be reference as an override in testsuite and a flag. for expert users.
| @@ -1004,7 +998,7 @@ func RunCommands(logger Logger, namespace string, command string, commands []har | |||
| } | |||
|
|
|||
| for _, cmd := range commands { | |||
| logger.Logf("running command: %s %s", command, cmd) | |||
| logger.Logf("running command: %s %q", command, cmd.Command) | |||
There was a problem hiding this comment.
output for other reviewers
kuttl/harness/cli-test: logger.go:41: 16:24:57 | cli-test/1-patch | running command: "kubectl label pod cli-test-pod test=true"
vs what used to be:
kuttl/harness/cli-test: logger.go:41: 16:33:29 | cli-test/1-patch | running command: {"kubectl label pod cli-test-pod test=true" %!q(bool=true) %!q(bool=false) %!q(bool=false)}
There was a problem hiding this comment.
Oh thanks! That's a lot cleaner :D
Co-authored-by: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Ken Sipe <kensipe@gmail.com>
|
The approvals still stand... we had a PR on a PR both with all needed approvals. The merge of the first PR reset approvals but we will merge without forcing more attention after tests pass. |
Summary:
we used to be very permissive with admission controllers for the control plane tests by using
--admission-control=AlwaysAdmitoption. It is deprecated and it effectively disables all admission controllers. This PR introduces a much narrower setup by disabling only two admission controllers:--disable-admission-plugins=ServiceAccount,NamespaceLifecyclewhich are still needed for some tests.Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com