PR feedback

* Make ingressLink and externalService mutually exclusive
* Add missing docs.
* Add service type.
* Make ingressLink depend on reportIngressStatus

Author: Dean-Coakley

deploy/crds/k8s.nginx.org_nginxingresscontrollers_crd.yaml

@@ -69,11 +69,9 @@ spec:
               nullable: true
               type: boolean
             enableLeaderElection:
-              description: 'Specifies the name of the IngressLink resource, which
-                exposes the Ingress Controller pods via a BIG-IP system. The IP of
-                the BIG-IP system is used when reporting the status of Ingress, VirtualServer
-                and VirtualServerRoute resources. For Ingress resources only: Requires
-                -report-ingress-status.'
+              description: Enables Leader election to avoid multiple replicas of the
+                controller reporting the status of Ingress resources – only one replica
+                will report status.
               type: boolean
             enablePreviewPolicies:
               description: Enables preview policies. Requires enableCRDs set to true.
@@ -86,11 +84,6 @@ spec:
               description: Enable TLS Passthrough on port 443. Requires enableCRDs
                 set to true.
               type: boolean
-            extraLabels:
-              additionalProperties:
-                type: string
-              description: Specifies extra labels of the service.
-              type: object
             globalConfiguration:
               description: The GlobalConfiguration resource for global configuration
                 of the Ingress Controller. Format is namespace/name. Requires enableCRDs
@@ -140,11 +133,6 @@ spec:
                 that annotation, which can be disabled by setting UseIngressClassOnly
                 to true. Default is `nginx`.
               type: string
-            ingressLink:
-              description: Enables Leader election to avoid multiple replicas of the
-                controller reporting the status of Ingress resources – only one replica
-                will report status.
-              type: string
             logLevel:
               description: Log level for V logs. Format is 0 - 3
               maximum: 3
@@ -222,9 +210,25 @@ spec:
                     status of Ingress resources. Note: Only if ServiceType is different
                     than LoadBalancer.'
                   type: string
+                ingressLink:
+                  description: Specifies the name of the IngressLink resource, which
+                    exposes the Ingress Controller pods via a BIG-IP system. The IP
+                    of the BIG-IP system is used when reporting the status of Ingress,
+                    VirtualServer and VirtualServerRoute resources. For Ingress resources
+                    only. Requires reportIngressStatus.Enable set to true.
+                  type: string
               required:
               - enable
               type: object
+            service:
+              description: The service of the Ingress controller.
+              properties:
+                extraLabels:
+                  additionalProperties:
+                    type: string
+                  description: Specifies extra labels of the service.
+                  type: object
+              type: object
             serviceType:
               description: 'The type of the Service for the Ingress Controller. Valid
                 Service types are: NodePort and LoadBalancer.'

docs/nginx-ingress-controller.md

@@ -61,6 +61,7 @@ spec:
    reportIngressStatus:
      enable: true
      externalService: my-nginx-ingress
+     ingressLink: my-ingresslink
    prometheus:
      enable: true
      port: 9114
@@ -85,6 +86,7 @@ spec:
 | `enableCRDs` | `boolean` | Enables the use of NGINX Ingress Resource Definitions (VirtualServer and VirtualServerRoute). | No |
 | `enableSnippets` | `boolean` | Enable custom NGINX configuration snippets in VirtualServer and VirtualServerRoute resources. Requires enableCRDs set to true. | No |
 | `ingressClass` | `string` | A class of the Ingress controller. For Kubernetes >= 1.18, the Ingress controller only processes resources that belong to its class - i.e. have the "ingressClassName" field resource equal to the class. Additionally the Ingress Controller processes all the VirtualServer/VirtualServerRoute resources that do not have the "ingressClassName" field. For Kubernetes < 1.18, the Ingress Controller only processes resources that belong to its class - i.e have the annotation "kubernetes.io/ingress.class" (for Ingress resources) or field "ingressClassName" (for VirtualServer/VirtualServerRoute resources) equal to the class. Additionally, the Ingress Controller processes resources that do not have the class set, which can be disabled by setting `useIngressClassOnly` to `true`. Default is `nginx`. | No |
+| `service` | [service](#nginxingresscontrollerservice) | The service of the Ingress Controller. | No |
 | `useIngressClassOnly` | `boolean` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. For kubernetes versions >= 1.18 this flag will be IGNORED. | No |
 | `watchNamespace` | `boolean` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | No |
 | `healthStatus` | [healthStatus](#nginxingresscontrollerhealthstatus) | Adds a new location to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress Controller. | No |
@@ -124,12 +126,19 @@ spec:
 | `port` | `int` | Set the port where the NGINX stub_status or the NGINX Plus API is exposed. Default is `8080`. Format is `1023 - 65535` | No |
 | `allowCidrs` | `string` | Whitelist IPv4 IP/CIDR blocks to allow access to NGINX stub_status or the NGINX Plus API. Separate multiple IP/CIDR by commas. (default `127.0.0.1`) | No |
 
+## NginxIngressController.Service
+
+| Field | Type | Description | Required |
+| --- | --- | --- | --- |
+| `extraLabels` | `map[string]string` | Specifies extra labels of the service. | No |
+
 ## NginxIngressController.ReportIngressStatus
 
 | Field | Type | Description | Required |
 | --- | --- | --- | --- |
 | `enable` | `boolean` | Enable reporting of the Ingress status. | Yes |
 | `externalService` | `string` | Specifies the name of the service with the type LoadBalancer through which the Ingress controller pods are exposed externally. The external address of the service is used when reporting the status of Ingress resources. Note: Only if ServiceType is different than LoadBalancer. | No |
+| `ingressLink` | `string` | Specifies the name of the IngressLink resource, which exposes the Ingress Controller pods via a BIG-IP system. The IP of the BIG-IP system is used when reporting the status of Ingress, VirtualServer and VirtualServerRoute resources. For Ingress resources only. Requires `reportIngressStatus.enable` set to `true`. | No |
 
 ## NginxIngressController.Prometheus
 

pkg/apis/k8s/v1alpha1/nginxingresscontroller_types.go

@@ -53,10 +53,10 @@ type NginxIngressControllerSpec struct {
 	// +kubebuilder:validation:Optional
 	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
 	IngressClass string `json:"ingressClass"`
-	// Specifies extra labels of the service.
+	// The service of the Ingress controller.
 	// +kubebuilder:validation:Optional
 	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
-	ExtraLabels map[string]string `json:"extraLabels,omitempty"`
+	Service *Service `json:"service"`
 	// Ignore Ingress resources without the “kubernetes.io/ingress.class” annotation.
 	// +kubebuilder:validation:Optional
 	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
@@ -96,12 +96,6 @@ type NginxIngressControllerSpec struct {
 	// – only one replica will report status.
 	// +kubebuilder:validation:Optional
 	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
-	IngressLink string `json:"ingressLink,omitempty"`
-	// Specifies the name of the IngressLink resource, which exposes the Ingress Controller pods via a BIG-IP system.
-	// The IP of the BIG-IP system is used when reporting the status of Ingress, VirtualServer and VirtualServerRoute resources.
-	// For Ingress resources only: Requires -report-ingress-status.
-	// +kubebuilder:validation:Optional
-	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
 	EnableLeaderElection bool `json:"enableLeaderElection"`
 	// A Secret with a TLS certificate and key for TLS termination of every Ingress host for which TLS termination is enabled but the Secret is not specified.
 	// The secret must be of the type kubernetes.io/tls.
@@ -198,6 +192,12 @@ type ReportIngressStatus struct {
 	// Note: Only if ServiceType is different than LoadBalancer.
 	// +kubebuilder:validation:Optional
 	ExternalService string `json:"externalService"`
+	// Specifies the name of the IngressLink resource, which exposes the Ingress Controller pods via a BIG-IP system.
+	// The IP of the BIG-IP system is used when reporting the status of Ingress, VirtualServer and VirtualServerRoute resources.
+	// For Ingress resources only. Requires reportIngressStatus.Enable set to true.
+	// +kubebuilder:validation:Optional
+	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
+	IngressLink string `json:"ingressLink,omitempty"`
 }
 
 // Prometheus defines the Prometheus metrics for the Ingress Controller.
@@ -213,12 +213,19 @@ type Prometheus struct {
 	Port *uint16 `json:"port"`
 }
 
-// App Protect support configuration.
+// AppProtect support configuration.
 type AppProtect struct {
 	// Enable App Protect.
 	Enable bool `json:"enable"`
 }
 
+// Service defines the Service for the Ingress Controller.
+type Service struct {
+	// Specifies extra labels of the service.
+	// +kubebuilder:validation:Optional
+	ExtraLabels map[string]string `json:"extraLabels,omitempty"`
+}
+
 // NginxIngressControllerStatus defines the observed state of NginxIngressController
 type NginxIngressControllerStatus struct {
 	// Deployed is true if the Operator has finished the deployment of the NginxIngressController.

pkg/apis/k8s/v1alpha1/zz_generated.deepcopy.go

@@ -12,7 +12,7 @@ func serviceForNginxIngressController(instance *k8sv1alpha1.NginxIngressControll
 		ObjectMeta: v1.ObjectMeta{
 			Name:      instance.Name,
 			Namespace: instance.Namespace,
-			Labels:    instance.Spec.ExtraLabels,
+			Labels:    instance.Spec.Service.ExtraLabels,
 		},
 		Spec: corev1.ServiceSpec{
 			Ports: []corev1.ServicePort{

pkg/controller/nginxingresscontroller/service.go

@@ -24,7 +24,9 @@ func TestServiceForNginxIngressController(t *testing.T) {
 		},
 		Spec: k8sv1alpha1.NginxIngressControllerSpec{
 			ServiceType: serviceType,
-			ExtraLabels: extraLabels,
+			Service: &k8sv1alpha1.Service{
+				ExtraLabels: extraLabels,
+			},
 		},
 	}
 	expected := &corev1.Service{

pkg/controller/nginxingresscontroller/service_test.go

@@ -90,10 +90,8 @@ func generatePodArgs(instance *k8sv1alpha1.NginxIngressController) []string {
 			args = append(args, fmt.Sprintf("-external-service=%v", instance.Spec.ReportIngressStatus.ExternalService))
 		} else if instance.Spec.ServiceType == "LoadBalancer" {
 			args = append(args, fmt.Sprintf("-external-service=%v", instance.Name))
-		}
-
-		if instance.Spec.IngressLink != "" {
-			args = append(args, fmt.Sprintf("-ingresslink=%v", instance.Spec.IngressLink))
+		} else if instance.Spec.ReportIngressStatus.IngressLink != "" {
+			args = append(args, fmt.Sprintf("-ingresslink=%v", instance.Spec.ReportIngressStatus.IngressLink))
 		}
 	}
 

pkg/controller/nginxingresscontroller/utils.go

@@ -111,9 +111,35 @@ func TestGeneratePodArgs(t *testing.T) {
 					Namespace: namespace,
 				},
 				Spec: k8sv1alpha1.NginxIngressControllerSpec{
-					DefaultSecret:       "my-nginx-ingress/my-secret",
-					ServiceType:         "LoadBalancer",
-					ReportIngressStatus: &k8sv1alpha1.ReportIngressStatus{Enable: true},
+					DefaultSecret: "my-nginx-ingress/my-secret",
+					ServiceType:   "NodePort",
+					ReportIngressStatus: &k8sv1alpha1.ReportIngressStatus{
+						Enable:      true,
+						IngressLink: "my-ingresslink",
+					},
+				},
+			},
+			expected: []string{
+				"-nginx-configmaps=my-nginx-ingress/my-nginx-ingress",
+				"-default-server-tls-secret=my-nginx-ingress/my-secret",
+				"-enable-custom-resources=false",
+				"-report-ingress-status",
+				"-ingresslink=my-ingresslink",
+			},
+		},
+		{
+			instance: &k8sv1alpha1.NginxIngressController{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: namespace,
+				},
+				Spec: k8sv1alpha1.NginxIngressControllerSpec{
+					DefaultSecret: "my-nginx-ingress/my-secret",
+					ServiceType:   "LoadBalancer",
+					ReportIngressStatus: &k8sv1alpha1.ReportIngressStatus{
+						Enable:      true,
+						IngressLink: "my-invalid-ingresslink",
+					},
 				},
 			},
 			expected: []string{
@@ -173,8 +199,8 @@ func TestGeneratePodArgs(t *testing.T) {
 					ReportIngressStatus: &k8sv1alpha1.ReportIngressStatus{
 						Enable:          true,
 						ExternalService: "external",
+						IngressLink:     "my-invalid-ingressLink",
 					},
-					IngressLink:          "my-ingresslink",
 					EnableLeaderElection: true,
 					WildcardTLS:          "my-nginx-ingress/wildcard-secret",
 					Prometheus: &k8sv1alpha1.Prometheus{
@@ -211,7 +237,6 @@ func TestGeneratePodArgs(t *testing.T) {
 				"-nginx-status-allow-cidrs=127.0.0.1",
 				"-report-ingress-status",
 				"-external-service=external",
-				"-ingresslink=my-ingresslink",
 				"-enable-leader-election",
 				"-wildcard-tls-secret=my-nginx-ingress/wildcard-secret",
 				"-enable-prometheus-metrics",