Skip to content
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

Add new operator capability to check if it has access to do an operation #2467

Merged
merged 13 commits into from
Jan 4, 2024
16 changes: 16 additions & 0 deletions .chloggen/mandate-rbac.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: enables the operator to create subject access reviews for different required permissions.

# One or more tracking issues related to the change
issues: [2426]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ all: manager
.PHONY: ci
ci: test

# helper to get your gomod up-to-date
.PHONY: gomod
gomod:
go mod tidy && go mod vendor && (cd cmd/otel-allocator && go mod tidy) && (cd cmd/operator-opamp-bridge && go mod tidy)
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved

# Run tests
# setup-envtest uses KUBEBUILDER_ASSETS which points to a directory with binaries (api-server, etcd and kubectl)
.PHONY: test
Expand Down
43 changes: 30 additions & 13 deletions apis/v1alpha1/collector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"

"github.com/go-logr/logr"
v1 "k8s.io/api/authorization/v1"
autoscalingv2 "k8s.io/api/autoscaling/v2"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
Expand All @@ -29,11 +30,17 @@ import (
"github.com/open-telemetry/opentelemetry-operator/internal/config"
ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
"github.com/open-telemetry/opentelemetry-operator/pkg/rbac"
)

var (
_ admission.CustomValidator = &CollectorWebhook{}
_ admission.CustomDefaulter = &CollectorWebhook{}
_ admission.CustomValidator = &CollectorWebhook{}
_ admission.CustomDefaulter = &CollectorWebhook{}
targetAllocatorNamespaceRes = &v1.ResourceAttributes{
Namespace: "",
Verb: "list",
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved
Resource: "namespaces",
}
)

// +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=true,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=create;update,versions=v1alpha1,name=mopentelemetrycollector.kb.io,sideEffects=none,admissionReviewVersions=v1
Expand All @@ -42,9 +49,10 @@ var (
// +kubebuilder:object:generate=false

type CollectorWebhook struct {
logger logr.Logger
cfg config.Config
scheme *runtime.Scheme
logger logr.Logger
cfg config.Config
scheme *runtime.Scheme
reviewer *rbac.Reviewer
}

func (c CollectorWebhook) Default(ctx context.Context, obj runtime.Object) error {
Expand All @@ -60,23 +68,23 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object
if !ok {
return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj)
}
return c.validate(otelcol)
return c.validate(ctx, otelcol)
}

func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
otelcol, ok := newObj.(*OpenTelemetryCollector)
if !ok {
return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", newObj)
}
return c.validate(otelcol)
return c.validate(ctx, otelcol)
}

func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
otelcol, ok := obj.(*OpenTelemetryCollector)
if !ok || otelcol == nil {
return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj)
}
return c.validate(otelcol)
return c.validate(ctx, otelcol)
}

func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
Expand Down Expand Up @@ -169,7 +177,7 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
return nil
}

func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warnings, error) {
func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) {
warnings := admission.Warnings{}
// validate volumeClaimTemplates
if r.Spec.Mode != ModeStatefulSet && len(r.Spec.VolumeClaimTemplates) > 0 {
Expand Down Expand Up @@ -214,6 +222,14 @@ func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warning
if err != nil {
return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
// if the prometheusCR is enabled, it needs a suite of permissions to function
if r.Spec.TargetAllocator.PrometheusCR.Enabled {
if ok, err := c.reviewer.CanAccess(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorNamespaceRes); err != nil {
return warnings, fmt.Errorf("unable to check rbac rules %w", err)
} else if !ok {
warnings = append(warnings, "target allocator's serviceaccount is missing a permission for listing namespaces.")
}
}
}

// validator port config
Expand Down Expand Up @@ -360,11 +376,12 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error {
return nil
}

func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config) error {
func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer) error {
cvw := &CollectorWebhook{
logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"),
scheme: mgr.GetScheme(),
cfg: cfg,
reviewer: reviewer,
logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"),
scheme: mgr.GetScheme(),
cfg: cfg,
}
return ctrl.NewWebhookManagedBy(mgr).
For(&OpenTelemetryCollector{}).
Expand Down
137 changes: 137 additions & 0 deletions apis/v1alpha1/collector_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,19 @@ import (
"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
authv1 "k8s.io/api/authorization/v1"
autoscalingv2 "k8s.io/api/autoscaling/v2"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/kubernetes/scheme"
kubeTesting "k8s.io/client-go/testing"

"github.com/open-telemetry/opentelemetry-operator/internal/config"
"github.com/open-telemetry/opentelemetry-operator/pkg/rbac"
)

var (
Expand Down Expand Up @@ -438,6 +442,7 @@ func TestOTELColValidatingWebhook(t *testing.T) {
otelcol OpenTelemetryCollector
expectedErr string
expectedWarnings []string
shouldFailSar bool
}{
{
name: "valid empty spec",
Expand Down Expand Up @@ -469,6 +474,117 @@ func TestOTELColValidatingWebhook(t *testing.T) {
protocols:
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
{
Name: "port1",
Port: 5555,
},
{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
},
},
Autoscaler: &AutoscalerSpec{
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleDown: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &three,
},
ScaleUp: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &five,
},
},
TargetCPUUtilization: &five,
},
},
},
},
{
name: "prom CR admissions warning",
shouldFailSar: true, // force failure
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Mode: ModeStatefulSet,
MinReplicas: &one,
Replicas: &three,
MaxReplicas: &five,
UpgradeStrategy: "adhoc",
TargetAllocator: OpenTelemetryTargetAllocator{
Enabled: true,
PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true},
},
Config: `receivers:
examplereceiver:
endpoint: "0.0.0.0:12345"
examplereceiver/settings:
endpoint: "0.0.0.0:12346"
prometheus:
config:
scrape_configs:
- job_name: otel-collector
scrape_interval: 10s
jaeger/custom:
protocols:
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
{
Name: "port1",
Port: 5555,
},
{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
},
},
Autoscaler: &AutoscalerSpec{
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleDown: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &three,
},
ScaleUp: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &five,
},
},
TargetCPUUtilization: &five,
},
},
},
expectedWarnings: []string{
"target allocator's serviceaccount is missing a permission for listing namespaces.",
},
},
{
name: "prom CR no admissions warning",
shouldFailSar: false, // force SAR okay
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Mode: ModeStatefulSet,
MinReplicas: &one,
Replicas: &three,
MaxReplicas: &five,
UpgradeStrategy: "adhoc",
TargetAllocator: OpenTelemetryTargetAllocator{
Enabled: true,
PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true},
},
Config: `receivers:
examplereceiver:
endpoint: "0.0.0.0:12345"
examplereceiver/settings:
endpoint: "0.0.0.0:12346"
prometheus:
config:
scrape_configs:
- job_name: otel-collector
scrape_interval: 10s
jaeger/custom:
protocols:
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
{
Expand Down Expand Up @@ -923,6 +1039,7 @@ func TestOTELColValidatingWebhook(t *testing.T) {
config.WithCollectorImage("collector:v0.0.0"),
config.WithTargetAllocatorImage("ta:v0.0.0"),
),
reviewer: getReviewer(test.shouldFailSar),
}
ctx := context.Background()
warnings, err := cvw.ValidateCreate(ctx, &test.otelcol)
Expand All @@ -939,3 +1056,23 @@ func TestOTELColValidatingWebhook(t *testing.T) {
})
}
}

func getReviewer(shouldFailSAR bool) *rbac.Reviewer {
c := fake.NewSimpleClientset()
c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) {
// check our expectation here
if !action.Matches("create", "subjectaccessreviews") {
return false, nil, fmt.Errorf("must be a create for a SAR")
}
sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview)
if !ok || sar == nil {
return false, nil, fmt.Errorf("bad object")
}
sar.Status = authv1.SubjectAccessReviewStatus{
Allowed: !shouldFailSAR,
Denied: shouldFailSAR,
}
return true, sar, nil
})
return rbac.NewReviewer(c)
}
9 changes: 8 additions & 1 deletion controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/record"
Expand All @@ -52,6 +53,7 @@ import (
"github.com/open-telemetry/opentelemetry-operator/internal/config"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata"
"github.com/open-telemetry/opentelemetry-operator/pkg/rbac"
// +kubebuilder:scaffold:imports
)

Expand Down Expand Up @@ -147,8 +149,13 @@ func TestMain(m *testing.M) {
fmt.Printf("failed to start webhook server: %v", mgrErr)
os.Exit(1)
}
clientset, clientErr := kubernetes.NewForConfig(cfg)
if err != nil {
fmt.Printf("failed to setup kubernetes clientset %v", clientErr)
}
reviewer := rbac.NewReviewer(clientset)

if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil {
if err = v1alpha1.SetupCollectorWebhook(mgr, config.New(), reviewer); err != nil {
fmt.Printf("failed to SetupWebhookWithManager: %v", err)
os.Exit(1)
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ require (
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
github.com/envoyproxy/go-control-plane v0.11.1 // indirect
github.com/envoyproxy/protoc-gen-validate v1.0.2 // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.6.0 // indirect
github.com/fatih/color v1.15.0 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
Expand Down
10 changes: 9 additions & 1 deletion internal/webhook/podmutation/webhookhandler_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/client-go/util/retry"
Expand All @@ -38,6 +39,7 @@ import (

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/config"
"github.com/open-telemetry/opentelemetry-operator/pkg/rbac"
// +kubebuilder:scaffold:imports
)

Expand Down Expand Up @@ -98,7 +100,13 @@ func TestMain(m *testing.M) {
os.Exit(1)
}

if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil {
clientset, clientErr := kubernetes.NewForConfig(cfg)
if err != nil {
fmt.Printf("failed to setup kubernetes clientset %v", clientErr)
}
reviewer := rbac.NewReviewer(clientset)

if err = v1alpha1.SetupCollectorWebhook(mgr, config.New(), reviewer); err != nil {
fmt.Printf("failed to SetupWebhookWithManager: %v", err)
os.Exit(1)
}
Expand Down
Loading
Loading