-
Notifications
You must be signed in to change notification settings - Fork 50
CM-1225: Apply cluster TLS profile to trust-manager, operator metrics, and operand HTTPS metrics #466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CM-1225: Apply cluster TLS profile to trust-manager, operator metrics, and operand HTTPS metrics #466
Changes from all commits
880f04f
6187bde
a2391db
11b8fa3
11d5f69
b795b39
e7a47ce
01c94a9
7a63e04
5dbd644
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the manifest added manually? If so the manifest update script must be updated to preserve this. Is this manifest not already available in upstream chart? |
||
| kind: RoleBinding | ||
| metadata: | ||
| labels: | ||
| app: cert-manager | ||
| app.kubernetes.io/component: controller | ||
| app.kubernetes.io/instance: cert-manager | ||
| app.kubernetes.io/name: cert-manager | ||
| app.kubernetes.io/version: v1.20.3 | ||
| name: cert-manager-metrics-dynamic-serving | ||
| namespace: cert-manager | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: Role | ||
| name: cert-manager-metrics-dynamic-serving | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: cert-manager | ||
| namespace: cert-manager | ||
| - kind: ServiceAccount | ||
| name: cert-manager-webhook | ||
| namespace: cert-manager | ||
| - kind: ServiceAccount | ||
| name: cert-manager-cainjector | ||
| namespace: cert-manager | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| labels: | ||
| app: cert-manager | ||
| app.kubernetes.io/component: controller | ||
| app.kubernetes.io/instance: cert-manager | ||
| app.kubernetes.io/name: cert-manager | ||
| app.kubernetes.io/version: v1.20.3 | ||
| name: cert-manager-metrics-dynamic-serving | ||
| namespace: cert-manager | ||
| rules: | ||
| - apiGroups: | ||
| - "" | ||
| resourceNames: | ||
| - cert-manager-metrics-ca | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - update | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - create |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| annotations: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to be problem when and if we are going to use the cert-manager for internal certificates too. We will need to provide an option for users to configure the TLS keys for operator metrics too, as an arg, probably reference to secret. |
||
| service.beta.openshift.io/serving-cert-secret-name: cert-manager-operator-serving-cert | ||
| creationTimestamp: null | ||
| labels: | ||
| app.kubernetes.io/created-by: cert-manager-operator | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,23 +2,89 @@ package operator | |
|
|
||
| import ( | ||
| "context" | ||
| "math/rand" | ||
| "os" | ||
| "time" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "k8s.io/apiserver/pkg/server" | ||
| "k8s.io/component-base/logs" | ||
| "k8s.io/klog/v2" | ||
| "k8s.io/utils/clock" | ||
|
|
||
| "github.com/openshift/cert-manager-operator/pkg/operator" | ||
| "github.com/openshift/cert-manager-operator/pkg/tlsprofile" | ||
| "github.com/openshift/cert-manager-operator/pkg/version" | ||
| "github.com/openshift/library-go/pkg/controller/controllercmd" | ||
| "github.com/spf13/cobra" | ||
| "k8s.io/utils/clock" | ||
| "github.com/openshift/library-go/pkg/controller/fileobserver" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| "github.com/openshift/library-go/pkg/serviceability" | ||
| ) | ||
|
|
||
| func NewOperator() *cobra.Command { | ||
| cmd := controllercmd.NewControllerCommandConfig( | ||
| cc := controllercmd.NewControllerCommandConfig( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the need for changes in this file, specifically duplicating the whole But I think this can be avoided and will help other operators as well, if the library-go can be updated with a hook function to patch the TLS configs. Something like |
||
| "cert-manager-operator", | ||
| version.Get(), | ||
| operator.RunOperator, | ||
| clock.RealClock{}, | ||
| ).NewCommandWithContext(context.TODO()) | ||
| ) | ||
|
|
||
| cmd := cc.NewCommandWithContext(context.TODO()) | ||
| cmd.Use = "start" | ||
| cmd.Short = "Start the cert-manager Operator" | ||
|
|
||
| // Replace the default Run so we can apply the cluster TLS profile to the | ||
| // metrics serving config before the HTTPS listener is created. | ||
| cmd.Run = func(cmd *cobra.Command, args []string) { | ||
| rand.Seed(time.Now().UTC().UnixNano()) | ||
| logs.InitLogs() | ||
| defer logs.FlushLogs() | ||
| defer serviceability.BehaviorOnPanic(os.Getenv("OPENSHIFT_ON_PANIC"), version.Get())() | ||
| defer serviceability.Profile(os.Getenv("OPENSHIFT_PROFILE")).Stop() | ||
| serviceability.StartProfiler() | ||
|
|
||
| shutdownCtx, cancel := context.WithCancel(context.Background()) | ||
| shutdownHandler := server.SetupSignalHandler() | ||
| go func() { | ||
| defer cancel() | ||
| <-shutdownHandler | ||
| klog.Infof("Received SIGTERM or SIGINT signal, shutting down controller.") | ||
| }() | ||
|
|
||
| ctx, terminate := context.WithCancel(shutdownCtx) | ||
| defer terminate() | ||
|
|
||
| terminateOnFiles, err := cmd.Flags().GetStringArray("terminate-on-files") | ||
| if err != nil { | ||
| klog.Fatal(err) | ||
| } | ||
| if len(terminateOnFiles) > 0 { | ||
| obs, err := fileobserver.NewObserver(10 * time.Second) | ||
| if err != nil { | ||
| klog.Fatal(err) | ||
| } | ||
| files := map[string][]byte{} | ||
| for _, fn := range terminateOnFiles { | ||
| fileBytes, err := os.ReadFile(fn) | ||
| if err != nil { | ||
| klog.Warningf("Unable to read initial content of %q: %v", fn, err) | ||
| continue | ||
| } | ||
| files[fn] = fileBytes | ||
| } | ||
| obs.AddReactor(func(filename string, action fileobserver.ActionType) error { | ||
| klog.Infof("exiting because %q changed", filename) | ||
| terminate() | ||
| return nil | ||
| }, files, terminateOnFiles...) | ||
| go obs.Run(shutdownHandler) | ||
| } | ||
|
|
||
| if err := startControllerWithClusterTLS(ctx, cc, cmd); err != nil { | ||
| klog.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&operator.TrustedCAConfigMapName, "trusted-ca-configmap", "", "The name of the config map containing TLS CA(s) which should be trusted by the controller's containers. PEM encoded file under \"ca-bundle.crt\" key is expected.") | ||
| cmd.Flags().StringVar(&operator.CloudCredentialSecret, "cloud-credentials-secret", "", "The name of the secret containing cloud credentials for authenticating using cert-manager ambient credentials mode.") | ||
|
|
||
|
|
@@ -34,3 +100,86 @@ These features provide early access to upcoming product features, | |
| enabling customers to test functionality and provide feedback during the development process.`) | ||
| return cmd | ||
| } | ||
|
|
||
| func startControllerWithClusterTLS(ctx context.Context, c *controllercmd.ControllerCommandConfig, cmd *cobra.Command) error { | ||
| unstructuredConfig, config, configContent, err := c.Config() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| startingFileContent, observedFiles, err := c.AddDefaultRotationToConfig(config, configContent) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| listen, err := cmd.Flags().GetString("listen") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(listen) != 0 { | ||
| config.ServingInfo.BindAddress = listen | ||
| } | ||
|
|
||
| kubeConfigFile, err := cmd.Flags().GetString("kubeconfig") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| namespace, err := cmd.Flags().GetString("namespace") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !c.DisableServing { | ||
| restConfig, err := tlsprofile.RESTConfigFromKubeConfig(kubeConfigFile) | ||
| if err != nil { | ||
| klog.Warningf("unable to build rest config for cluster TLS profile lookup; using Controllercmd default TLS settings: %v", err) | ||
| } else { | ||
| lookupCtx, cancelLookup := context.WithTimeout(ctx, 30*time.Second) | ||
| err := tlsprofile.ApplyClusterProfileToHTTPServingInfo(lookupCtx, restConfig, &config.ServingInfo) | ||
| cancelLookup() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| exitOnChangeReactorCh := make(chan struct{}) | ||
| controllerCtx, cancel := context.WithCancel(ctx) | ||
| go func() { | ||
| select { | ||
| case <-exitOnChangeReactorCh: | ||
| cancel() | ||
| case <-ctx.Done(): | ||
| cancel() | ||
| } | ||
| }() | ||
|
|
||
| config.LeaderElection.Disable = c.DisableLeaderElection | ||
| config.LeaderElection.LeaseDuration = c.LeaseDuration | ||
| config.LeaderElection.RenewDeadline = c.RenewDeadline | ||
| config.LeaderElection.RetryPeriod = c.RetryPeriod | ||
|
|
||
| builder := controllercmd.NewController("cert-manager-operator", operator.RunOperator, clock.RealClock{}). | ||
| WithKubeConfigFile(kubeConfigFile, nil). | ||
| WithComponentNamespace(namespace). | ||
| WithLeaderElection(config.LeaderElection, namespace, "cert-manager-operator-lock"). | ||
| WithVersion(version.Get()). | ||
| WithEventRecorderOptions(events.RecommendedClusterSingletonCorrelatorOptions()). | ||
| WithRestartOnChange(exitOnChangeReactorCh, startingFileContent, observedFiles...) | ||
|
|
||
| if !c.DisableServing { | ||
| builder = builder.WithServer(config.ServingInfo, config.Authentication, config.Authorization) | ||
| if c.EnableHTTP2 { | ||
| builder = builder.WithHTTP2() | ||
| } | ||
| if c.SkipInClusterAuthenticationLookup { | ||
| builder = builder.WithSkipInClusterAuthenticationLookup() | ||
| } | ||
| } | ||
|
|
||
| if c.TopologyDetector != nil { | ||
| builder = builder.WithTopologyDetector(c.TopologyDetector) | ||
| } | ||
|
|
||
| return builder.Run(controllerCtx, unstructuredConfig) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this manually added? If so this needs to be handled through
hack/update-cert-manager-manifests.shto patch the content.Also, IIRC, port 9402 is for http APIs, was the port updated to handle https APIs?