From 82be7bfcdd47ca6f4687c07ae09240e546cca1d4 Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Tue, 13 Aug 2024 10:53:34 -0700 Subject: [PATCH 1/7] OSD-24275: Validate machineCIDR is contained in default ingresscontroller allowedSourceRanges --- build/resources.go | 59 ++++ build/selectorsyncset.yaml | 29 ++ .../ingresscontroller/ingresscontroller.go | 182 +++++++++- .../ingresscontroller_test.go | 334 +++++++++++++++++- 4 files changed, 586 insertions(+), 18 deletions(-) diff --git a/build/resources.go b/build/resources.go index 4ac14524..d1d8fdee 100644 --- a/build/resources.go +++ b/build/resources.go @@ -32,6 +32,9 @@ const ( roleName string = "validation-webhook" prometheusRoleName string = "prometheus-k8s" repoName string = "managed-cluster-validating-webhooks" + // Role and Binding for reading cluster-config-v1 config map... + clusterConfigRole string = "config-v1-reader-wh" + clusterConfigRoleBinding string = "validation-webhook-cluster-config-v1-reader" // Used to define what phase a resource should be deployed in by package-operator pkoPhaseAnnotation string = "package-operator.run/phase" // Defines the 'rbac' package-operator phase for any resources related to RBAC @@ -211,6 +214,60 @@ func createClusterRoleBinding() *rbacv1.ClusterRoleBinding { } } +func createClusterConfigRole() *rbacv1.Role { + return &rbacv1.Role{ + TypeMeta: metav1.TypeMeta{ + Kind: "Role", + APIVersion: rbacv1.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: clusterConfigRole, + Namespace: "kube-system", + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{ + "", + }, + Resources: []string{ + "configmaps", + }, + Verbs: []string{ + "get", + }, + ResourceNames: []string{ + "cluster-config-v1", + }, + }, + }, + } +} + +func createClusterConfigRoleBinding() *rbacv1.RoleBinding { + return &rbacv1.RoleBinding{ + TypeMeta: metav1.TypeMeta{ + Kind: "RoleBinding", + APIVersion: rbacv1.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: clusterConfigRoleBinding, + Namespace: "kube-system", + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: serviceAccountName, + Namespace: *namespace, + }, + }, + RoleRef: rbacv1.RoleRef{ + Name: clusterConfigRole, + Kind: "Role", + APIGroup: rbacv1.GroupName, + }, + } +} + func createPrometheusRole() *rbacv1.Role { return &rbacv1.Role{ TypeMeta: metav1.TypeMeta{ @@ -851,6 +908,8 @@ func main() { templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterRoleBinding()}) templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createPrometheusRole()}) templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createPromethusRoleBinding()}) + templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterConfigRole()}) + templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createClusterConfigRoleBinding()}) templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createServiceMonitor()}) templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createCACertConfigMap()}) templateResources.Add(utils.DefaultLabelSelector(), runtime.RawExtension{Object: createService()}) diff --git a/build/selectorsyncset.yaml b/build/selectorsyncset.yaml index 59f30e16..8459155d 100644 --- a/build/selectorsyncset.yaml +++ b/build/selectorsyncset.yaml @@ -123,6 +123,35 @@ objects: - kind: ServiceAccount name: prometheus-k8s namespace: openshift-monitoring + - apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + metadata: + creationTimestamp: null + name: config-v1-reader-wh + namespace: kube-system + rules: + - apiGroups: + - "" + resourceNames: + - cluster-config-v1 + resources: + - configmaps + verbs: + - get + - apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + creationTimestamp: null + name: validation-webhook-cluster-config-v1-reader + namespace: kube-system + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: config-v1-reader-wh + subjects: + - kind: ServiceAccount + name: validation-webhook + namespace: openshift-validation-webhook - apiVersion: monitoring.coreos.com/v1 kind: ServiceMonitor metadata: diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index 437c4368..5b9f6407 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -1,25 +1,37 @@ package ingresscontroller import ( + "bytes" + "context" "fmt" + "net" "net/http" + "os" "slices" "strings" operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/managed-cluster-validating-webhooks/pkg/k8sutil" "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils" + "github.com/pkg/errors" + admissionv1 "k8s.io/api/admission/v1" admissionregv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - admissionctl "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - + "k8s.io/apimachinery/pkg/util/yaml" + "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" + admissionctl "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) const ( WebhookName string = "ingresscontroller-validation" docString string = `Managed OpenShift Customer may create IngressControllers without necessary taints. This can cause those workloads to be provisioned on master nodes.` legacyIngressSupportFeatureFlag = "ext-managed.openshift.io/legacy-ingress-support" + installConfigMap = "cluster-config-v1" + installConfigNamespace = "kube-system" + installConfigKeyName = "install-config" ) var ( @@ -42,7 +54,22 @@ var ( ) type IngressControllerWebhook struct { - s runtime.Scheme + s runtime.Scheme + kubeClient client.Client + // Allow caching install config and machineCIDR values... + machineCIDRIP net.IP + machineCIDRNet *net.IPNet +} + +// installConfig struct to load the min values needed from the install-config data. +// Alternative would be to vendor all of github.com/openshift/installer/pkg/types to import the proper struct. +// Required values: machineCidr info, anything else we gather from this? hcp vs classic, privatelink info, etc? +type installConfig struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata"` + Networking struct { + MachineCIDR string `json:"machineCIDR"` + } `json:"networking"` } // ObjectSelector implements Webhook interface @@ -149,12 +176,151 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm } } - ret = admissionctl.Allowed("IngressController operation is allowed") + /* TODO: + * 1) ONLY check for privatelink clusters? How? + * 2) HCP vs Classic, etc.. ...any other cluster type this should apply to? Is this handled by the + * HypershiftEnabled vs ClassicEnabled flags set in this module? Currently HCP disabled. + * If HCP is to be enabled for allowed source ranges, should this part of a 2nd ingress validator to + * allow separation of validations between cluster install types? Is there a run time method available + * to this validator to determine classic vs hcp? + * 3) What other specifics should be checked here for this cidr check to be applicable?m + */ + // Only check for machine cidr in allowed ranges if creating or updating resource... + reqOp := request.AdmissionRequest.Operation + if reqOp == admissionv1.Create || reqOp == admissionv1.Update { + //TODO: Will these need to iterate over more than just the default IngressController config? + if ic.ObjectMeta.Name == "default" && ic.ObjectMeta.Namespace == "openshift-ingress-operator" { + ret := wh.checkAllowsMachineCIDR(ic.Spec.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges) + ret.UID = request.AdmissionRequest.UID + if !ret.Allowed { + log.Info("Error checking minimum AllowedSourceRange", "err", ret.AdmissionResponse.String()) + } + return ret + } + } + log.Info("############# DEBUG LOG: IngressController operation is allowed ###########") + ret = admissionctl.Allowed("IngressController operation is allowed, machineCIDR n/a") ret.UID = request.AdmissionRequest.UID return ret } +func (wh *IngressControllerWebhook) getMachineCIDR() (net.IP, *net.IPNet, error) { + if wh.machineCIDRIP == nil || wh.machineCIDRNet == nil { + instConf, err := wh.getClusterConfig() + if err != nil { + log.Error(err, "Failed to fetch machineCIDR", "namespace", installConfigNamespace, "configmap", installConfigMap) + return nil, nil, err + } + if instConf == nil { + err := fmt.Errorf("can not fetch machineCIDR from empty '%s' install config", installConfigMap) + log.Error(err, "getMachineCIDR failed to find CIDR value") + return nil, nil, err + } + if len(instConf.Networking.MachineCIDR) <= 0 { + err := fmt.Errorf("empty machineCIDR string value parsed from '%s' install config", installConfigMap) + log.Error(err, "getMachineCIDR found empty CIDR value") + return nil, nil, err + } + machIP, machNet, err := net.ParseCIDR(string(instConf.Networking.MachineCIDR)) + if err != nil { + log.Error(err, "err parsing machineCIDR into network cidr", "machineCIDR", string(instConf.Networking.MachineCIDR)) + return nil, nil, err + } + if machIP == nil || machNet == nil { + err := fmt.Errorf("failed to parse machineCIDR string:'%s' into network structures", string(instConf.Networking.MachineCIDR)) + log.Error(err, "failed to parse install-config machineCIDR") + return nil, nil, err + } + // Successfully fetched, parsed, and converted the machineCIDR string into net structures... + wh.machineCIDRIP = machIP + wh.machineCIDRNet = machNet + } + return wh.machineCIDRIP, wh.machineCIDRNet, nil +} + +/* Fetch the install-config from the kube-system config map's data. + * this requires proper role, rolebinding for this service account's get() request + * to succeed. (see toplevel selectorsyncset). This config should not change during runtime so + * this operation should cache this if possible. + * TODO: Should it retry fetching the config if there are any failures/errors encountered while + * parsing out the the desired values? + */ +func (wh *IngressControllerWebhook) getClusterConfig() (*installConfig, error) { + var err error + if wh.kubeClient == nil { + wh.kubeClient, err = k8sutil.KubeClient(&wh.s) + if err != nil { + log.Error(err, "Fail creating KubeClient for IngressControllerWebhook") + return nil, err + } + } + clusterConfig := &corev1.ConfigMap{} + err = wh.kubeClient.Get(context.Background(), client.ObjectKey{Name: installConfigMap, Namespace: installConfigNamespace}, clusterConfig) + if err != nil { + log.Error(err, "Failed to fetch configmap: 'cluster-config-v1' for cluster config") + return nil, err + } + data, ok := clusterConfig.Data[installConfigKeyName] + if !ok { + return nil, fmt.Errorf("did not find key %s in configmap %s/%s", installConfigKeyName, installConfigNamespace, installConfigMap) + } + instConf := &installConfig{} + + decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader([]byte(data)), 4096) + if err := decoder.Decode(instConf); err != nil { + return nil, errors.Wrap(err, "failed to decode install config") + } + return instConf, nil +} + +func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1.CIDR) admissionctl.Response { + // https://docs.openshift.com/container-platform/4.13/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-load-balancer-allowed-source-ranges.html + // Note: From docs it appears a missing ASR value/attr allows all. However... + // once ASR values have been added to an ingresscontroller, later deleting all the ASRs can expose an issue + // where the IGC will remaining in progressing state indefinitely. + // For now return Allowed with a warning? + if ipRanges == nil || len(ipRanges) <= 0 { + return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'. Populate this value if operator remains in 'progressing' state") + } + machIP, machNet, err := wh.getMachineCIDR() + if err != nil { + // This represents a fault in either the webhook itself, webhook permissions, or install config. + // Might be nice to have an env var etc we can set to allow proceeding w/o the immediate need to roll new code? + return admissionctl.Errored(http.StatusInternalServerError, err) + } + machNetSize, machNetBits := machNet.Mask.Size() + log.Info("Checking AllowedSourceRanges", "MachineCIDR", fmt.Sprintf("%s/%d", machIP.String(), machNetSize), "NetBits", machNetBits, "AllowedSourceRanges", ipRanges) + for _, OpV1CIDR := range ipRanges { + // Clean up the operatorV1.CIDR value into trimmed CIDR 'a.b.c.d/x' string + ASRstring := strings.TrimSpace(string(OpV1CIDR)) + log.Info(fmt.Sprintf("Checking allowed source:'%s'", ASRstring)) + if len(ASRstring) <= 0 { + continue + } + // Parse the Allowed Source Range Cidr entry into network structures... + _, ASRNet, err := net.ParseCIDR(ASRstring) + if err != nil { + log.Info(fmt.Sprintf("failed to parse AllowedSourceRanges value: '%s'. Err: %s", string(ASRstring), err)) + return admissionctl.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse AllowedSourceRanges value: '%s'. Err: %s", string(ASRstring), err)) + } + // First check if this AlloweSourceRange entry network contains the machine cidr ip... + if !ASRNet.Contains(machIP) { + log.Info(fmt.Sprintf("AllowedSourceRange:'%s' does not contain machine CIDR:'%s/%d'", ASRstring, machIP.String(), machNetSize)) + continue + } + // Check if this AlloweSourceRange entry mask includes the network. + ASRNetSize, ASRNetBits := ASRNet.Mask.Size() + if machNetBits == ASRNetBits && ASRNetSize <= machNetSize { + log.Info(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring)) + return admissionctl.Allowed(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring)) + //return admissionctl.Allowed("IngressController operation is allowed. Minimum AllowedSourceRanges are met.") + } + } + log.Info(fmt.Sprintf("machineCidr:'%s/%d' not found within networks provided by AllowedSourceRanges:'%v'", machIP.String(), machNetSize, ipRanges)) + return admissionctl.Denied(fmt.Sprintf("At least one AllowedSourceRange must allow machine cidr:'%s/%d'", machIP.String(), machNetSize)) +} + // isAllowedUser checks if the user is allowed to perform the action func isAllowedUser(request admissionctl.Request) bool { log.Info(fmt.Sprintf("Checking username %s on whitelist", request.UserInfo.Username)) @@ -199,7 +365,13 @@ func (s *IngressControllerWebhook) HypershiftEnabled() bool { return false } // NewWebhook creates a new webhook func NewWebhook() *IngressControllerWebhook { scheme := runtime.NewScheme() + err := corev1.AddToScheme(scheme) + if err != nil { + log.Error(err, "Fail adding corev1 scheme to IngressControllerWebhook") + os.Exit(1) + } return &IngressControllerWebhook{ - s: *scheme, + s: *scheme, + kubeClient: nil, } } diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller_test.go b/pkg/webhooks/ingresscontroller/ingresscontroller_test.go index bee3b435..773ca90d 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller_test.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller_test.go @@ -3,10 +3,13 @@ package ingresscontroller import ( "encoding/json" "fmt" + "net" "testing" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/managed-cluster-validating-webhooks/pkg/testutils" admissionv1 "k8s.io/api/admission/v1" + admissionregv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -32,6 +35,7 @@ const template string = `{ "domain": "apps.dummy.devshift.org", "endpointPublishingStrategy": { "loadBalancer": { + %s "providerParameters": { "aws": { "classicLoadBalancer": { @@ -61,7 +65,8 @@ const template string = `{ } ` -func createRawIngressControllerJSON(name string, namespace string, nodeSelector corev1.NodeSelector, tolerations []corev1.Toleration) (string, error) { +func createRawIngressControllerJSON(name string, namespace string, nodeSelector corev1.NodeSelector, tolerations []corev1.Toleration, allowedRanges []operatorv1.CIDR) (string, error) { + var AllowedSourceRangesPartial string = "" nodeSelectorPartial, err := json.Marshal(nodeSelector) if err != nil { return "", err @@ -70,22 +75,32 @@ func createRawIngressControllerJSON(name string, namespace string, nodeSelector if err != nil { return "", err } + // Allow a nil value to exclude the 'allowedSourceRanges' param from the request. + if allowedRanges != nil { + ASRString, err := json.Marshal(allowedRanges) + if err != nil { + return "", err + } + AllowedSourceRangesPartial = fmt.Sprintf("\"allowedSourceRanges\": %s,", ASRString) + } - output := fmt.Sprintf(template, name, namespace, string(nodeSelectorPartial), string(tolerationsPartial)) - + output := fmt.Sprintf(template, name, namespace, string(AllowedSourceRangesPartial), string(nodeSelectorPartial), string(tolerationsPartial)) return output, nil } type ingressControllerTestSuites struct { - testID string - name string - namespace string - username string - userGroups []string - operation admissionv1.Operation - nodeSelector corev1.NodeSelector - tolerations []corev1.Toleration - shouldBeAllowed bool + testID string + name string + namespace string + username string + userGroups []string + operation admissionv1.Operation + nodeSelector corev1.NodeSelector + tolerations []corev1.Toleration + allowedSourceRanges []operatorv1.CIDR + machineCIDR string + shouldBeAllowed bool + errorContains string } func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites) { @@ -100,7 +115,7 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites Resource: "ingresscontroller", } for _, test := range tests { - rawObjString, err := createRawIngressControllerJSON(test.name, test.namespace, test.nodeSelector, test.tolerations) + rawObjString, err := createRawIngressControllerJSON(test.name, test.namespace, test.nodeSelector, test.tolerations, test.allowedSourceRanges) if err != nil { t.Fatalf("Couldn't create a JSON fragment %s", err.Error()) } @@ -114,9 +129,14 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites } hook := NewWebhook() + err = setMachineCidr(t, test, hook) + if err != nil { + t.Fatalf("Expected no error, got err parsing machineCIDR:'%s', got '%s'", string(test.machineCIDR), err.Error()) + } httprequest, err := testutils.CreateHTTPRequest(hook.GetURI(), test.testID, gvk, gvr, test.operation, test.username, test.userGroups, "", &obj, &oldObj) if err != nil { + t.Logf("Request object:'%s'", obj) t.Fatalf("Expected no error, got %s", err.Error()) } @@ -125,14 +145,32 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites t.Fatalf("Expected no error, got %s", err.Error()) } if response.UID == "" { + //t.Logf("Request object:'%s'", obj) + t.Logf("Response object:'%v'", response) t.Fatalf("No tracking UID associated with the response.") } if response.Allowed != test.shouldBeAllowed { + t.Logf("%s", obj) + t.Logf("Response.reason:'%v'", response) t.Fatalf("[%s] Mismatch: %s (groups=%s) %s %s the ingress controller. Test's expectation is that the user %s", test.testID, test.username, test.userGroups, testutils.CanCanNot(response.Allowed), test.operation, testutils.CanCanNot(test.shouldBeAllowed)) + } + } +} +func setMachineCidr(t *testing.T, test ingressControllerTestSuites, hook *IngressControllerWebhook) error { + if len(test.machineCIDR) > 0 { + ip, net, err := net.ParseCIDR(string(test.machineCIDR)) + if err != nil { + return err } + hook.machineCIDRIP = ip + hook.machineCIDRNet = net + } else { + hook.machineCIDRIP = nil + hook.machineCIDRNet = nil } + return nil } func TestIngressControllerTolerations(t *testing.T) { @@ -362,3 +400,273 @@ func TestIngressControllerExceptions(t *testing.T) { } runIngressControllerTests(t, tests) } + +func runIngressControllerAllowedSourceRangesTests(t *testing.T, op admissionv1.Operation) { + tests := []ingressControllerTestSuites{ + { + testID: fmt.Sprintf("allowedSourceRanges-test-missing-allowedSourceRanges-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + //AllowedSourceRanges: nil, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-missing-allowedSourceRanges-and-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + //machineCIDR: "10.0.0.0/16", + //AllowedSourceRanges: nil, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-empty-allowedSourceRanges-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-empty-ASR-no-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + //machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-include-only-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"10.0.0.0/8"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-include-many-before-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"10.0.0.0/8", "192.168.1.0/24", "172.20.4.0/16"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-include-many-after-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "172.20.4.0/16", "10.0.0.0/8"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-exclude-single-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: false, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-exclude-many-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "192.168.1.0/16", "172.20.4.5/32", "10.0.0.0/17"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: false, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-invalid-network-value-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"10"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: false, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-invalid-input-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"ABC"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: false, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-include-machineCIDR-with-ipv6-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"2001:db8:abcd:1234::1/64", "10.0.0.0/16"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-valid-input-no-machineCIDR-%s", op), + name: "default", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + //machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "10.0.0.0/16"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: false, + errorContains: "", + }, + } + runIngressControllerTests(t, tests) +} + +func TestIngressControllerAllowedSourceRangesCreate(t *testing.T) { + // Test the update and create operations in parallel? + t.Parallel() + runIngressControllerAllowedSourceRangesTests(t, admissionv1.Create) +} + +func TestIngressControllerAllowedSourceRangesUpdate(t *testing.T) { + // Test the update and create operations in parallel? + t.Parallel() + runIngressControllerAllowedSourceRangesTests(t, admissionv1.Update) +} + +func runIngressControllerAllowedSourceRangesNonDefaultTest(t *testing.T, op admissionv1.Operation) { + tests := []ingressControllerTestSuites{ + { + testID: fmt.Sprintf("allowedSourceRanges-test-non-default-exclude-machineCIDR-%s", op), + name: "shiny-newingress", + namespace: "openshift-ingress-operator", + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "172.20.4.0/24"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + errorContains: "", + }, + { + testID: fmt.Sprintf("allowedSourceRanges-test-non-os-namespace-exclude-machineCIDR-%s", op), + name: "default", + namespace: "shiny-newingress-namespace", //May not be a valid test beyond testing the webhook. + username: "admin", + userGroups: []string{"system:authenticated", "cluster-admins"}, + operation: op, + machineCIDR: "10.0.0.0/16", + allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "172.20.4.0/24"}, + nodeSelector: corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + tolerations: []corev1.Toleration{}, + shouldBeAllowed: true, + errorContains: "", + }, + } + runIngressControllerTests(t, tests) +} + +func TestIngressControllerAllowedSourceRangesNonDefaultCreate(t *testing.T) { + runIngressControllerAllowedSourceRangesTests(t, admissionv1.Create) +} +func TestIngressControllerAllowedSourceRangesNonDefaultUpdate(t *testing.T) { + runIngressControllerAllowedSourceRangesTests(t, admissionv1.Update) +} +func TestIngressControllerCheckDeleteSupport(t *testing.T) { + hook := NewWebhook() + rules := hook.Rules() + for _, rule := range rules { + for _, op := range rule.Operations { + if op == admissionregv1.OperationType(admissionv1.Delete) { + t.Fatalf("IngressController web hook is supporting the Delete operation. Replace this check with unit tests supporting 'Delete'") + } + } + } +} From 349e84aa0251f0a81a60ea57103b1ed53c992c91 Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Wed, 21 Aug 2024 07:55:24 -0700 Subject: [PATCH 2/7] OSD-24275: Attempt to fetch and cache machineCidr at Init. Update comments. --- .../ingresscontroller/ingresscontroller.go | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index 5b9f6407..875f671e 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -177,13 +177,13 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm } /* TODO: - * 1) ONLY check for privatelink clusters? How? - * 2) HCP vs Classic, etc.. ...any other cluster type this should apply to? Is this handled by the - * HypershiftEnabled vs ClassicEnabled flags set in this module? Currently HCP disabled. - * If HCP is to be enabled for allowed source ranges, should this part of a 2nd ingress validator to - * allow separation of validations between cluster install types? Is there a run time method available - * to this validator to determine classic vs hcp? - * 3) What other specifics should be checked here for this cidr check to be applicable?m + * - HypershiftEnabled is currently set to false/disabled. + * If HCP is to be enabled for allowed source ranges, should this part of a 2nd ingress validator to + * allow separation of validations between cluster install types? ...or if there's a reliable run time method available + * to this validator to determine classic vs hcp they can remain in the single webhook(?) + * - Classic vs HCP could likely share some of the network funcions, but will need slightly + * different logic as well as permissions fetching the network config info from different + * source (configmap) locations and config formats(?). */ // Only check for machine cidr in allowed ranges if creating or updating resource... reqOp := request.AdmissionRequest.Operation @@ -279,7 +279,7 @@ func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1 // Note: From docs it appears a missing ASR value/attr allows all. However... // once ASR values have been added to an ingresscontroller, later deleting all the ASRs can expose an issue // where the IGC will remaining in progressing state indefinitely. - // For now return Allowed with a warning? + // For now return Allowed, but with a warning? if ipRanges == nil || len(ipRanges) <= 0 { return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'. Populate this value if operator remains in 'progressing' state") } @@ -370,8 +370,12 @@ func NewWebhook() *IngressControllerWebhook { log.Error(err, "Fail adding corev1 scheme to IngressControllerWebhook") os.Exit(1) } - return &IngressControllerWebhook{ + wh := &IngressControllerWebhook{ s: *scheme, kubeClient: nil, } + // Try to populate machine cidr at init. If this fails it will try again on the + // first update/create request involving the default ingress controller. + wh.getMachineCIDR() + return wh } From fdc7a30b8ad1ddb31ce25943dc57d7b2400e5cb2 Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Wed, 11 Sep 2024 20:37:21 -0700 Subject: [PATCH 3/7] Update pkg/webhooks/ingresscontroller/ingresscontroller.go Co-authored-by: Trevor Nierman <31354782+tnierman@users.noreply.github.com> --- pkg/webhooks/ingresscontroller/ingresscontroller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index 875f671e..c2e87f75 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -281,7 +281,7 @@ func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1 // where the IGC will remaining in progressing state indefinitely. // For now return Allowed, but with a warning? if ipRanges == nil || len(ipRanges) <= 0 { - return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'. Populate this value if operator remains in 'progressing' state") + return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'") } machIP, machNet, err := wh.getMachineCIDR() if err != nil { From 5f3ce0a303eaf828a66a2102809914fd9925df93 Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Wed, 11 Sep 2024 20:37:35 -0700 Subject: [PATCH 4/7] Update pkg/webhooks/ingresscontroller/ingresscontroller.go Co-authored-by: Trevor Nierman <31354782+tnierman@users.noreply.github.com> --- pkg/webhooks/ingresscontroller/ingresscontroller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index c2e87f75..7ff3ca98 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -198,7 +198,6 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm return ret } } - log.Info("############# DEBUG LOG: IngressController operation is allowed ###########") ret = admissionctl.Allowed("IngressController operation is allowed, machineCIDR n/a") ret.UID = request.AdmissionRequest.UID From 2aed50ce4a1031dc92f2f19cb00acdd44b10160c Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Tue, 17 Sep 2024 17:52:33 -0700 Subject: [PATCH 5/7] OSD-24275: Read Install Config during WH creation. Allow unit test params, and shared test flag. --- cmd/main.go | 18 ++- go.mod | 1 + go.sum | 2 + .../ingresscontroller/ingresscontroller.go | 149 +++++++++--------- .../ingresscontroller_test.go | 90 +++-------- pkg/webhooks/utils/utils.go | 2 + 6 files changed, 114 insertions(+), 148 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 42b7254b..8e36a713 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,6 +21,7 @@ import ( "github.com/openshift/managed-cluster-validating-webhooks/pkg/k8sutil" "github.com/openshift/managed-cluster-validating-webhooks/pkg/localmetrics" "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks" + "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils" ) var log = logf.Log.WithName("handler") @@ -28,7 +29,7 @@ var log = logf.Log.WithName("handler") var ( listenAddress = flag.String("listen", "0.0.0.0", "listen address") listenPort = flag.String("port", "5000", "port to listen on") - testHooks = flag.Bool("testhooks", false, "Test webhook URI uniqueness and quit?") + metricsAddr string useTLS = flag.Bool("tls", false, "Use TLS? Must specify -tlskey, -tlscert, -cacert") tlsKey = flag.String("tlskey", "", "TLS Key for TLS") @@ -39,15 +40,18 @@ var ( metricsPort = "8080" ) -func main() { - var metricsAddr string +func init() { + // Allow export webhook var to share flag value... + flag.BoolVar(&utils.TestHooks, "testhooks", false, "Test webhook URI uniqueness and quit?") flag.StringVar(&metricsAddr, "metrics-bind-address", ":"+metricsPort, "The address the metric endpoint binds to.") flag.Parse() - klog.SetOutput(os.Stdout) +} +func main() { + klog.SetOutput(os.Stdout) logf.SetLogger(klogr.New()) - if !*testHooks { + if !utils.TestHooks { log.Info("HTTP server running at", "listen", net.JoinHostPort(*listenAddress, *listenPort)) } dispatcher := dispatcher.NewDispatcher(webhooks.Webhooks) @@ -58,12 +62,12 @@ func main() { panic(fmt.Errorf("Duplicate webhook trying to listen on %s", realHook.GetURI())) } seen[name] = true - if !*testHooks { + if !utils.TestHooks { log.Info("Listening", "webhookName", name, "URI", realHook.GetURI()) } http.HandleFunc(realHook.GetURI(), dispatcher.HandleRequest) } - if *testHooks { + if utils.TestHooks { os.Exit(0) } diff --git a/go.mod b/go.mod index 1f4eb507..44db7fe3 100644 --- a/go.mod +++ b/go.mod @@ -51,6 +51,7 @@ require ( github.com/onsi/gomega v1.29.0 // indirect github.com/openshift/custom-resource-status v1.1.3-0.20220503160415-f2fdb4999d87 // indirect github.com/openshift/elasticsearch-operator v0.0.0-20220613183908-e1648e67c298 // indirect + github.com/openshift/installer v0.16.1 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/prometheus/client_model v0.4.0 // indirect github.com/prometheus/common v0.44.0 // indirect diff --git a/go.sum b/go.sum index 2cef43fa..1229218b 100644 --- a/go.sum +++ b/go.sum @@ -181,6 +181,8 @@ github.com/openshift/elasticsearch-operator v0.0.0-20220613183908-e1648e67c298 h github.com/openshift/elasticsearch-operator v0.0.0-20220613183908-e1648e67c298/go.mod h1:6dxhWPY3Wr/0b0eGrFpV7gcyeS+ne48Mo9OQ9dxrLNI= github.com/openshift/hive/apis v0.0.0-20230327212335-7fd70848a6d5 h1:adHXZ1WFqCvXpargpTa6divneeUuvV2xr/D6NWgbqS8= github.com/openshift/hive/apis v0.0.0-20230327212335-7fd70848a6d5/go.mod h1:VIxA5HhvBmsqVn7aUVQYs004B9K4U5A+HrFwvRq2nK8= +github.com/openshift/installer v0.16.1 h1:PmjALN9x1NVNVi3SCqfz0ZwVCgOkQLQWo2nHYXREq/A= +github.com/openshift/installer v0.16.1/go.mod h1:VWGgpJgF8DGCKQjbccnigglhZnHtRLCZ6cxqkXN4Ck0= github.com/openshift/operator-custom-metrics v0.5.1 h1:1pk4YMUV+cmqfV0f2fyxY62cl7Gc76kwudJT+EdcfYM= github.com/openshift/operator-custom-metrics v0.5.1/go.mod h1:0dYDHi/ubKRWzsC9MmW6bRMdBgo1QSOuAh3GupTe0Sw= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index 7ff3ca98..79fde266 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -11,6 +11,7 @@ import ( "strings" operatorv1 "github.com/openshift/api/operator/v1" + installer "github.com/openshift/installer/pkg/types" "github.com/openshift/managed-cluster-validating-webhooks/pkg/k8sutil" "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils" "github.com/pkg/errors" @@ -54,24 +55,11 @@ var ( ) type IngressControllerWebhook struct { - s runtime.Scheme - kubeClient client.Client + s runtime.Scheme // Allow caching install config and machineCIDR values... - machineCIDRIP net.IP machineCIDRNet *net.IPNet } -// installConfig struct to load the min values needed from the install-config data. -// Alternative would be to vendor all of github.com/openshift/installer/pkg/types to import the proper struct. -// Required values: machineCidr info, anything else we gather from this? hcp vs classic, privatelink info, etc? -type installConfig struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata"` - Networking struct { - MachineCIDR string `json:"machineCIDR"` - } `json:"networking"` -} - // ObjectSelector implements Webhook interface func (wh *IngressControllerWebhook) ObjectSelector() *metav1.LabelSelector { return nil } @@ -178,12 +166,10 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm /* TODO: * - HypershiftEnabled is currently set to false/disabled. - * If HCP is to be enabled for allowed source ranges, should this part of a 2nd ingress validator to - * allow separation of validations between cluster install types? ...or if there's a reliable run time method available - * to this validator to determine classic vs hcp they can remain in the single webhook(?) * - Classic vs HCP could likely share some of the network funcions, but will need slightly - * different logic as well as permissions fetching the network config info from different - * source (configmap) locations and config formats(?). + * different logic for the different minimum CIDR sets required, as well as different + * permissions fetching the network config info from different + * source (configmap) locations and config formats(?). */ // Only check for machine cidr in allowed ranges if creating or updating resource... reqOp := request.AdmissionRequest.Operation @@ -204,58 +190,46 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm return ret } -func (wh *IngressControllerWebhook) getMachineCIDR() (net.IP, *net.IPNet, error) { - if wh.machineCIDRIP == nil || wh.machineCIDRNet == nil { - instConf, err := wh.getClusterConfig() - if err != nil { - log.Error(err, "Failed to fetch machineCIDR", "namespace", installConfigNamespace, "configmap", installConfigMap) - return nil, nil, err - } +func (wh *IngressControllerWebhook) getMachineCIDR(instConf *installer.InstallConfig) (*net.IPNet, error) { + if wh.machineCIDRNet == nil { if instConf == nil { err := fmt.Errorf("can not fetch machineCIDR from empty '%s' install config", installConfigMap) log.Error(err, "getMachineCIDR failed to find CIDR value") - return nil, nil, err - } - if len(instConf.Networking.MachineCIDR) <= 0 { - err := fmt.Errorf("empty machineCIDR string value parsed from '%s' install config", installConfigMap) - log.Error(err, "getMachineCIDR found empty CIDR value") - return nil, nil, err + return nil, err } - machIP, machNet, err := net.ParseCIDR(string(instConf.Networking.MachineCIDR)) - if err != nil { - log.Error(err, "err parsing machineCIDR into network cidr", "machineCIDR", string(instConf.Networking.MachineCIDR)) - return nil, nil, err + if instConf.Networking.MachineCIDR == nil { + err := fmt.Errorf("nil installConfig.machineCIDR value found") + log.Error(err, "nil installConfig.machineCIDR value found") + return nil, err } - if machIP == nil || machNet == nil { - err := fmt.Errorf("failed to parse machineCIDR string:'%s' into network structures", string(instConf.Networking.MachineCIDR)) - log.Error(err, "failed to parse install-config machineCIDR") - return nil, nil, err + if len(instConf.Networking.MachineCIDR.Network()) <= 0 || len(instConf.Networking.MachineCIDR.IPNet.Network()) <= 0 { + err := fmt.Errorf("empty machineCIDR network() value parsed from '%s' install config", installConfigMap) + log.Error(err, "getMachineCIDR found empty network value") + return nil, err } // Successfully fetched, parsed, and converted the machineCIDR string into net structures... - wh.machineCIDRIP = machIP - wh.machineCIDRNet = machNet + wh.machineCIDRNet = &instConf.Networking.MachineCIDR.IPNet } - return wh.machineCIDRIP, wh.machineCIDRNet, nil + return wh.machineCIDRNet, nil } /* Fetch the install-config from the kube-system config map's data. * this requires proper role, rolebinding for this service account's get() request * to succeed. (see toplevel selectorsyncset). This config should not change during runtime so - * this operation should cache this if possible. + * this operation should cache the value(s) if possible. * TODO: Should it retry fetching the config if there are any failures/errors encountered while * parsing out the the desired values? */ -func (wh *IngressControllerWebhook) getClusterConfig() (*installConfig, error) { +func (wh *IngressControllerWebhook) getClusterConfig() (*installer.InstallConfig, error) { var err error - if wh.kubeClient == nil { - wh.kubeClient, err = k8sutil.KubeClient(&wh.s) - if err != nil { - log.Error(err, "Fail creating KubeClient for IngressControllerWebhook") - return nil, err - } + + kubeClient, err := k8sutil.KubeClient(&wh.s) + if err != nil { + log.Error(err, "Fail creating KubeClient for IngressControllerWebhook") + return nil, err } clusterConfig := &corev1.ConfigMap{} - err = wh.kubeClient.Get(context.Background(), client.ObjectKey{Name: installConfigMap, Namespace: installConfigNamespace}, clusterConfig) + err = kubeClient.Get(context.Background(), client.ObjectKey{Name: installConfigMap, Namespace: installConfigNamespace}, clusterConfig) if err != nil { log.Error(err, "Failed to fetch configmap: 'cluster-config-v1' for cluster config") return nil, err @@ -264,8 +238,7 @@ func (wh *IngressControllerWebhook) getClusterConfig() (*installConfig, error) { if !ok { return nil, fmt.Errorf("did not find key %s in configmap %s/%s", installConfigKeyName, installConfigNamespace, installConfigMap) } - instConf := &installConfig{} - + instConf := &installer.InstallConfig{} decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader([]byte(data)), 4096) if err := decoder.Decode(instConf); err != nil { return nil, errors.Wrap(err, "failed to decode install config") @@ -280,16 +253,12 @@ func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1 // where the IGC will remaining in progressing state indefinitely. // For now return Allowed, but with a warning? if ipRanges == nil || len(ipRanges) <= 0 { - return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'") - } - machIP, machNet, err := wh.getMachineCIDR() - if err != nil { - // This represents a fault in either the webhook itself, webhook permissions, or install config. - // Might be nice to have an env var etc we can set to allow proceeding w/o the immediate need to roll new code? - return admissionctl.Errored(http.StatusInternalServerError, err) + return admissionctl.Allowed("Allowing empty 'AllowedSourceRanges'.") } - machNetSize, machNetBits := machNet.Mask.Size() - log.Info("Checking AllowedSourceRanges", "MachineCIDR", fmt.Sprintf("%s/%d", machIP.String(), machNetSize), "NetBits", machNetBits, "AllowedSourceRanges", ipRanges) + + machNetSize, machNetBits := wh.machineCIDRNet.Mask.Size() + machineCIDRIP := wh.machineCIDRNet.IP + log.Info("Checking AllowedSourceRanges", "MachineCIDR", fmt.Sprintf("%s/%d", machineCIDRIP.String(), machNetSize), "NetBits", machNetBits, "AllowedSourceRanges", ipRanges) for _, OpV1CIDR := range ipRanges { // Clean up the operatorV1.CIDR value into trimmed CIDR 'a.b.c.d/x' string ASRstring := strings.TrimSpace(string(OpV1CIDR)) @@ -304,20 +273,19 @@ func (wh *IngressControllerWebhook) checkAllowsMachineCIDR(ipRanges []operatorv1 return admissionctl.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse AllowedSourceRanges value: '%s'. Err: %s", string(ASRstring), err)) } // First check if this AlloweSourceRange entry network contains the machine cidr ip... - if !ASRNet.Contains(machIP) { - log.Info(fmt.Sprintf("AllowedSourceRange:'%s' does not contain machine CIDR:'%s/%d'", ASRstring, machIP.String(), machNetSize)) + if !ASRNet.Contains(machineCIDRIP) { + //log.Info(fmt.Sprintf("AllowedSourceRange:'%s' does not contain machine CIDR:'%s/%d'", ASRstring, machineCIDRIP.String(), machNetSize)) continue } // Check if this AlloweSourceRange entry mask includes the network. ASRNetSize, ASRNetBits := ASRNet.Mask.Size() if machNetBits == ASRNetBits && ASRNetSize <= machNetSize { - log.Info(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring)) - return admissionctl.Allowed(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machIP.String(), machNetSize, ASRstring)) - //return admissionctl.Allowed("IngressController operation is allowed. Minimum AllowedSourceRanges are met.") + log.Info(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machineCIDRIP.String(), machNetSize, ASRstring)) + return admissionctl.Allowed(fmt.Sprintf("Found machineCidr:'%s/%d' within AllowedSourceRange:'%s'", machineCIDRIP.String(), machNetSize, ASRstring)) } } - log.Info(fmt.Sprintf("machineCidr:'%s/%d' not found within networks provided by AllowedSourceRanges:'%v'", machIP.String(), machNetSize, ipRanges)) - return admissionctl.Denied(fmt.Sprintf("At least one AllowedSourceRange must allow machine cidr:'%s/%d'", machIP.String(), machNetSize)) + log.Info(fmt.Sprintf("machineCidr:'%s/%d' not found within networks provided by AllowedSourceRanges:'%v'", machineCIDRIP.String(), machNetSize, ipRanges)) + return admissionctl.Denied(fmt.Sprintf("At least one AllowedSourceRange must allow machine cidr:'%s/%d'", machineCIDRIP.String(), machNetSize)) } // isAllowedUser checks if the user is allowed to perform the action @@ -362,7 +330,8 @@ func (s *IngressControllerWebhook) ClassicEnabled() bool { return true } func (s *IngressControllerWebhook) HypershiftEnabled() bool { return false } // NewWebhook creates a new webhook -func NewWebhook() *IngressControllerWebhook { +// Allow variadic args so unit tests can provide optional test values... +func NewWebhook(params ...interface{}) *IngressControllerWebhook { scheme := runtime.NewScheme() err := corev1.AddToScheme(scheme) if err != nil { @@ -370,11 +339,39 @@ func NewWebhook() *IngressControllerWebhook { os.Exit(1) } wh := &IngressControllerWebhook{ - s: *scheme, - kubeClient: nil, + s: *scheme, + } + // utils.TestHooks maps to cli flag 'testhooks' and is used during 'make test' to "test webhook URI uniqueness". + // 'make test' does not require this hook to build runtime clients/config at this time... + if utils.TestHooks { + return wh + } + + if len(params) > 0 { + param := params[0] + // As of know only *IPNet values can be provided by unit tests to set machineCIDR, normal webhook factory + // calls NewWebhook() without arguments... + if cidr, ok := param.(*net.IPNet); ok { + log.Info(fmt.Sprintf("Got test net.IPNet param network() for machineCIDR:'%s'\n", cidr.Network())) + wh.machineCIDRNet = cidr + } else { + log.Error(fmt.Errorf("invalid test param provided, expected *net.IPNet machineCIDR value"), "invalid test param provided, expected *net.IPNet machineCIDR value") + os.Exit(1) + } + } else { + // This is not a test run. + // Try to populate machine cidr at init. Exit with error if this fails... + instConf, err := wh.getClusterConfig() + if err != nil { + log.Error(err, "Failed to fetch configmap for machineCIDR", "namespace", installConfigNamespace, "configmap", installConfigMap) + os.Exit(1) + } + + _, err = wh.getMachineCIDR(instConf) + if err != nil || wh.machineCIDRNet == nil { + log.Error(err, "Failed to fetch cluster machineCIDR.") + os.Exit(1) + } } - // Try to populate machine cidr at init. If this fails it will try again on the - // first update/create request involving the default ingress controller. - wh.getMachineCIDR() return wh } diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller_test.go b/pkg/webhooks/ingresscontroller/ingresscontroller_test.go index 773ca90d..5ddd453e 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller_test.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net" + "os" "testing" operatorv1 "github.com/openshift/api/operator/v1" @@ -100,7 +101,6 @@ type ingressControllerTestSuites struct { allowedSourceRanges []operatorv1.CIDR machineCIDR string shouldBeAllowed bool - errorContains string } func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites) { @@ -115,8 +115,10 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites Resource: "ingresscontroller", } for _, test := range tests { + //fmt.Fprintf(os.Stderr, "Starting test:'%s'\n", test.testID) rawObjString, err := createRawIngressControllerJSON(test.name, test.namespace, test.nodeSelector, test.tolerations, test.allowedSourceRanges) if err != nil { + fmt.Fprintf(os.Stderr, "Couldn't create a JSON fragment %s \n", err.Error()) t.Fatalf("Couldn't create a JSON fragment %s", err.Error()) } @@ -127,25 +129,31 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites oldObj := runtime.RawExtension{ Raw: []byte(rawObjString), } - - hook := NewWebhook() - err = setMachineCidr(t, test, hook) + cidr, err := getMachineCidr(t, test.machineCIDR) if err != nil { + fmt.Fprintf(os.Stderr, "Expected no error, got err parsing machineCIDR:'%s', got '%s'\n", string(test.machineCIDR), err.Error()) t.Fatalf("Expected no error, got err parsing machineCIDR:'%s', got '%s'", string(test.machineCIDR), err.Error()) } + hook := NewWebhook(cidr) httprequest, err := testutils.CreateHTTPRequest(hook.GetURI(), test.testID, gvk, gvr, test.operation, test.username, test.userGroups, "", &obj, &oldObj) if err != nil { t.Logf("Request object:'%s'", obj) + // go test is quirky about muting its logger when run against more than 1 package + fmt.Fprintf(os.Stderr, "Expected no error, got %s \n", err.Error()) t.Fatalf("Expected no error, got %s", err.Error()) } response, err := testutils.SendHTTPRequest(httprequest, hook) if err != nil { + // go test is quirky about muting its logger when run against more than 1 package + fmt.Fprintf(os.Stderr, "Expected no error, got %s", err.Error()) t.Fatalf("Expected no error, got %s", err.Error()) } if response.UID == "" { //t.Logf("Request object:'%s'", obj) + // go test is quirky about muting its logger when run against more than 1 package + fmt.Fprintf(os.Stderr, "No tracking UID associated with the response.") t.Logf("Response object:'%v'", response) t.Fatalf("No tracking UID associated with the response.") } @@ -153,24 +161,23 @@ func runIngressControllerTests(t *testing.T, tests []ingressControllerTestSuites if response.Allowed != test.shouldBeAllowed { t.Logf("%s", obj) t.Logf("Response.reason:'%v'", response) + // go test is quirky about muting its logger when run against more than 1 package + fmt.Fprintf(os.Stderr, "[%s] Mismatch: %s (groups=%s) %s %s the ingress controller. Test's expectation is that the user %s", test.testID, test.username, test.userGroups, testutils.CanCanNot(response.Allowed), test.operation, testutils.CanCanNot(test.shouldBeAllowed)) t.Fatalf("[%s] Mismatch: %s (groups=%s) %s %s the ingress controller. Test's expectation is that the user %s", test.testID, test.username, test.userGroups, testutils.CanCanNot(response.Allowed), test.operation, testutils.CanCanNot(test.shouldBeAllowed)) } + //fmt.Fprintf(os.Stderr, "Finished Success: '%s'\n", test.testID) } } -func setMachineCidr(t *testing.T, test ingressControllerTestSuites, hook *IngressControllerWebhook) error { - if len(test.machineCIDR) > 0 { - ip, net, err := net.ParseCIDR(string(test.machineCIDR)) - if err != nil { - return err - } - hook.machineCIDRIP = ip - hook.machineCIDRNet = net - } else { - hook.machineCIDRIP = nil - hook.machineCIDRNet = nil +func getMachineCidr(t *testing.T, machineCIDR string) (*net.IPNet, error) { + if len(machineCIDR) <= 0 { + machineCIDR = "10.0.0.0/16" + } + _, net, err := net.ParseCIDR(string(machineCIDR)) + if err != nil { + t.Fatalf("Expected no error, got err parsing machineCIDR:'%s', got '%s'", string(machineCIDR), err.Error()) } - return nil + return net, nil } func TestIngressControllerTolerations(t *testing.T) { @@ -418,21 +425,6 @@ func runIngressControllerAllowedSourceRangesTests(t *testing.T, op admissionv1.O tolerations: []corev1.Toleration{}, shouldBeAllowed: true, }, - { - testID: fmt.Sprintf("allowedSourceRanges-test-missing-allowedSourceRanges-and-machineCIDR-%s", op), - name: "default", - namespace: "openshift-ingress-operator", - username: "admin", - userGroups: []string{"system:authenticated", "cluster-admins"}, - operation: op, - //machineCIDR: "10.0.0.0/16", - //AllowedSourceRanges: nil, - nodeSelector: corev1.NodeSelector{ - NodeSelectorTerms: []corev1.NodeSelectorTerm{}, - }, - tolerations: []corev1.Toleration{}, - shouldBeAllowed: true, - }, { testID: fmt.Sprintf("allowedSourceRanges-test-empty-allowedSourceRanges-%s", op), name: "default", @@ -448,21 +440,6 @@ func runIngressControllerAllowedSourceRangesTests(t *testing.T, op admissionv1.O tolerations: []corev1.Toleration{}, shouldBeAllowed: true, }, - { - testID: fmt.Sprintf("allowedSourceRanges-test-empty-ASR-no-machineCIDR-%s", op), - name: "default", - namespace: "openshift-ingress-operator", - username: "admin", - userGroups: []string{"system:authenticated", "cluster-admins"}, - operation: op, - //machineCIDR: "10.0.0.0/16", - allowedSourceRanges: []operatorv1.CIDR{}, - nodeSelector: corev1.NodeSelector{ - NodeSelectorTerms: []corev1.NodeSelectorTerm{}, - }, - tolerations: []corev1.Toleration{}, - shouldBeAllowed: true, - }, { testID: fmt.Sprintf("allowedSourceRanges-test-include-only-machineCIDR-%s", op), name: "default", @@ -583,22 +560,6 @@ func runIngressControllerAllowedSourceRangesTests(t *testing.T, op admissionv1.O tolerations: []corev1.Toleration{}, shouldBeAllowed: true, }, - { - testID: fmt.Sprintf("allowedSourceRanges-test-valid-input-no-machineCIDR-%s", op), - name: "default", - namespace: "openshift-ingress-operator", - username: "admin", - userGroups: []string{"system:authenticated", "cluster-admins"}, - operation: op, - //machineCIDR: "10.0.0.0/16", - allowedSourceRanges: []operatorv1.CIDR{"192.168.1.0/24", "10.0.0.0/16"}, - nodeSelector: corev1.NodeSelector{ - NodeSelectorTerms: []corev1.NodeSelectorTerm{}, - }, - tolerations: []corev1.Toleration{}, - shouldBeAllowed: false, - errorContains: "", - }, } runIngressControllerTests(t, tests) } @@ -631,7 +592,6 @@ func runIngressControllerAllowedSourceRangesNonDefaultTest(t *testing.T, op admi }, tolerations: []corev1.Toleration{}, shouldBeAllowed: true, - errorContains: "", }, { testID: fmt.Sprintf("allowedSourceRanges-test-non-os-namespace-exclude-machineCIDR-%s", op), @@ -647,7 +607,6 @@ func runIngressControllerAllowedSourceRangesNonDefaultTest(t *testing.T, op admi }, tolerations: []corev1.Toleration{}, shouldBeAllowed: true, - errorContains: "", }, } runIngressControllerTests(t, tests) @@ -659,8 +618,9 @@ func TestIngressControllerAllowedSourceRangesNonDefaultCreate(t *testing.T) { func TestIngressControllerAllowedSourceRangesNonDefaultUpdate(t *testing.T) { runIngressControllerAllowedSourceRangesTests(t, admissionv1.Update) } + func TestIngressControllerCheckDeleteSupport(t *testing.T) { - hook := NewWebhook() + hook := NewWebhook(getMachineCidr(t, "")) rules := hook.Rules() for _, rule := range rules { for _, op := range rule.Operations { diff --git a/pkg/webhooks/utils/utils.go b/pkg/webhooks/utils/utils.go index b33fd584..7e1c65be 100644 --- a/pkg/webhooks/utils/utils.go +++ b/pkg/webhooks/utils/utils.go @@ -26,6 +26,8 @@ const ( ) var ( + // Allows sharing of testhooks 'make test' flag value used by main to test "webhook URI uniqueness" + TestHooks bool admissionScheme = runtime.NewScheme() admissionCodecs = serializer.NewCodecFactory(admissionScheme) ) From e566f3c6195c01793b8a77bb7a30ea53f1439ba7 Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Thu, 26 Sep 2024 23:58:06 -0700 Subject: [PATCH 6/7] OSD-24275: Add webhook build flag to allow building outside of cluster env --- build/resources.go | 1 + pkg/webhooks/ingresscontroller/ingresscontroller.go | 2 +- pkg/webhooks/utils/utils.go | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/build/resources.go b/build/resources.go index d1d8fdee..4924aacd 100644 --- a/build/resources.go +++ b/build/resources.go @@ -884,6 +884,7 @@ func sliceContains(needle string, haystack []string) bool { func main() { flag.Parse() + utils.BuildRun = true skip := strings.Split(*excludes, ",") onlyInclude := strings.Split(*only, "") diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index 79fde266..d09721bf 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -343,7 +343,7 @@ func NewWebhook(params ...interface{}) *IngressControllerWebhook { } // utils.TestHooks maps to cli flag 'testhooks' and is used during 'make test' to "test webhook URI uniqueness". // 'make test' does not require this hook to build runtime clients/config at this time... - if utils.TestHooks { + if utils.TestHooks || utils.BuildRun { return wh } diff --git a/pkg/webhooks/utils/utils.go b/pkg/webhooks/utils/utils.go index 7e1c65be..252b920d 100644 --- a/pkg/webhooks/utils/utils.go +++ b/pkg/webhooks/utils/utils.go @@ -28,6 +28,7 @@ const ( var ( // Allows sharing of testhooks 'make test' flag value used by main to test "webhook URI uniqueness" TestHooks bool + BuildRun bool admissionScheme = runtime.NewScheme() admissionCodecs = serializer.NewCodecFactory(admissionScheme) ) From 49fe4ea64bb25b8dcb871e3476be13f52f0c33fd Mon Sep 17 00:00:00 2001 From: Matt Clark Date: Tue, 8 Oct 2024 14:28:31 -0700 Subject: [PATCH 7/7] OSD-24275: Remove comment, update log msg with config var --- pkg/webhooks/ingresscontroller/ingresscontroller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/webhooks/ingresscontroller/ingresscontroller.go b/pkg/webhooks/ingresscontroller/ingresscontroller.go index d09721bf..429ff12f 100644 --- a/pkg/webhooks/ingresscontroller/ingresscontroller.go +++ b/pkg/webhooks/ingresscontroller/ingresscontroller.go @@ -165,16 +165,16 @@ func (wh *IngressControllerWebhook) authorized(request admissionctl.Request) adm } /* TODO: + * - Currently only 'classic' handled by this webhook, and 'hypershift' work may or may not follow once defined. * - HypershiftEnabled is currently set to false/disabled. * - Classic vs HCP could likely share some of the network funcions, but will need slightly - * different logic for the different minimum CIDR sets required, as well as different + * different logic for the different minimum CIDR sets required, different * permissions fetching the network config info from different - * source (configmap) locations and config formats(?). + * source (configmap) locations, and different parsing of config formats, etc.. */ // Only check for machine cidr in allowed ranges if creating or updating resource... reqOp := request.AdmissionRequest.Operation if reqOp == admissionv1.Create || reqOp == admissionv1.Update { - //TODO: Will these need to iterate over more than just the default IngressController config? if ic.ObjectMeta.Name == "default" && ic.ObjectMeta.Namespace == "openshift-ingress-operator" { ret := wh.checkAllowsMachineCIDR(ic.Spec.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges) ret.UID = request.AdmissionRequest.UID @@ -231,7 +231,7 @@ func (wh *IngressControllerWebhook) getClusterConfig() (*installer.InstallConfig clusterConfig := &corev1.ConfigMap{} err = kubeClient.Get(context.Background(), client.ObjectKey{Name: installConfigMap, Namespace: installConfigNamespace}, clusterConfig) if err != nil { - log.Error(err, "Failed to fetch configmap: 'cluster-config-v1' for cluster config") + log.Error(err, fmt.Sprintf("Failed to fetch configmap: '%s' for cluster config", installConfigMap)) return nil, err } data, ok := clusterConfig.Data[installConfigKeyName]