diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index 730c12ab..76345902 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -3,7 +3,6 @@ package v1alpha1 import ( "context" "fmt" - "unicode" "github.com/go-logr/logr" netv1 "k8s.io/api/networking/v1" @@ -19,14 +18,8 @@ func init() { } const ( - maxNameLength = 100 // MaxCollisions is the maximum number of collisions allowed when generating a name for a managed resource. This corresponds to the status.CollisionCount - MaxCollisions = 5 - maxControllerNamePrefix = 253 - 10 // 253 is the max length of resource names - 10 to account for the length of the suffix https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names -) - -const ( - defaultControllerNamePrefix = "nginx" + MaxCollisions = 5 ) // Important: Run "make crd" to regenerate code after modifying this file @@ -177,49 +170,6 @@ func (n *NginxIngressController) SetCondition(c metav1.Condition) { meta.SetStatusCondition(&n.Status.Conditions, c) } -// Valid checks this NginxIngressController to see if it's valid. Returns a string describing the validation error, if any, or empty string if there is no error. -func (n *NginxIngressController) Valid() string { - // controller name prefix must follow https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names - // we don't check for ending because this is a prefix - if n.Spec.ControllerNamePrefix == "" { - return "spec.controllerNamePrefix must be specified" - } - - if !isLowercaseRfc1123Subdomain(n.Spec.ControllerNamePrefix) { - return "spec.controllerNamePrefix " + lowercaseRfc1123SubdomainValidationFailReason - } - - if len(n.Spec.ControllerNamePrefix) > maxControllerNamePrefix { - return fmt.Sprintf("spec.controllerNamePrefix length must be less than or equal to %d characters", maxControllerNamePrefix) - - } - - if n.Spec.IngressClassName == "" { - return "spec.ingressClassName must be specified" - } - - if !isLowercaseRfc1123Subdomain(n.Spec.IngressClassName) { - return "spec.ingressClassName " + lowercaseRfc1123SubdomainValidationFailReason - } - - if len(n.Name) > maxNameLength { - return fmt.Sprintf("Name length must be less than or equal to %d characters", maxNameLength) - } - - return "" -} - -// Default sets default spec values for this NginxIngressController -func (n *NginxIngressController) Default() { - if n.Spec.IngressClassName == "" { - n.Spec.IngressClassName = n.Name - } - - if n.Spec.ControllerNamePrefix == "" { - n.Spec.ControllerNamePrefix = defaultControllerNamePrefix - } -} - // Collides returns whether the fields in this NginxIngressController would collide with an existing resources making it // impossible for this NginxIngressController to become available. This should be run before an NginxIngressController is created. // Returns whether there's a collision, the collision reason, and an error if one occurred. The collision reason is something that @@ -275,61 +225,3 @@ type NginxIngressControllerList struct { metav1.ListMeta `json:"metadata,omitempty"` Items []NginxIngressController `json:"items"` } - -var lowercaseRfc1123SubdomainValidationFailReason = "must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" - -func isLowercaseRfc1123Subdomain(s string) bool { - if !startsWithAlphaNum(s) { - return false - } - - if !endsWithAlphaNum(s) { - return false - } - - if !onlyAlphaNumDashPeriod(s) { - return false - } - - if !isLower(s) { - return false - } - - return true -} - -func startsWithAlphaNum(s string) bool { - if len(s) == 0 { - return false - } - - return unicode.IsLetter(rune(s[0])) || unicode.IsDigit(rune(s[0])) -} - -func endsWithAlphaNum(s string) bool { - if len(s) == 0 { - return false - } - - return unicode.IsLetter(rune(s[len(s)-1])) || unicode.IsDigit(rune(s[len(s)-1])) -} - -func onlyAlphaNumDashPeriod(s string) bool { - for _, c := range s { - if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '-' && c != '.' { - return false - } - } - - return true -} - -func isLower(s string) bool { - for _, c := range s { - if unicode.IsUpper(c) && unicode.IsLetter(c) { - return false - } - } - - return true -} diff --git a/api/v1alpha1/nginxingresscontroller_types_test.go b/api/v1alpha1/nginxingresscontroller_types_test.go index a14e88fc..28728991 100644 --- a/api/v1alpha1/nginxingresscontroller_types_test.go +++ b/api/v1alpha1/nginxingresscontroller_types_test.go @@ -3,8 +3,6 @@ package v1alpha1 import ( "context" "errors" - "fmt" - "strings" "testing" "github.com/stretchr/testify/require" @@ -30,188 +28,6 @@ func validNginxIngressController() NginxIngressController { } } -func TestNginxIngressControllerValid(t *testing.T) { - cases := []struct { - name string - nic NginxIngressController - want string - }{ - { - name: "valid NginxIngressController", - nic: validNginxIngressController(), - want: "", - }, - { - name: "missing controller name prefix", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = "" - return nic - }(), - want: "spec.controllerNamePrefix must be specified", - }, - { - name: "controller name prefix starts with non alphanum", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = "-controllernameprefix" - return nic - }(), - want: "spec.controllerNamePrefix must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "controller name prefix contains invalid characters", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = "controllernameprefix!" - return nic - }(), - want: "spec.controllerNamePrefix must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "controller name prefix too long", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = strings.Repeat("a", maxControllerNamePrefix+1) - return nic - }(), - want: fmt.Sprintf("spec.controllerNamePrefix length must be less than or equal to %d characters", maxControllerNamePrefix), - }, - { - name: "controller name prefix capitalized", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = "ControllerNamePrefix" - return nic - }(), - want: "spec.controllerNamePrefix must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "missing ingress class name", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "" - return nic - }(), - want: "spec.ingressClassName must be specified", - }, - { - name: "ingress class name capitalized", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "IngressClassName" - return nic - }(), - want: "spec.ingressClassName must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "ingress class name capitalized with special characters", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "ingress-Class.Name" - return nic - }(), - want: "spec.ingressClassName must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "ingress class name with special characters", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "ingress-class.name" - return nic - }(), - want: "", - }, - { - name: "ingress class name starts with non alphanum", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "-ingressclassname" - return nic - }(), - want: "spec.ingressClassName must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "ingress class name contains invalid characters", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "ingressclassname!" - return nic - }(), - want: "spec.ingressClassName must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "ingress class name ends with non alphanum", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "ingressclassname-" - return nic - }(), - want: "spec.ingressClassName must be a lowercase RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", - }, - { - name: "long name", - nic: func() NginxIngressController { - nic := validNginxIngressController() - nic.ObjectMeta.Name = strings.Repeat("a", maxNameLength+1) - return nic - }(), - want: fmt.Sprintf("Name length must be less than or equal to %d characters", maxNameLength), - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := c.nic.Valid() - if got != c.want { - t.Errorf("NginxIngressController.Valid() = %v, want %v", got, c.want) - } - }) - } -} - -func TestNginxIngressControllerDefault(t *testing.T) { - t.Run("default ingress class name", func(t *testing.T) { - nic := validNginxIngressController() - nic.Spec.IngressClassName = "" - nic.Default() - expected := nic.Name - if nic.Spec.IngressClassName != expected { - t.Errorf("NginxIngressController.Default() = %v, want %v", nic.Spec.IngressClassName, expected) - } - }) - - t.Run("default controller name prefix", func(t *testing.T) { - nic := validNginxIngressController() - nic.Spec.ControllerNamePrefix = "" - nic.Default() - expected := defaultControllerNamePrefix - if nic.Spec.ControllerNamePrefix != expected { - t.Errorf("NginxIngressController.Default() = %v, want %v", nic.Spec.ControllerNamePrefix, expected) - } - }) - - t.Run("doesn't overwrite ingress class name", func(t *testing.T) { - nic := validNginxIngressController() - existingIngressClassName := "existingIngressClassName" - nic.Spec.IngressClassName = existingIngressClassName - nic.Default() - if nic.Spec.IngressClassName != existingIngressClassName { - t.Errorf("NginxIngressController.Default() = %v, want %v", nic.Spec.IngressClassName, existingIngressClassName) - } - }) - - t.Run("doesn't overwrite controller name prefix", func(t *testing.T) { - nic := validNginxIngressController() - existingControllerNamePrefix := "existingControllerNamePrefix" - nic.Spec.ControllerNamePrefix = existingControllerNamePrefix - nic.Default() - if nic.Spec.ControllerNamePrefix != existingControllerNamePrefix { - t.Errorf("NginxIngressController.Default() = %v, want %v", nic.Spec.ControllerNamePrefix, existingControllerNamePrefix) - } - }) -} - func TestNginxIngressControllerGetCondition(t *testing.T) { nic := validNginxIngressController() cond := metav1.Condition{ @@ -463,308 +279,3 @@ func TestNginxIngressControllerCollides(t *testing.T) { require.True(t, errors.Is(err, getErr), "expected error \"%v\", to be type \"%v\"", err, getErr) }) } - -func TestStartsWithAlphaNum(t *testing.T) { - cases := []struct { - name string - s string - want bool - }{ - { - name: "starts with alpha", - s: "a", - want: true, - }, - { - name: "starts with num", - s: "1", - want: true, - }, - { - name: "empty", - s: "", - want: false, - }, - { - name: "longer starts with alpha", - s: "abc23", - want: true, - }, - { - name: "longer starts with num", - s: "123abc", - want: true, - }, - { - name: "starts with dash", - s: "-abc", - want: false, - }, - { - name: "starts with period", - s: ".123", - want: false, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := startsWithAlphaNum(c.s) - if got != c.want { - t.Errorf("startsWithAlphaNum(%v) = %v, want %v", c.s, got, c.want) - } - }) - } -} - -func TestEndsWithAlphaNum(t *testing.T) { - cases := []struct { - name string - s string - want bool - }{ - { - name: "ends with alpha", - s: "a", - want: true, - }, - { - name: "ends with num", - s: "1", - want: true, - }, - { - name: "empty", - s: "", - want: false, - }, - { - name: "longer ends with alpha", - s: "abc23b", - want: true, - }, - { - name: "longer ends with num", - s: "123abc22", - want: true, - }, - { - name: "ends with dash", - s: "abc-", - want: false, - }, - { - name: "ends with period", - s: "123.", - want: false, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := endsWithAlphaNum(c.s) - if got != c.want { - t.Errorf("endsWithAlphaNum(%v) = %v, want %v", c.s, got, c.want) - } - }) - } -} - -func TestOnlyAlphaNumDashPeriod(t *testing.T) { - cases := []struct { - name string - s string - want bool - }{ - { - name: "only alpha", - s: "abc", - want: true, - }, - { - name: "only num", - s: "123", - want: true, - }, - { - name: "only dash", - s: "---", - want: true, - }, - { - name: "only period", - s: "...", - want: true, - }, - { - name: "alpha num dash period", - s: "abc123.-", - want: true, - }, - { - name: "empty", - s: "", - want: true, - }, - { - name: "alpha num dash period with space", - s: "abc 123.-", - want: false, - }, - { - name: "alpha num dash period with underscore", - s: "abc_123.-", - want: false, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := onlyAlphaNumDashPeriod(c.s) - if got != c.want { - t.Errorf("onlyAlphaNumDashPeriod(%v) = %v, want %v", c.s, got, c.want) - } - }) - } -} - -func TestIsLower(t *testing.T) { - cases := []struct { - name string - s string - want bool - }{ - { - name: "lower", - s: "abc", - want: true, - }, - { - name: "upper", - s: "ABC", - want: false, - }, - { - name: "mixed", - s: "AbC", - want: false, - }, - { - name: "empty", - s: "", - want: true, - }, - { - name: "lower with space", - s: "abc ", - want: true, - }, - { - name: "lower with underscore", - s: "abc_", - want: true, - }, - { - name: "lower with dash", - s: "abc-", - want: true, - }, - { - name: "lower with period", - s: "abc.", - want: true, - }, - { - name: "upper with space", - s: "ABC ", - want: false, - }, - { - name: "upper with underscore", - s: "ABC_", - want: false, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := isLower(c.s) - if got != c.want { - t.Errorf("isLower(%v) = %v, want %v", c.s, got, c.want) - } - }) - } -} - -func TestIsLowercaseRfc1123Subdomain(t *testing.T) { - cases := []struct { - name string - s string - want bool - }{ - { - name: "lower", - s: "abc", - want: true, - }, - { - name: "upper", - s: "ABC", - want: false, - }, - { - name: "mixed", - s: "AbC", - want: false, - }, - { - name: "empty", - s: "", - want: false, - }, - { - name: "lower with space", - s: "abc ", - want: false, - }, - { - name: "lower with ending underscore", - s: "abc_", - want: false, - }, - { - name: "lower with ending dash", - s: "abc-", - want: false, - }, - { - name: "lower with ending period", - s: "abc.", - want: false, - }, - { - name: "lower with middle underscore", - s: "ab_c", - want: false, - }, - { - name: "lower with middle dash", - s: "ab-c", - want: true, - }, - { - name: "lower with middle period", - s: "ab.c", - want: true, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - got := isLowercaseRfc1123Subdomain(c.s) - if got != c.want { - t.Errorf("isLowercaseRfc1123Subdomain(%v) = %v, want %v", c.s, got, c.want) - } - }) - } -} diff --git a/pkg/config/config.go b/pkg/config/config.go index 9ba24c71..9807ce4c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -41,18 +41,9 @@ func init() { flag.StringVar(&Flags.MetricsAddr, "metrics-addr", "0.0.0.0:8081", "address to serve Prometheus metrics on") flag.StringVar(&Flags.ProbeAddr, "probe-addr", "0.0.0.0:8080", "address to serve readiness/liveness probes on") flag.StringVar(&Flags.OperatorDeployment, "operator-deployment", "app-routing-operator", "name of the operator's k8s deployment") - flag.StringVar(&Flags.OperatorNs, "operator-namespace", "kube-system", "namespace of the operator's k8s deployment") - flag.StringVar(&Flags.OperatorWebhookService, "operator-webhook-service", "", "name of the operator's webhook service") - flag.StringVar(&Flags.OperatorWebhookServiceUrl, "operator-webhook-service-url", "", "url of the operator's webhook service") - flag.IntVar(&Flags.WebhookPort, "webhook-port", 9443, "port to serve the webhook on") flag.StringVar(&Flags.ClusterUid, "cluster-uid", "", "unique identifier of the cluster the add-on belongs to") flag.DurationVar(&Flags.DnsSyncInterval, "dns-sync-interval", defaultDnsSyncInterval, "interval at which to sync DNS records") flag.StringVar(&Flags.CrdPath, "crd", "/crd", "location of the CRD manifests. manifests should be directly in this directory, not in a subdirectory") - flag.StringVar(&Flags.CertDir, "cert-dir", "/tmp/k8s-webhook-server/serving-certs", "location of the certificates") - flag.StringVar(&Flags.CertName, "cert-name", "tls.crt", "name of the certificate file in the cert-dir") - flag.StringVar(&Flags.KeyName, "key-name", "tls.key", "name of the key file in the cert-dir") - flag.StringVar(&Flags.CaName, "ca-name", "ca.crt", "name of the CA file in the cert-dir") - flag.BoolVar(&Flags.EnableWebhook, "enable-webhook", false, "enable the webhook server") } type DnsZoneConfig struct { @@ -72,44 +63,13 @@ type Config struct { ConcurrencyWatchdogThres float64 ConcurrencyWatchdogVotes int DisableOSM bool - OperatorNs string OperatorDeployment string - OperatorWebhookService string - OperatorWebhookServiceUrl string - WebhookPort int ClusterUid string DnsSyncInterval time.Duration CrdPath string - CertDir string - CertName, KeyName, CaName string - EnableWebhook bool } func (c *Config) Validate() error { - if c.EnableWebhook { - if c.OperatorNs == "" { - return errors.New("--operator-namespace is required") - } - if c.OperatorWebhookService == "" && c.OperatorWebhookServiceUrl == "" { - return errors.New("--operator-webhook-service or operator-webhook-service-url is required") - } - if c.OperatorWebhookService != "" && c.OperatorWebhookServiceUrl != "" { - return errors.New("only one of --operator-webhook-service or --operator-webhook-service-url should be specified") - } - if c.CertDir == "" { - return errors.New("--cert-dir is required") - } - if c.CertName == "" { - return errors.New("--cert-name is required") - } - if c.KeyName == "" { - return errors.New("--key-name is required") - } - if c.CaName == "" { - return errors.New("--ca-name is required") - } - } - if c.NS == "" { return errors.New("--namespace is required") } @@ -134,9 +94,6 @@ func (c *Config) Validate() error { if c.ConcurrencyWatchdogVotes < 1 { return errors.New("--concurrency-watchdog-votes must be a positive number") } - if c.WebhookPort == 0 { - return errors.New("--webhook-port is required") - } if c.OperatorDeployment == "" { return errors.New("--operator-deployment is required") } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index cbd55a16..2132e90b 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -37,177 +37,11 @@ var validateTestCases = []struct { ConcurrencyWatchdogVotes: 2, ClusterUid: "cluster-uid", OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, CrdPath: validCrdPath, }, }, - { - Name: "valid-minimal-with-webhooks", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - }, { Name: "valid-full", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, - CrdPath: validCrdPath, - }, - }, - { - Name: "valid-full-with-webhooks", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - }, - { - Name: "webhook service url but not webhook service", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookServiceUrl: "app-routing-operator-webhook.namespace.svc", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - }, - { - Name: "webhook service url and webhook service", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - OperatorWebhookServiceUrl: "app-routing-operator-webhook.namespace.svc", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - Error: "only one of --operator-webhook-service or --operator-webhook-service-url should be specified", - }, - { - Name: "webhook service url and webhook service, disabled webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - OperatorWebhookServiceUrl: "app-routing-operator-webhook.namespace.svc", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: false, - }, - }, - { - Name: "missing webhook port", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - Error: "--webhook-port is required", - }, - { - Name: "missing operator ns", Conf: &Config{ NS: "test-namespace", Registry: "test-registry", @@ -219,16 +53,8 @@ var validateTestCases = []struct { ConcurrencyWatchdogVotes: 2, ClusterUid: "test-cluster-uid", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, }, - Error: "--operator-namespace is required", }, { Name: "missing operator deployment", @@ -242,109 +68,10 @@ var validateTestCases = []struct { ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--operator-deployment is required", }, - { - Name: "missing operator webhook service", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - Error: "--operator-webhook-service or operator-webhook-service-url is required", - }, - { - Name: "missing operator webhook service, disabled webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, - CertDir: "/certs", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - }, - }, - { - Name: "missing cert dir", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, - OperatorWebhookService: "app-routing-operator-webhook", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - EnableWebhook: true, - }, - Error: "--cert-dir is required", - }, - { - Name: "missing cert dir, disabled webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - WebhookPort: 9443, - OperatorWebhookService: "app-routing-operator-webhook", - CrdPath: validCrdPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", - }, - }, { Name: "nonexistent crd path", Conf: &Config{ @@ -357,15 +84,8 @@ var validateTestCases = []struct { ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", CrdPath: notAValidPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: fmt.Sprintf("crd path %s does not exist", notAValidPath), }, @@ -381,15 +101,8 @@ var validateTestCases = []struct { ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, ClusterUid: "test-cluster-uid", - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", CrdPath: notADirectoryPath, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: fmt.Sprintf("crd path %s is not a directory", notADirectoryPath), }, @@ -403,15 +116,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--namespace is required", }, @@ -425,15 +131,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--registry is required", }, @@ -447,15 +146,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--msi is required", }, @@ -469,15 +161,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--tenant-id is required", }, @@ -491,15 +176,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--cloud is required", }, @@ -513,15 +191,8 @@ var validateTestCases = []struct { Cloud: "test-cloud", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--location is required", }, @@ -536,15 +207,8 @@ var validateTestCases = []struct { Location: "test-location", ConcurrencyWatchdogThres: 100, ConcurrencyWatchdogVotes: 2, - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ClusterUid: "test-cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--concurrency-watchdog-threshold must be greater than 100", }, @@ -557,15 +221,8 @@ var validateTestCases = []struct { TenantID: "test-tenant-id", Cloud: "test-cloud", Location: "test-location", - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ConcurrencyWatchdogThres: 101, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--concurrency-watchdog-votes must be a positive number", }, @@ -578,16 +235,9 @@ var validateTestCases = []struct { TenantID: "test-tenant-id", Cloud: "test-cloud", Location: "test-location", - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "--cluster-uid is required", }, @@ -600,150 +250,14 @@ var validateTestCases = []struct { TenantID: "test-tenant-id", Cloud: "test-cloud", Location: "test-location", - OperatorNs: "kube-system", OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", ConcurrencyWatchdogThres: 101, ConcurrencyWatchdogVotes: 2, ClusterUid: "cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CaName: "test-ca-name", }, Error: "while parsing dns zone resource ID invalid: parsing failed for invalid. Invalid resource Id format", DnsZone: "invalid,dns,zone", }, - { - Name: "missing-cert-name", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - EnableWebhook: true, - }, - Error: "--cert-name is required", - }, - { - Name: "missing-cert-name, disable webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - CrdPath: validCrdPath, - }, - }, - { - Name: "missing-key-name", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - CertName: "test-cert-name", - EnableWebhook: true, - }, - Error: "--key-name is required", - }, - { - Name: "missing-key-name, disabled webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - CertName: "test-cert-name", - CrdPath: validCrdPath, - }, - }, - { - Name: "missing-ca-name", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - EnableWebhook: true, - }, - Error: "--ca-name is required", - }, - { - Name: "missing-ca-name, disabled webhook", - Conf: &Config{ - NS: "test-namespace", - Registry: "test-registry", - MSIClientID: "test-msi-client-id", - TenantID: "test-tenant-id", - Cloud: "test-cloud", - Location: "test-location", - OperatorNs: "kube-system", - OperatorDeployment: "app-routing-operator", - OperatorWebhookService: "app-routing-operator-webhook", - WebhookPort: 9443, - CertDir: "/certs", - ConcurrencyWatchdogThres: 101, - ConcurrencyWatchdogVotes: 2, - ClusterUid: "cluster-uid", - CertName: "test-cert-name", - KeyName: "test-key-name", - CrdPath: validCrdPath, - }, - }, } func TestConfigValidate(t *testing.T) { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 61bb5369..443e7ab5 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -7,12 +7,10 @@ import ( "context" "fmt" "net/http" - "os" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/Azure/aks-app-routing-operator/pkg/controller/nginxingress" "github.com/Azure/aks-app-routing-operator/pkg/controller/service" - "github.com/Azure/aks-app-routing-operator/pkg/webhook" "github.com/go-logr/logr" cfgv1alpha2 "github.com/openservicemesh/osm/pkg/apis/config/v1alpha2" policyv1alpha1 "github.com/openservicemesh/osm/pkg/apis/policy/v1alpha1" @@ -32,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log/zap" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - ctrlwebhook "sigs.k8s.io/controller-runtime/pkg/webhook" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "github.com/Azure/aks-app-routing-operator/pkg/config" @@ -95,20 +92,13 @@ func NewManagerForRestConfig(conf *config.Config, rc *rest.Config) (ctrl.Manager LeaderElection: true, LeaderElectionNamespace: "kube-system", LeaderElectionID: "aks-app-routing-operator-leader", - WebhookServer: ctrlwebhook.NewServer(ctrlwebhook.Options{ - Port: conf.WebhookPort, - CertDir: conf.CertDir, - CertName: conf.CertName, - KeyName: conf.KeyName, - }), }) if err != nil { return nil, fmt.Errorf("creating manager: %w", err) } setupLog := m.GetLogger().WithName("setup") - webhooksReady := make(chan struct{}) - if err := setupProbes(conf, m, webhooksReady, setupLog); err != nil { + if err := setupProbes(conf, m, setupLog); err != nil { return nil, fmt.Errorf("setting up probes: %w", err) } @@ -122,23 +112,16 @@ func NewManagerForRestConfig(conf *config.Config, rc *rest.Config) (ctrl.Manager return nil, fmt.Errorf("loading CRDs: %w", err) } - go func() { - if err := setupControllers(m, conf, webhooksReady, setupLog, cl); err != nil { - setupLog.Error(err, "unable to setup controllers") - os.Exit(1) - } - }() - - if err := setupWebhooks(conf, m, setupLog, cl, webhooksReady); err != nil { - setupLog.Error(err, "unable to setup webhooks") - return nil, fmt.Errorf("setting up webhooks: %w", err) - } - if err := setupIndexers(m, setupLog); err != nil { setupLog.Error(err, "unable to setup indexers") return nil, fmt.Errorf("setting up indexers: %w", err) } + if err := setupControllers(m, conf, setupLog, cl); err != nil { + setupLog.Error(err, "unable to setup controllers") + return nil, fmt.Errorf("setting up controllers: %w", err) + } + return m, nil } @@ -155,44 +138,7 @@ func setupIndexers(mgr ctrl.Manager, lgr logr.Logger) error { return nil } -func setupWebhooks(conf *config.Config, mgr ctrl.Manager, lgr logr.Logger, cl client.Client, webhookSetupDone chan<- struct{}) error { - if !conf.EnableWebhook { - lgr.Info("webhooks are disabled, skipping setup") - close(webhookSetupDone) - return nil - } - - lgr.Info("setting up webhooks") - webhookCfg, err := webhook.New(conf) - if err != nil { - lgr.Error(err, "unable to create webhook config") - return fmt.Errorf("creating webhook config: %w", err) - } - - lgr.Info("ensuring webhook certificates") - if err := webhookCfg.EnsureCertificates(lgr); err != nil { - lgr.Error(err, "unable to ensure certificates") - return fmt.Errorf("ensuring certificates: %w", err) - } - - lgr.Info("ensuring webhook configurations") - if err := webhookCfg.EnsureWebhookConfigurations(context.Background(), cl, conf); err != nil { - lgr.Error(err, "unable to ensure webhook configurations") - return fmt.Errorf("ensuring webhook configurations: %w", err) - } - - lgr.Info("adding webhooks to manager") - if err := webhookCfg.AddWebhooks(mgr); err != nil { - lgr.Error(err, "unable to add webhooks to manager") - return fmt.Errorf("adding webhooks to manager: %w", err) - } - - lgr.Info("finished setting up webhooks") - close(webhookSetupDone) - return nil -} - -func setupControllers(mgr ctrl.Manager, conf *config.Config, webhooksReady <-chan struct{}, lgr logr.Logger, cl client.Client) error { +func setupControllers(mgr ctrl.Manager, conf *config.Config, lgr logr.Logger, cl client.Client) error { lgr.Info("setting up controllers") lgr.Info("determining default IngressClass controller class") @@ -216,10 +162,6 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config, webhooksReady <-cha return fmt.Errorf("setting up ingress cert config reconciler: %w", err) } - lgr.Info("waiting for webhooks to be ready") - <-webhooksReady // some controllers need to wait for webhooks to be ready because they interact directly with our CRDs - lgr.Info("finished waiting for webhooks to be ready") - defaultNic := nginxingress.GetDefaultNginxIngressController() if err := service.NewNginxIngressReconciler(mgr, nginxingress.ToNginxIngressConfig(&defaultNic, defaultCc)); err != nil { return fmt.Errorf("setting up nginx ingress reconciler: %w", err) @@ -263,24 +205,10 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config, webhooksReady <-cha return nil } -func setupProbes(conf *config.Config, mgr ctrl.Manager, webhooksReady <-chan struct{}, log logr.Logger) error { +func setupProbes(conf *config.Config, mgr ctrl.Manager, log logr.Logger) error { log.Info("adding probes to manager") - // checks if the webhooks are ready so that the service can only serve webhook - // traffic to ready webhooks - check := func(req *http.Request) error { - select { - case <-webhooksReady: - return mgr.GetWebhookServer().StartedChecker()(req) - default: - return fmt.Errorf("webhooks aren't ready yet") - } - } - - if !conf.EnableWebhook { - log.Info("webhooks are disabled, skipping webhook readiness check") - check = func(_ *http.Request) error { return nil } - } + check := func(req *http.Request) error { return nil } if err := mgr.AddReadyzCheck("readyz", check); err != nil { return fmt.Errorf("adding readyz check: %w", err) diff --git a/pkg/webhook/cert.go b/pkg/webhook/cert.go deleted file mode 100644 index c2c92ac1..00000000 --- a/pkg/webhook/cert.go +++ /dev/null @@ -1,207 +0,0 @@ -package webhook - -import ( - "bytes" - "crypto/rand" - "crypto/rsa" - "crypto/x509" - "crypto/x509/pkix" - "encoding/pem" - "errors" - "fmt" - "math/big" - "net/url" - "os" - "path/filepath" - "time" - - "github.com/go-logr/logr" -) - -// this file is heavily inspired by Fleet's webhook cert gen https://github.com/Azure/fleet/blob/main/pkg/webhook/webhook.go -type cert struct { - caPem []byte - certPem []byte - keyPem []byte -} - -func (c *config) newCert() (*cert, error) { - // CA config - ca := &x509.Certificate{ - SerialNumber: big.NewInt(2023), - Subject: pkix.Name{ - CommonName: "approuting.kubernetes.azure.com", - }, - NotBefore: time.Now(), - NotAfter: time.Now().AddDate(10, 0, 0), // 10 years - IsCA: true, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - BasicConstraintsValid: true, - } - - // CA private key - caPrvKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - return nil, fmt.Errorf("generating ca private key: %w", err) - } - - // self-signed CA certificate - caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrvKey.PublicKey, caPrvKey) - if err != nil { - return nil, fmt.Errorf("creating self-signed ca certificate: %w", err) - } - - // PEM encode CA cert - caPEM := new(bytes.Buffer) - if err := pem.Encode(caPEM, &pem.Block{ - Type: "CERTIFICATE", - Bytes: caBytes, - }); err != nil { - return nil, fmt.Errorf("pem encoding ca certificate: %w", err) - } - - var dnsNames []string - if c.serviceName != "" { - dnsNames = append(dnsNames, fmt.Sprintf("%s.%s.svc", c.serviceName, c.namespace)) - } - if c.serviceUrl != "" { - serviceUrl, err := url.Parse(c.serviceUrl) - if err != nil { - return nil, fmt.Errorf("parsing service url: %w", err) - } - dnsNames = append(dnsNames, serviceUrl.Hostname()) - } - - // server cert config - certCfg := &x509.Certificate{ - DNSNames: dnsNames, - SerialNumber: big.NewInt(2023), - Subject: pkix.Name{ - CommonName: fmt.Sprintf("%s.cert.server", c.serviceName), - }, - NotBefore: time.Now(), - NotAfter: time.Now().AddDate(10, 0, 0), - SubjectKeyId: []byte{1, 2, 3, 4, 5}, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature, - } - - // server private key - certPrvKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - return nil, fmt.Errorf("generating server cert private key: %w", err) - } - - // sign the server cert - certBytes, err := x509.CreateCertificate(rand.Reader, certCfg, ca, &certPrvKey.PublicKey, caPrvKey) - if err != nil { - return nil, fmt.Errorf("signing the server cert: %w", err) - } - - // PEM encode the server cert and key - certPEM := new(bytes.Buffer) - if err := pem.Encode(certPEM, &pem.Block{ - Type: "CERTIFICATE", - Bytes: certBytes, - }); err != nil { - return nil, fmt.Errorf("pem encoding the server cert: %w", err) - } - - certPrvKeyPEM := new(bytes.Buffer) - if err := pem.Encode(certPrvKeyPEM, &pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(certPrvKey), - }); err != nil { - return nil, fmt.Errorf("pem encoding the cert private key: %w", err) - } - - return &cert{ - caPem: caPEM.Bytes(), - certPem: certPEM.Bytes(), - keyPem: certPrvKeyPEM.Bytes(), - }, nil -} - -func (c *config) EnsureCertificates(lgr logr.Logger) error { - lgr.Info("ensuring certificates") - - lgr.Info("checking if certs exists") - needToGen := false - certsFiles := []string{c.certName, c.keyName, c.caName} - for _, certFile := range certsFiles { - path := filepath.Join(c.certDir, certFile) - if _, err := os.ReadFile(path); err != nil { - if errors.Is(err, os.ErrNotExist) { - needToGen = true - break - } - - return fmt.Errorf("reading cert file %s: %w", path, err) - } - } - - if !needToGen { - lgr.Info("certs already exist") - return nil - } - - lgr.Info("certs do not exist, creating new certs") - newCert, err := c.newCert() - if err != nil { - return fmt.Errorf("creating new cert: %w", err) - } - - // we need to fully clean any certs so we can ensure that our certs are the ones being used - lgr.Info("fully cleaning any old certs") - if err := os.RemoveAll(c.certDir); err != nil { - return fmt.Errorf("removing old certs: %w", err) - } - - lgr.Info("creating new certs dir") - if err := os.MkdirAll(c.certDir, 0755); err != nil { - return fmt.Errorf("creating new certs dir: %w", err) - } - - // we use O_EXCL to ensure the file doesn't exist when we open it. - // this ensures that another replica doesn't attempt to write the same file at the same time - // which would cause unintended sideeffects. Only one instance of the operator should be - // generating certs at a time which this guarantees. - openFileFlags := os.O_CREATE | os.O_EXCL | os.O_RDWR - - lgr.Info("writing cert") - certPath := filepath.Join(c.certDir, c.certName) - certFile, err := os.OpenFile(certPath, openFileFlags, 0600) - if err != nil { - return fmt.Errorf("opening cert file: %w", err) - } - defer certFile.Close() - if _, err := certFile.Write(newCert.certPem); err != nil { - return fmt.Errorf("writing cert: %w", err) - } - - lgr.Info("writing key") - keyPath := filepath.Join(c.certDir, c.keyName) - keyFile, err := os.OpenFile(keyPath, openFileFlags, 0600) - if err != nil { - return fmt.Errorf("opening key file: %w", err) - } - defer keyFile.Close() - if _, err := keyFile.Write(newCert.keyPem); err != nil { - return fmt.Errorf("writing key: %w", err) - } - - lgr.Info("writing ca") - caPath := filepath.Join(c.certDir, c.caName) - caFile, err := os.OpenFile(caPath, openFileFlags, 0600) - if err != nil { - return fmt.Errorf("opening ca file: %w", err) - } - defer caFile.Close() - if _, err := caFile.Write(newCert.caPem); err != nil { - return fmt.Errorf("writing ca: %w", err) - } - - lgr.Info("certs created") - return nil -} diff --git a/pkg/webhook/cert_test.go b/pkg/webhook/cert_test.go deleted file mode 100644 index cfd638e4..00000000 --- a/pkg/webhook/cert_test.go +++ /dev/null @@ -1,135 +0,0 @@ -package webhook - -import ( - "crypto/x509" - "encoding/pem" - "fmt" - "os" - "testing" - - "github.com/go-logr/logr" - "github.com/stretchr/testify/require" -) - -func TestNewCert(t *testing.T) { - t.Run("with service name", func(t *testing.T) { - c := &config{ - serviceName: "test-service", - namespace: "test-namespace", - } - cert, err := c.newCert() - require.NoError(t, err, "expected no error creating new cert") - require.NotNil(t, cert, "expected cert to not be nil") - require.NotNil(t, cert.caPem, "expected caPem to not be nil") - require.NotNil(t, cert.certPem, "expected certPem to not be nil") - require.NotNil(t, cert.keyPem, "expected keyPem to not be nil") - - // Verify that the CA cert is valid - caCertBlock, _ := pem.Decode(cert.caPem) - require.NotNil(t, caCertBlock, "expected caCertBlock to not be nil") - caCert, err := x509.ParseCertificate(caCertBlock.Bytes) - require.NoError(t, err, "expected no error parsing ca cert") - require.Equal(t, "approuting.kubernetes.azure.com", caCert.Subject.CommonName, "expected common name to match") - require.Equal(t, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, caCert.ExtKeyUsage, "expected ext key usage to match") - require.Equal(t, x509.KeyUsageDigitalSignature|x509.KeyUsageCertSign, caCert.KeyUsage, "expected key usage to match") - require.True(t, caCert.IsCA, "expected IsCA to be true") - - // Verify that the server cert is valid - serverCertBlock, _ := pem.Decode(cert.certPem) - require.NotNil(t, serverCertBlock, "expected serverCertBlock to not be nil") - serverCert, err := x509.ParseCertificate(serverCertBlock.Bytes) - require.NoError(t, err, "expected no error parsing server cert") - require.Equal(t, []string{fmt.Sprintf("%s.%s.svc", c.serviceName, c.namespace)}, serverCert.DNSNames, "expected DNS names to match") - require.Equal(t, fmt.Sprintf("%s.cert.server", c.serviceName), serverCert.Subject.CommonName, "expected common name to match") - require.Equal(t, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, serverCert.ExtKeyUsage, "expected ext key usage to match") - require.Equal(t, x509.KeyUsageDigitalSignature, serverCert.KeyUsage, "expected key usage to match") - - // Verify that the server cert is signed by the CA - err = serverCert.CheckSignatureFrom(caCert) - require.NoError(t, err, "expected no error checking signature") - - // Verify that the server key is valid - serverKeyBlock, _ := pem.Decode(cert.keyPem) - require.NotNil(t, serverKeyBlock, "expected serverKeyBlock to not be nil") - serverKey, err := x509.ParsePKCS1PrivateKey(serverKeyBlock.Bytes) - require.NoError(t, err, "expected no error parsing server key") - require.Equal(t, serverCert.PublicKey, serverKey.Public(), "expected public key to match") - }) - - t.Run("with service url ", func(t *testing.T) { - c := &config{ - serviceUrl: "https://app-routing-operator-webhook.123456.svc.cluster.local:9443", - namespace: "test-namespace", - } - cert, err := c.newCert() - require.NoError(t, err, "expected no error creating new cert") - require.NotNil(t, cert, "expected cert to not be nil") - require.NotNil(t, cert.caPem, "expected caPem to not be nil") - require.NotNil(t, cert.certPem, "expected certPem to not be nil") - require.NotNil(t, cert.keyPem, "expected keyPem to not be nil") - - // Verify that the CA cert is valid - caCertBlock, _ := pem.Decode(cert.caPem) - require.NotNil(t, caCertBlock, "expected caCertBlock to not be nil") - caCert, err := x509.ParseCertificate(caCertBlock.Bytes) - require.NoError(t, err, "expected no error parsing ca cert") - require.Equal(t, "approuting.kubernetes.azure.com", caCert.Subject.CommonName, "expected common name to match") - require.Equal(t, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, caCert.ExtKeyUsage, "expected ext key usage to match") - require.Equal(t, x509.KeyUsageDigitalSignature|x509.KeyUsageCertSign, caCert.KeyUsage, "expected key usage to match") - require.True(t, caCert.IsCA, "expected IsCA to be true") - - // Verify that the server cert is valid - serverCertBlock, _ := pem.Decode(cert.certPem) - require.NotNil(t, serverCertBlock, "expected serverCertBlock to not be nil") - serverCert, err := x509.ParseCertificate(serverCertBlock.Bytes) - require.NoError(t, err, "expected no error parsing server cert") - require.Equal(t, []string{"app-routing-operator-webhook.123456.svc.cluster.local"}, serverCert.DNSNames, "expected DNS names to match") - require.Equal(t, fmt.Sprintf("%s.cert.server", c.serviceName), serverCert.Subject.CommonName, "expected common name to match") - require.Equal(t, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, serverCert.ExtKeyUsage, "expected ext key usage to match") - require.Equal(t, x509.KeyUsageDigitalSignature, serverCert.KeyUsage, "expected key usage to match") - - // Verify that the server cert is signed by the CA - err = serverCert.CheckSignatureFrom(caCert) - require.NoError(t, err, "expected no error checking signature") - - // Verify that the server key is valid - serverKeyBlock, _ := pem.Decode(cert.keyPem) - require.NotNil(t, serverKeyBlock, "expected serverKeyBlock to not be nil") - serverKey, err := x509.ParsePKCS1PrivateKey(serverKeyBlock.Bytes) - require.NoError(t, err, "expected no error parsing server key") - require.Equal(t, serverCert.PublicKey, serverKey.Public(), "expected public key to match") - }) -} -func TestEnsureCertificates(t *testing.T) { - - lgr := logr.Discard() - - t.Run("cert already exists", func(t *testing.T) { - c := &config{ - certDir: "testcerts", - certName: "tls.crt", - keyName: "tls.key", - caName: "ca.crt", - } - - err := c.EnsureCertificates(lgr) - require.NoError(t, err, "expected no error") - }) - - t.Run("create new certs", func(t *testing.T) { - c := &config{ - certDir: "test-dir", - certName: "tls.crt", - keyName: "tls.key", - caName: "ca.crt", - } - - err := c.EnsureCertificates(lgr) - require.NoError(t, err, "expected no error") - require.FileExists(t, "test-dir/tls.crt", "expected tls.crt to exist") - require.FileExists(t, "test-dir/tls.key", "expected tls.key to exist") - require.FileExists(t, "test-dir/ca.crt", "expected ca.crt to exist") - - os.RemoveAll("test-dir") - }) -} diff --git a/pkg/webhook/nginxingress.go b/pkg/webhook/nginxingress.go deleted file mode 100644 index 923be02c..00000000 --- a/pkg/webhook/nginxingress.go +++ /dev/null @@ -1,263 +0,0 @@ -package webhook - -import ( - "context" - "encoding/json" - "fmt" - "net/http" - - approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" - "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" - "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" - "github.com/Azure/aks-app-routing-operator/pkg/controller/nginxingress" - "github.com/Azure/aks-app-routing-operator/pkg/manifests" - "github.com/Azure/aks-app-routing-operator/pkg/util" - "github.com/go-logr/logr" - admissionv1 "k8s.io/api/admission/v1" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - authv1 "k8s.io/api/authorization/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -const ( - validationPath = "/validate-nginx-ingress-controller" - mutationPath = "/mutate-nginx-ingress-controller" -) - -var ( - nginxResourceValidationName = controllername.New("nginx", "ingress", "resource", "validator") - nginxResourceMutationName = controllername.New("nginx", "ingress", "resource", "mutator") -) - -func init() { - Validating = append(Validating, Webhook[admissionregistrationv1.ValidatingWebhook]{ - AddToManager: func(mgr manager.Manager) error { - metrics.InitControllerMetrics(nginxResourceValidationName) - mgr.GetWebhookServer().Register(validationPath, &webhook.Admission{ - Handler: &nginxIngressResourceValidator{ - client: mgr.GetClient(), - decoder: admission.NewDecoder(mgr.GetScheme()), - authenticate: SarAuthenticateNginxIngressController, - }, - }) - - return nil - }, - Definition: func(c *config) (admissionregistrationv1.ValidatingWebhook, error) { - clientCfg, err := c.GetClientConfig(validationPath) - if err != nil { - return admissionregistrationv1.ValidatingWebhook{}, fmt.Errorf("getting client config: %w", err) - } - - return admissionregistrationv1.ValidatingWebhook{ - Name: "validating.nginxingresscontroller.approuting.kubernetes.azure.com", - AdmissionReviewVersions: []string{admissionregistrationv1.SchemeGroupVersion.Version}, - ClientConfig: clientCfg, - Rules: []admissionregistrationv1.RuleWithOperations{ - { - Operations: []admissionregistrationv1.OperationType{admissionregistrationv1.Create, admissionregistrationv1.Update, admissionregistrationv1.Delete}, - Rule: admissionregistrationv1.Rule{ - APIGroups: []string{approutingv1alpha1.GroupVersion.Group}, - APIVersions: []string{approutingv1alpha1.GroupVersion.Version}, - Resources: []string{"nginxingresscontrollers"}, - }, - }, - }, - FailurePolicy: util.ToPtr(admissionregistrationv1.Fail), // need this because we have to check permissions, better to fail than let a request through - SideEffects: util.ToPtr(admissionregistrationv1.SideEffectClassNone), - }, nil - }, - }) - - Mutating = append(Mutating, Webhook[admissionregistrationv1.MutatingWebhook]{ - AddToManager: func(mgr manager.Manager) error { - metrics.InitControllerMetrics(nginxResourceMutationName) - mgr.GetWebhookServer().Register(mutationPath, &webhook.Admission{ - Handler: &nginxIngressResourceMutator{ - decoder: admission.NewDecoder(mgr.GetScheme()), - }, - }) - - return nil - }, - Definition: func(c *config) (admissionregistrationv1.MutatingWebhook, error) { - clientCfg, err := c.GetClientConfig(mutationPath) - if err != nil { - return admissionregistrationv1.MutatingWebhook{}, fmt.Errorf("getting client config: %w", err) - } - - return admissionregistrationv1.MutatingWebhook{ - Name: "mutating.nginxingresscontroller.approuting.kubernetes.azure.com", - AdmissionReviewVersions: []string{admissionregistrationv1.SchemeGroupVersion.Version}, - ClientConfig: clientCfg, - Rules: []admissionregistrationv1.RuleWithOperations{ - { - Operations: []admissionregistrationv1.OperationType{admissionregistrationv1.OperationAll}, - Rule: admissionregistrationv1.Rule{ - APIGroups: []string{approutingv1alpha1.GroupVersion.Group}, - APIVersions: []string{approutingv1alpha1.GroupVersion.Version}, - Resources: []string{"nginxingresscontrollers"}, - }, - }, - }, - FailurePolicy: util.ToPtr(admissionregistrationv1.Ignore), - SideEffects: util.ToPtr(admissionregistrationv1.SideEffectClassNone), - }, nil - }, - }) -} - -type authenticateFn func(ctx context.Context, lgr logr.Logger, cl client.Client, req admission.Request) (string, error) - -type nginxIngressResourceValidator struct { - client client.Client - decoder *admission.Decoder - // authenticate is a function that checks if the request user is authorized to perform the request. - // The returned string indicates whether the user is allowed, empty string indicates allowed and non-empty will - // be equal to the reason why they're not allowed. Error will be returned if something goes wrong while verifying the user can request - authenticate authenticateFn -} - -// SarAuthenticateNginxIngressController checks if the user is allowed to perform a request against an NginxIngressController resource. If the user is allowed it returns an empty string, otherwise it returns the reason why they're not allowed. -func SarAuthenticateNginxIngressController(ctx context.Context, lgr logr.Logger, cl client.Client, req admission.Request) (string, error) { - // ensure user has permissions required - lgr.Info("checking permissions") - extra := make(map[string]authv1.ExtraValue) - for k, v := range req.UserInfo.Extra { - extra[k] = authv1.ExtraValue(v) - } - for _, resource := range manifests.NginxResourceTypes { - lgr := lgr.WithValues("sarResource", resource.Name, "sarGroup", resource.Group, "sarVersion", resource.Version) - lgr.Info("checking permissions for resource") - sar := authv1.SubjectAccessReview{ - Spec: authv1.SubjectAccessReviewSpec{ - ResourceAttributes: &authv1.ResourceAttributes{ - // TODO: add namespace check, this is a bit harder because we need to check if resource is namespaced - Namespace: "", - Verb: "*", - Group: resource.Group, - Version: resource.Version, - Resource: resource.Name, - }, - User: req.UserInfo.Username, - Groups: req.UserInfo.Groups, - Extra: extra, - UID: req.UserInfo.UID, - }, - } - lgr.Info("performing SubjectAccessReview") - if err := cl.Create(ctx, &sar); err != nil { - lgr.Error(err, "creating SubjectAccessReview") - return "", fmt.Errorf("creating SubjectAccessReview: %w", err) - } - if sar.Status.Denied || (!sar.Status.Allowed) { - lgr.Info("denied due to permissions", "reason", sar.Status.Reason) - return fmt.Sprintf("user '%s' does not have permissions to create/update NginxIngressController. Verb '%s' needed for resource '%s' in group '%s' version '%s'.", - req.UserInfo.Username, sar.Spec.ResourceAttributes.Verb, sar.Spec.ResourceAttributes.Resource, sar.Spec.ResourceAttributes.Group, sar.Spec.ResourceAttributes.Version, - ), nil - } - - } - lgr.Info("permissions check passed") - return "", nil -} - -func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admission.Request) (resp admission.Response) { - var err error - defer func() { - metrics.HandleWebhookHandlerMetrics(nginxResourceValidationName, resp, err) - }() - - lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", req.Name, "namespace", req.Namespace, "operation", req.Operation).WithName(nginxResourceValidationName.LoggerName()) - lgr.Info("validating NginxIngressController request") - - // ensure user has permissions required - var cantPerform string - cantPerform, err = n.authenticate(ctx, lgr, n.client, req) - if err != nil { - lgr.Error(err, "checking permissions") - return admission.Errored(http.StatusInternalServerError, err) - } - if cantPerform != "" { - lgr.Info("denied due to permissions", "reason", cantPerform) - return admission.Denied(cantPerform) - } - - if req.Operation == admissionv1.Delete { - return admission.Allowed("") - } - - lgr.Info("decoding NginxIngressController resource") - var nginxIngressController approutingv1alpha1.NginxIngressController - if err = n.decoder.Decode(req, &nginxIngressController); err != nil { - lgr.Error(err, "decoding nginx ingress controller") - return admission.Errored(http.StatusBadRequest, fmt.Errorf("decoding NginxIngressController: %w", err)) - } - - // basic spec validation (everything we can check without making API calls) - if invalidReason := nginxIngressController.Valid(); invalidReason != "" { - return admission.Denied(invalidReason) - } - - if req.Operation == admissionv1.Create { - if nginxingress.IsDefaultNic(&nginxIngressController) { - // need to allow for ic to exist already for the default migration case (migrating from non-crd versions of app routing to crd versions) - return admission.Allowed("") - } - - lgr.Info("checking if NginxIngressController has unreconcilable collision") - collides, reason, err := nginxIngressController.Collides(ctx, n.client) - if err != nil { - lgr.Error(err, "checking if it collides") - return admission.Errored(http.StatusInternalServerError, fmt.Errorf("checking if it collides: %w", err)) - } - - if collides { - lgr.Info("denied due to collision", "reason", reason) - return admission.Denied(reason) - } - } - - lgr.Info("admission allowed") - return admission.Allowed("") -} - -type nginxIngressResourceMutator struct { - decoder *admission.Decoder -} - -func (n nginxIngressResourceMutator) Handle(ctx context.Context, request admission.Request) (resp admission.Response) { - var err error - defer func() { - metrics.HandleWebhookHandlerMetrics(nginxResourceMutationName, resp, err) - }() - - lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", request.Name, "namespace", request.Namespace, "operation", request.Operation).WithName(nginxResourceMutationName.LoggerName()) - - if request.Operation == admissionv1.Delete { - lgr.Info("delete operation, skipping mutation") - return admission.Allowed("") - } - - lgr.Info("decoding NginxIngressController resource") - var nginxIngressController approutingv1alpha1.NginxIngressController - if err = n.decoder.Decode(request, &nginxIngressController); err != nil { - lgr.Error(err, "decoding nginx ingress controller") - return admission.Errored(http.StatusBadRequest, fmt.Errorf("decoding NginxIngressController: %w", err)) - } - - lgr.Info("defaulting NginxIngressController resource") - nginxIngressController.Default() - - var marshalled []byte - marshalled, err = json.Marshal(nginxIngressController) - if err != nil { - lgr.Error(err, "encoding nginx ingress controller") - return admission.Errored(http.StatusInternalServerError, fmt.Errorf("marshalling NginxIngressController: %w", err)) - } - - return admission.PatchResponseFromRaw(request.Object.Raw, marshalled) -} diff --git a/pkg/webhook/nginxingress_test.go b/pkg/webhook/nginxingress_test.go deleted file mode 100644 index 81b9a82e..00000000 --- a/pkg/webhook/nginxingress_test.go +++ /dev/null @@ -1,548 +0,0 @@ -package webhook - -import ( - "bytes" - "context" - "encoding/json" - "errors" - "fmt" - "net/http" - "reflect" - "testing" - - approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" - "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" - "github.com/Azure/aks-app-routing-operator/pkg/controller/nginxingress" - "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" - "github.com/go-logr/logr" - "github.com/stretchr/testify/require" - "gomodules.xyz/jsonpatch/v2" - admissionv1 "k8s.io/api/admission/v1" - authenticationv1 "k8s.io/api/authentication/v1" - authv1 "k8s.io/api/authorization/v1" - netv1 "k8s.io/api/networking/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -var scheme = runtime.NewScheme() - -func init() { - approutingv1alpha1.AddToScheme(scheme) - clientgoscheme.AddToScheme(scheme) -} - -var validNginxIngressController = &approutingv1alpha1.NginxIngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid", - }, - Spec: approutingv1alpha1.NginxIngressControllerSpec{ - IngressClassName: "ingressclassname", - ControllerNamePrefix: "prefix", - }, -} - -var invalidNginxIngressController = &approutingv1alpha1.NginxIngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "invalid", - }, - Spec: approutingv1alpha1.NginxIngressControllerSpec{ - IngressClassName: "ingressclassname", - }, -} - -func toRaw(n *approutingv1alpha1.NginxIngressController) []byte { - raw, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(n) - buf := bytes.Buffer{} - encoder := json.NewEncoder(&buf) - encoder.Encode(raw) - return buf.Bytes() -} - -var validUser = func(_ context.Context, _ logr.Logger, _ client.Client, _ admission.Request) (string, error) { - return "", nil -} - -var invalidUser = func(_ context.Context, _ logr.Logger, _ client.Client, _ admission.Request) (string, error) { - return "", errors.New("invalid user") -} - -func TestNginxIngressResourceValidator(t *testing.T) { - scheme := runtime.NewScheme() - require.NoError(t, approutingv1alpha1.AddToScheme(scheme)) - require.NoError(t, clientgoscheme.AddToScheme(scheme)) - cl := fake.NewClientBuilder().WithScheme(scheme).Build() - - existingIc := &netv1.IngressClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: "existing", - }, - } - require.NoError(t, cl.Create(context.Background(), existingIc)) - existingNic := &approutingv1alpha1.NginxIngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "existing", - }, - Spec: approutingv1alpha1.NginxIngressControllerSpec{ - IngressClassName: "existing2", - ControllerNamePrefix: "prefix", - }, - } - require.NoError(t, cl.Create(context.Background(), existingNic)) - - defaultIc := &netv1.IngressClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: nginxingress.DefaultIcName, - }, - } - require.NoError(t, cl.Create(context.Background(), defaultIc)) - - cases := []struct { - name string - req admission.Request - authenticator authenticateFn - expected admission.Response - }{ - { - name: "valid nginx ingress controller, valid user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(validNginxIngressController), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "invalid user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - }, - }, - authenticator: invalidUser, - expected: admission.Denied("invalid user"), - }, - { - name: "invalid nginx ingress controller, valid user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(invalidNginxIngressController), - }, - }, - }, - authenticator: validUser, - expected: admission.Denied("spec.controllerNamePrefix must be specified"), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingIc.Name - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Denied("spec.ingressClassName \"existing\" is invalid because IngressClass \"existing\" already exists"), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class, update", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingIc.Name - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class, delete", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingIc.Name - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class on other nginx ingress controller", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingNic.Spec.IngressClassName - copy.Name = "other" - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Denied("spec.ingressClassName \"existing2\" is invalid because NginxIngressController \"existing\" already uses IngressClass \"existing2\""), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class on other nginx ingress controller, updating", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingIc.Name - copy.Name = "other" - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "valid nginx ingress controller, valid user, existing ingress class on other nginx ingress controller, deleting", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = existingIc.Name - copy.Name = "other" - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "valid nginx ingress controller, valid user, default nic", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = nginxingress.DefaultIcName - copy.Name = nginxingress.DefaultNicName - return copy - }()), - }, - }, - }, - authenticator: validUser, - expected: admission.Allowed(""), - }, - { - name: "valid nginx ingress controller, invalid user, default nic", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { - copy := validNginxIngressController.DeepCopy() - copy.Spec.IngressClassName = nginxingress.DefaultIcName - copy.Name = nginxingress.DefaultNicName - return copy - }()), - }, - }, - }, - authenticator: invalidUser, - expected: admission.Denied("invalid user"), - }, - } - - metrics.InitControllerMetrics(nginxResourceValidationName) - beforeErrCount := testutils.GetErrMetricCount(t, nginxResourceValidationName) - beforeSuccessCount := testutils.GetReconcileMetricCount(t, nginxResourceValidationName, metrics.LabelSuccess) - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - validator := nginxIngressResourceValidator{ - client: cl, - decoder: admission.NewDecoder(cl.Scheme()), - authenticate: tc.authenticator, - } - actual := validator.Handle(context.Background(), tc.req) - - if actual.Allowed != tc.expected.Allowed { - t.Errorf("expected allowed %v, got %v", tc.expected.Allowed, actual.Allowed) - } - - if tc.expected.Result != nil && tc.expected.Result.Message != actual.Result.Message { - t.Errorf("expected message %v, got %v", tc.expected.Result.Message, actual.Result.Message) - } - }) - } - - require.Greater(t, testutils.GetErrMetricCount(t, nginxResourceValidationName), beforeErrCount) - require.Greater(t, testutils.GetReconcileMetricCount(t, nginxResourceValidationName, metrics.LabelSuccess), beforeSuccessCount) -} - -func TestNginxIngressResourceMutator(t *testing.T) { - cases := []struct { - name string - req admission.Request - expected admission.Response - }{ - { - name: "no mutation, all fields set", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(&approutingv1alpha1.NginxIngressController{ - Spec: approutingv1alpha1.NginxIngressControllerSpec{ - IngressClassName: "ingressClassName", - ControllerNamePrefix: "prefix", - }, - }), - }, - }, - }, - expected: admission.Response{ - AdmissionResponse: admissionv1.AdmissionResponse{ - Allowed: true, - }, - }, - }, - { - name: "mutation", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: toRaw(&approutingv1alpha1.NginxIngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - }, - Spec: approutingv1alpha1.NginxIngressControllerSpec{ - IngressClassName: "ingressClassName", - }, - }), - }, - }, - }, - expected: admission.Response{ - AdmissionResponse: admissionv1.AdmissionResponse{ - Allowed: true, - }, - Patches: []jsonpatch.JsonPatchOperation{ - { - Operation: "replace", - Path: "/spec/controllerNamePrefix", - Value: "nginx", - }, - }, - }, - }, - { - name: "mutation fails to decode bad input", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte{0, 0, 1, 2, 3}, - }, - }, - }, - expected: admission.Response{ - AdmissionResponse: admissionv1.AdmissionResponse{ - Allowed: false, - Result: &metav1.Status{ - Code: http.StatusBadRequest, - Message: fmt.Errorf("decoding NginxIngressController: %w", errors.New("failed decode")).Error(), - }, - }, - }, - }, - } - - metrics.InitControllerMetrics(nginxResourceMutationName) - beforeErrCount := testutils.GetErrMetricCount(t, nginxResourceMutationName) - beforeSuccessCount := testutils.GetReconcileMetricCount(t, nginxResourceMutationName, metrics.LabelSuccess) - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - mutator := nginxIngressResourceMutator{ - decoder: admission.NewDecoder(scheme), - } - actual := mutator.Handle(context.Background(), tc.req) - - if actual.Allowed != tc.expected.Allowed { - t.Errorf("expected allowed %v, got %v", tc.expected.Allowed, actual.Allowed) - } - - if len(actual.Patches) != len(tc.expected.Patches) { - t.Errorf("expected %d patches, got %d", len(tc.expected.Patches), len(actual.Patches)) - } - - for i, patch := range actual.Patches { - if !reflect.DeepEqual(patch, tc.expected.Patches[i]) { - t.Errorf("expected patch %v, got %v", tc.expected.Patches[i], patch) - } - } - }) - } - - require.Greater(t, testutils.GetErrMetricCount(t, nginxResourceMutationName), beforeErrCount) - require.Greater(t, testutils.GetReconcileMetricCount(t, nginxResourceMutationName, metrics.LabelSuccess), beforeSuccessCount) -} - -func TestSarAuthenticateNginxIngressController(t *testing.T) { - allowedUserUid := "allowed-user-uid" - forbiddenUserUid := "forbidden-user-uid" - deniedAndAllowedUserUid := "denied-and-allowed-user-uid" - notAllowedAndNotDeniedUserUid := "not-allowed-and-not-denied-user-uid" - errUserUid := "err-user-uid" - failedError := errors.New("failed to create sar") - cl := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ - Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error { - sar, ok := obj.(*authv1.SubjectAccessReview) - if !ok { - return nil - } - - switch sar.Spec.UID { - case allowedUserUid: - sar.Status.Allowed = true - sar.Status.Denied = false - case forbiddenUserUid: - sar.Status.Allowed = false - sar.Status.Reason = "forbidden user" - case deniedAndAllowedUserUid: - sar.Status.Allowed = true - sar.Status.Denied = true - sar.Status.Reason = "denied and allowed user" - case notAllowedAndNotDeniedUserUid: - sar.Status.Allowed = false - sar.Status.Denied = false - sar.Status.Reason = "not allowed and not denied user" - case errUserUid: - return failedError - } - - return nil - }, - }).Build() - - cases := []struct { - name string - req admission.Request - expectedDenyReason string - expectedError error - }{ - { - name: "allowed user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - UID: allowedUserUid, - }, - }, - }, - expectedDenyReason: "", - expectedError: nil, - }, - { - name: "forbidden user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - UID: forbiddenUserUid, - Username: "forbidden-user", - }, - }, - }, - expectedDenyReason: "user 'forbidden-user' does not have permissions to create/update NginxIngressController. Verb '*' needed for resource 'IngressClass' in group 'networking.k8s.io' version 'v1'.", - expectedError: nil, - }, - { - name: "denied and allowed user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - UID: deniedAndAllowedUserUid, - Username: "denied-and-allowed-user", - }, - }, - }, - expectedDenyReason: "user 'denied-and-allowed-user' does not have permissions to create/update NginxIngressController. Verb '*' needed for resource 'IngressClass' in group 'networking.k8s.io' version 'v1'.", - expectedError: nil, - }, - { - name: "not allowed and not denied user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - UID: notAllowedAndNotDeniedUserUid, - Username: "not-allowed-and-not-denied-user", - }, - }, - }, - expectedDenyReason: "user 'not-allowed-and-not-denied-user' does not have permissions to create/update NginxIngressController. Verb '*' needed for resource 'IngressClass' in group 'networking.k8s.io' version 'v1'.", - expectedError: nil, - }, - { - name: "error user", - req: admission.Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - UserInfo: authenticationv1.UserInfo{ - UID: errUserUid, - }, - }, - }, - expectedDenyReason: "", - expectedError: failedError, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - denyReason, err := SarAuthenticateNginxIngressController(context.Background(), logr.Discard(), cl, tc.req) - - if denyReason != tc.expectedDenyReason { - t.Errorf("expected denyReason %v, got %v", tc.expectedDenyReason, denyReason) - } - - if !errors.Is(err, tc.expectedError) { - t.Errorf("expected error %v, to be of type %v", tc.expectedError, err) - } - }) - } -} diff --git a/pkg/webhook/testcerts/ca.crt b/pkg/webhook/testcerts/ca.crt deleted file mode 100644 index df54c5d9..00000000 --- a/pkg/webhook/testcerts/ca.crt +++ /dev/null @@ -1 +0,0 @@ -this is a fake ca crt used for unit testing \ No newline at end of file diff --git a/pkg/webhook/testcerts/tls.crt b/pkg/webhook/testcerts/tls.crt deleted file mode 100644 index 3253d8c1..00000000 --- a/pkg/webhook/testcerts/tls.crt +++ /dev/null @@ -1 +0,0 @@ -this is a fake tls crt used for unit testing \ No newline at end of file diff --git a/pkg/webhook/testcerts/tls.key b/pkg/webhook/testcerts/tls.key deleted file mode 100644 index 687ce75a..00000000 --- a/pkg/webhook/testcerts/tls.key +++ /dev/null @@ -1 +0,0 @@ -this is a fake tls key used for unit testing \ No newline at end of file diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go deleted file mode 100644 index 1c60b093..00000000 --- a/pkg/webhook/webhook.go +++ /dev/null @@ -1,198 +0,0 @@ -package webhook - -import ( - "context" - "errors" - "fmt" - "os" - "path/filepath" - - globalCfg "github.com/Azure/aks-app-routing-operator/pkg/config" - "github.com/Azure/aks-app-routing-operator/pkg/manifests" - "github.com/Azure/aks-app-routing-operator/pkg/util" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" -) - -type webhookType interface { - admissionregistrationv1.ValidatingWebhook | admissionregistrationv1.MutatingWebhook -} - -// Webhook defines a webhook that can be registered and added to the manager -type Webhook[T webhookType] struct { - AddToManager func(manager.Manager) error - Definition func(c *config) (T, error) -} - -// Validating is a list of ValidatingWebhook to be registered. Append to this slice to register more webhooks -var Validating []Webhook[admissionregistrationv1.ValidatingWebhook] - -// Mutating is a list of MutatingWebhook to be registered. Append to this slice to register more webhooks -var Mutating []Webhook[admissionregistrationv1.MutatingWebhook] - -type config struct { - serviceName, namespace, serviceUrl string - port int32 - certDir string - certName, caName, keyName string - - validatingWebhookConfigName string - mutatingWebhookConfigName string - - validatingWebhooks []Webhook[admissionregistrationv1.ValidatingWebhook] - mutatingWebhooks []Webhook[admissionregistrationv1.MutatingWebhook] -} - -// New returns a new webhook config -func New(globalCfg *globalCfg.Config) (*config, error) { - if globalCfg == nil { - return nil, errors.New("config is nil") - } - - return &config{ - serviceName: globalCfg.OperatorWebhookService, - namespace: globalCfg.OperatorNs, - serviceUrl: globalCfg.OperatorWebhookServiceUrl, - port: int32(globalCfg.WebhookPort), - certDir: globalCfg.CertDir, - validatingWebhookConfigName: "app-routing-validating", - mutatingWebhookConfigName: "app-routing-mutating", - validatingWebhooks: Validating, - mutatingWebhooks: Mutating, - certName: globalCfg.CertName, - caName: globalCfg.CaName, - keyName: globalCfg.KeyName, - }, nil -} - -// EnsureWebhookConfigurations ensures the webhook configurations exist in the cluster in the desired state -func (c *config) EnsureWebhookConfigurations(ctx context.Context, cl client.Client, globalCfg *globalCfg.Config) error { - lgr := log.FromContext(ctx).WithName("webhooks") - - // todo: need to make this a reconciler? that's constantly running - - lgr.Info("calculating ValidatingWebhookConfiguration") - var validatingWhs []admissionregistrationv1.ValidatingWebhook - for _, wh := range c.validatingWebhooks { - wh, err := wh.Definition(c) - if err != nil { - return fmt.Errorf("getting webhook definition: %w", err) - } - - validatingWhs = append(validatingWhs, wh) - } - - validatingWhc := &admissionregistrationv1.ValidatingWebhookConfiguration{ - TypeMeta: metav1.TypeMeta{ - Kind: "ValidatingWebhookConfiguration", - APIVersion: "admissionregistration.k8s.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: c.validatingWebhookConfigName, - Labels: map[string]string{ - // https://learn.microsoft.com/en-us/azure/aks/faq#can-admission-controller-webhooks-impact-kube-system-and-internal-aks-namespaces - "admissions.enforcer/disabled": "true", - }, - }, - Webhooks: validatingWhs, - } - - lgr.Info("calculating MutatingWebhookConfiguration") - var mutatingWhs []admissionregistrationv1.MutatingWebhook - for _, wh := range c.mutatingWebhooks { - wh, err := wh.Definition(c) - if err != nil { - return fmt.Errorf("getting webhook definition: %w", err) - } - - mutatingWhs = append(mutatingWhs, wh) - } - - mutatingWhc := &admissionregistrationv1.MutatingWebhookConfiguration{ - TypeMeta: metav1.TypeMeta{ - Kind: "MutatingWebhookConfiguration", - APIVersion: "admissionregistration.k8s.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: c.mutatingWebhookConfigName, - Labels: map[string]string{ - // https://learn.microsoft.com/en-us/azure/aks/faq#can-admission-controller-webhooks-impact-kube-system-and-internal-aks-namespaces - "admissions.enforcer/disabled": "true", - }, - }, - Webhooks: mutatingWhs, - } - - lgr.Info("ensuring namespace exists") - appRoutingNamespace := &corev1.Namespace{} - appRoutingNamespace = manifests.Namespace(globalCfg) - if err := util.Upsert(ctx, cl, appRoutingNamespace); err != nil { - return fmt.Errorf("upserting namespace: %w", err) - } - - ownerRef := manifests.GetOwnerRefs(appRoutingNamespace, false) - - lgr.Info("ensuring webhook configuration") - whs := []client.Object{validatingWhc, mutatingWhc} - for _, wh := range whs { - wh.SetOwnerReferences(ownerRef) - copy := wh.DeepCopyObject().(client.Object) - lgr := lgr.WithValues("resource", wh.GetName(), "resourceKind", wh.GetObjectKind().GroupVersionKind().Kind) - lgr.Info("upserting resource") - - if err := util.Upsert(ctx, cl, copy); err != nil { - return fmt.Errorf("upserting resource: %w", err) - } - } - - lgr.Info("finished ensuring webhook configuration") - return nil -} - -// AddWebhooks adds the webhooks to the manager -func (c *config) AddWebhooks(mgr manager.Manager) error { - for _, wh := range c.validatingWebhooks { - if err := wh.AddToManager(mgr); err != nil { - return fmt.Errorf("adding webhook to manager: %w", err) - } - } - - for _, wh := range c.mutatingWebhooks { - if err := wh.AddToManager(mgr); err != nil { - return fmt.Errorf("adding webhook to manager: %w", err) - } - } - - return nil -} - -// GetClientConfig returns the client config for the webhook service. path should start with a / -func (c *config) GetClientConfig(path string) (admissionregistrationv1.WebhookClientConfig, error) { - caPath := filepath.Join(c.certDir, c.caName) - caBundle, err := os.ReadFile(caPath) - if err != nil { - return admissionregistrationv1.WebhookClientConfig{}, fmt.Errorf("reading ca bundle: %w", err) - } - - whClient := admissionregistrationv1.WebhookClientConfig{ - CABundle: caBundle, - } - - if c.serviceUrl != "" { - whClient.URL = util.ToPtr(c.serviceUrl + path) - - } else { - whClient.Service = &admissionregistrationv1.ServiceReference{ - Name: c.serviceName, - Namespace: c.namespace, - Port: &c.port, - Path: &path, - } - } - - return whClient, nil -} diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go deleted file mode 100644 index 0ab5b7ce..00000000 --- a/pkg/webhook/webhook_test.go +++ /dev/null @@ -1,224 +0,0 @@ -package webhook - -import ( - "context" - "errors" - "testing" - - globalCfg "github.com/Azure/aks-app-routing-operator/pkg/config" - "github.com/Azure/aks-app-routing-operator/pkg/util" - "github.com/stretchr/testify/require" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client/fake" -) - -func TestNew(t *testing.T) { - cases := []struct { - name string - global *globalCfg.Config - expected *config - err error - }{ - { - name: "nil config", - global: nil, - expected: nil, - err: errors.New("config is nil"), - }, - { - name: "valid config", - global: &globalCfg.Config{ - OperatorWebhookService: "service-name", - OperatorNs: "namespace", - WebhookPort: 443, - CertDir: "cert-dir", - }, - expected: &config{ - serviceName: "service-name", - namespace: "namespace", - port: 443, - certDir: "cert-dir", - validatingWebhookConfigName: "app-routing-validating", - mutatingWebhookConfigName: "app-routing-mutating", - validatingWebhooks: Validating, - mutatingWebhooks: Mutating, - }, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - actual, err := New(c.global) - require.Equal(t, c.err, err, "unexpected error") - if err != nil { - return - } - - if actual.serviceName != c.expected.serviceName { - t.Errorf("expected service name %s, got %s", c.expected.serviceName, actual.serviceName) - } - if actual.namespace != c.expected.namespace { - t.Errorf("expected namespace %s, got %s", c.expected.namespace, actual.namespace) - } - if actual.port != c.expected.port { - t.Errorf("expected port %d, got %d", c.expected.port, actual.port) - } - if actual.certDir != c.expected.certDir { - t.Errorf("expected cert dir %s, got %s", c.expected.certDir, actual.certDir) - } - if actual.validatingWebhookConfigName != c.expected.validatingWebhookConfigName { - t.Errorf("expected validating webhook config name %s, got %s", c.expected.validatingWebhookConfigName, actual.validatingWebhookConfigName) - } - if actual.mutatingWebhookConfigName != c.expected.mutatingWebhookConfigName { - t.Errorf("expected mutating webhook config name %s, got %s", c.expected.mutatingWebhookConfigName, actual.mutatingWebhookConfigName) - } - if len(actual.validatingWebhooks) != len(c.expected.validatingWebhooks) { - t.Errorf("expected %d validating webhooks, got %d", len(c.expected.validatingWebhooks), len(actual.validatingWebhooks)) - } - if len(actual.mutatingWebhooks) != len(c.expected.mutatingWebhooks) { - t.Errorf("expected %d mutating webhooks, got %d", len(c.expected.mutatingWebhooks), len(actual.mutatingWebhooks)) - } - }) - } -} - -func TestEnsureWebhookConfigurations(t *testing.T) { - t.Run("valid webhooks", func(t *testing.T) { - globalCfg := &globalCfg.Config{ - NS: "app-routing-system", - } - c := &config{ - validatingWebhookConfigName: "app-routing-validating", - mutatingWebhookConfigName: "app-routing-mutating", - validatingWebhooks: Validating, - mutatingWebhooks: Mutating, - certDir: "testcerts", - caName: "ca.crt", - } - - cl := fake.NewClientBuilder().Build() - var validatingWhCfg admissionregistrationv1.ValidatingWebhookConfiguration - var mutatingWhCfg admissionregistrationv1.MutatingWebhookConfiguration - // prove webhook configurations don't exist - require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &validatingWhCfg)), "expected not to find validating webhook config") - require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &mutatingWhCfg)), "expected not to find mutating webhook config") - - // prove webhook configurations exist after ensuring - require.NoError(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg), "unexpected error") - require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &validatingWhCfg), "unexpected error getting validating webhook config") - require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &mutatingWhCfg), "unexpected error getting mutating webhook config") - - // prove ownerReferences are set on webhook configurations - require.True(t, len(validatingWhCfg.OwnerReferences) == 1, "expected 1 owner reference") - require.True(t, validatingWhCfg.OwnerReferences[0].Name == "app-routing-system", "expected owner reference name to be app-routing-system") - require.True(t, validatingWhCfg.OwnerReferences[0].Kind == "Namespace", "expected owner reference kind to be Namespace") - require.True(t, validatingWhCfg.OwnerReferences[0].APIVersion == "v1", "expected owner reference api version to be v1") - require.True(t, len(mutatingWhCfg.OwnerReferences) == 1, "expected 1 owner reference") - require.True(t, mutatingWhCfg.OwnerReferences[0].Name == "app-routing-system", "expected owner reference name to be app-routing-system") - require.True(t, mutatingWhCfg.OwnerReferences[0].Kind == "Namespace", "expected owner reference kind to be Namespace") - require.True(t, mutatingWhCfg.OwnerReferences[0].APIVersion == "v1", "expected owner reference api version to be v1") - - }) - - t.Run("invalid webhooks", func(t *testing.T) { - - globalCfg := &globalCfg.Config{} - c := &config{ - validatingWebhooks: []Webhook[admissionregistrationv1.ValidatingWebhook]{ - { - Definition: func(c *config) (admissionregistrationv1.ValidatingWebhook, error) { - return admissionregistrationv1.ValidatingWebhook{}, errors.New("invalid webhook") - }, - }, - }, - mutatingWebhooks: []Webhook[admissionregistrationv1.MutatingWebhook]{ - { - Definition: func(c *config) (admissionregistrationv1.MutatingWebhook, error) { - return admissionregistrationv1.MutatingWebhook{}, errors.New("invalid webhook") - }, - }, - }, - } - - cl := fake.NewClientBuilder().Build() - require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg).Error() == "getting webhook definition: invalid webhook", "expected error") - - c.validatingWebhooks = nil - require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg).Error() == "getting webhook definition: invalid webhook", "expected error") - }) -} - -func TestGetClientConfig(t *testing.T) { - cases := []struct { - name string - path string - cfg *config - expected admissionregistrationv1.WebhookClientConfig - err error - }{ - { - name: "basic names service", - cfg: &config{ - serviceName: "service-name", - namespace: "namespace", - port: 443, - certDir: "testcerts", - caName: "ca.crt", - }, - path: "/example-path", - expected: admissionregistrationv1.WebhookClientConfig{ - Service: &admissionregistrationv1.ServiceReference{ - Name: "service-name", - Namespace: "namespace", - Path: util.ToPtr("/example-path"), - Port: util.ToPtr(int32(443)), - }, - }, - err: nil, - }, - { - name: "basic names with service url", - cfg: &config{ - serviceUrl: "service-name.namespace.svc.local", - certDir: "testcerts", - caName: "ca.crt", - }, - path: "/example-path", - expected: admissionregistrationv1.WebhookClientConfig{ - URL: util.ToPtr("service-name.namespace.svc.local/example-path"), - }, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - actual, err := c.cfg.GetClientConfig(c.path) - if err != c.err { - t.Errorf("unexpected error: %v", err) - } - - if (actual.URL != nil || c.expected.URL != nil) && (*actual.URL != *c.expected.URL) { - t.Errorf("expected url %s, got %s", *c.expected.URL, *actual.URL) - } - - if actual.Service == nil && c.expected.Service == nil { - return - } - - if actual.Service.Name != c.expected.Service.Name { - t.Errorf("expected service name %s, got %s", c.expected.Service.Name, actual.Service.Name) - } - if actual.Service.Namespace != c.expected.Service.Namespace { - t.Errorf("expected service namespace %s, got %s", c.expected.Service.Namespace, actual.Service.Namespace) - } - if *actual.Service.Port != *c.expected.Service.Port { - t.Errorf("expected service port %d, got %d", *c.expected.Service.Port, *actual.Service.Port) - } - if *actual.Service.Path != *c.expected.Service.Path { - t.Errorf("expected service path %s, got %s", *c.expected.Service.Path, *actual.Service.Path) - } - }) - } -} diff --git a/testing/e2e/manifests/clientServer.go b/testing/e2e/manifests/clientServer.go index eff67d73..1acc9fac 100644 --- a/testing/e2e/manifests/clientServer.go +++ b/testing/e2e/manifests/clientServer.go @@ -2,6 +2,7 @@ package manifests import ( _ "embed" + "fmt" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" @@ -41,10 +42,10 @@ func (t testingResources) Objects() []client.Object { return ret } -func ClientAndServer(namespace, name, zoneName, nameserver, keyvaultURI string, isClusterScoped bool) testingResources { +func ClientAndServer(namespace, name, zoneName, nameserver, keyvaultURI string, isClusterScoped bool, svc string) testingResources { var host string if isClusterScoped { - host = "nginx.app-routing-system.svc.cluster.local" + host = fmt.Sprintf("%s.app-routing-system.svc.cluster.local", svc) } else { host = strings.ToLower(namespace) + "." + strings.TrimRight(zoneName, ".") } @@ -87,6 +88,10 @@ func ClientAndServer(namespace, name, zoneName, nameserver, keyvaultURI string, service := &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: serviceName, Namespace: namespace, @@ -106,6 +111,10 @@ func ClientAndServer(namespace, name, zoneName, nameserver, keyvaultURI string, }, } ingress := &netv1.Ingress{ + TypeMeta: metav1.TypeMeta{ + Kind: "Ingress", + APIVersion: "networking.k8s.io/v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: ingressName, Namespace: namespace, diff --git a/testing/e2e/manifests/common.go b/testing/e2e/manifests/common.go index 6592bb24..c9d2e549 100644 --- a/testing/e2e/manifests/common.go +++ b/testing/e2e/manifests/common.go @@ -70,6 +70,10 @@ func newGoDeployment(contents, namespace, name string) *appsv1.Deployment { } return &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, @@ -102,6 +106,10 @@ func newGoDeployment(contents, namespace, name string) *appsv1.Deployment { // UncollisionedNs returns a namespace with a guaranteed unique name after creating the namespace func UncollisionedNs() *corev1.Namespace { return &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + Kind: "Namespace", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ GenerateName: "app-routing-e2e-", Labels: map[string]string{ diff --git a/testing/e2e/manifests/e2e.go b/testing/e2e/manifests/e2e.go index 1dda6425..2fe20907 100644 --- a/testing/e2e/manifests/e2e.go +++ b/testing/e2e/manifests/e2e.go @@ -16,12 +16,20 @@ const ( func E2e(image, loadableProvisionedJson string) []client.Object { ret := []client.Object{ &corev1.ServiceAccount{ + TypeMeta: metav1.TypeMeta{ + Kind: "ServiceAccount", + APIVersion: corev1.SchemeGroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-e2e", Namespace: e2eNamespace, }, }, &rbacv1.ClusterRoleBinding{ + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterRoleBinding", + APIVersion: rbacv1.SchemeGroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-e2e", }, @@ -39,6 +47,10 @@ func E2e(image, loadableProvisionedJson string) []client.Object { }, }, &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: corev1.SchemeGroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "infrastructure", Namespace: e2eNamespace, @@ -48,6 +60,10 @@ func E2e(image, loadableProvisionedJson string) []client.Object { }, }, &batchv1.Job{ + TypeMeta: metav1.TypeMeta{ + Kind: "Job", + APIVersion: batchv1.SchemeGroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-operator-e2e", Namespace: e2eNamespace, diff --git a/testing/e2e/manifests/nginxIngressController.go b/testing/e2e/manifests/nginxIngressController.go index eac65ba5..15aba1fa 100644 --- a/testing/e2e/manifests/nginxIngressController.go +++ b/testing/e2e/manifests/nginxIngressController.go @@ -7,6 +7,10 @@ import ( func NewNginxIngressController(name, ingressClassName string) *v1alpha1.NginxIngressController { return &v1alpha1.NginxIngressController{ + TypeMeta: metav1.TypeMeta{ + Kind: "NginxIngressController", + APIVersion: v1alpha1.GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: name, }, diff --git a/testing/e2e/manifests/operator.go b/testing/e2e/manifests/operator.go index 813339b7..39b71aa9 100644 --- a/testing/e2e/manifests/operator.go +++ b/testing/e2e/manifests/operator.go @@ -122,9 +122,6 @@ func (o *OperatorConfig) args(publicZones, privateZones []string) []string { if o.Version == OperatorVersionLatest { ret = append(ret, "--dns-sync-interval", (time.Second * 15).String()) - ret = append(ret, "--operator-namespace", operatorNs) - ret = append(ret, "--operator-webhook-service", "app-routing-operator-webhook") - ret = append(ret, "--enable-webhook") } var zones []string @@ -155,12 +152,20 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera var ret []client.Object namespace := &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, ObjectMeta: metav1.ObjectMeta{ Name: managedResourceNs, }, } serviceAccount := &corev1.ServiceAccount{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ServiceAccount", + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-operator", Namespace: operatorNs, @@ -168,6 +173,10 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera } clusterRoleBinding := &rbacv1.ClusterRoleBinding{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "rbac.authorization.k8s.io/v1", + Kind: "ClusterRoleBinding", + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-operator", Namespace: operatorNs, @@ -187,6 +196,10 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera } baseDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-operator", Namespace: operatorNs, @@ -244,6 +257,10 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera } podDisrutptionBudget := &policyv1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "policy/v1", + Kind: "PodDisruptionBudget", + }, ObjectMeta: metav1.ObjectMeta{ Name: "app-routing-operator", Namespace: "app-routing-system", @@ -256,35 +273,6 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera }, } - webhookService := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "app-routing-operator-webhook", - Namespace: operatorNs, - }, - Spec: corev1.ServiceSpec{ - IPFamilies: []corev1.IPFamily{ - "IPv4", - }, - IPFamilyPolicy: &SingleStackIPFamilyPolicy, - Ports: []corev1.ServicePort{ - { - Name: "client", - Port: 9443, - Protocol: corev1.ProtocolTCP, - TargetPort: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 9443, - }, - }, - }, - Selector: map[string]string{ - "app": "app-routing-operator", - }, - SessionAffinity: corev1.ServiceAffinityNone, - Type: corev1.ServiceTypeClusterIP, - }, - } - ret = append(ret, []client.Object{ namespace, serviceAccount, @@ -300,10 +288,9 @@ func Operator(latestImage string, publicZones, privateZones []string, cfg *Opera baseDeployment.Spec.Template.Spec.Containers[0].StartupProbe = nil baseDeployment.Spec.Template.Spec.Containers[0].VolumeMounts = nil baseDeployment.Spec.Template.Spec.Volumes = nil - ret = append(ret, baseDeployment) case OperatorVersionLatest: - ret = append(ret, webhookService, baseDeployment) + ret = append(ret, baseDeployment) if cleanDeploy { ret = append(ret, NewNginxIngressController("default", "webapprouting.kubernetes.azure.com")) diff --git a/testing/e2e/manifests/prometheus.go b/testing/e2e/manifests/prometheus.go index 5bbc7884..35fd4a6d 100644 --- a/testing/e2e/manifests/prometheus.go +++ b/testing/e2e/manifests/prometheus.go @@ -101,6 +101,10 @@ scrape_configs: server := []client.Object{ &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: name + "-server", Namespace: namespace, @@ -110,6 +114,10 @@ scrape_configs: }, }, &corev1.ServiceAccount{ + TypeMeta: metav1.TypeMeta{ + Kind: "ServiceAccount", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: name + "-server", Namespace: namespace, @@ -213,6 +221,10 @@ scrape_configs: }, }, &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: name + "-server", Namespace: namespace, diff --git a/testing/e2e/suites/all.go b/testing/e2e/suites/all.go index 4e1a5894..f386b7cd 100644 --- a/testing/e2e/suites/all.go +++ b/testing/e2e/suites/all.go @@ -16,7 +16,7 @@ func All(infra infra.Provisioned) tests.Ts { t = append(t, basicSuite(infra)...) t = append(t, osmSuite(infra)...) t = append(t, promSuite(infra)...) - t = append(t, nicWebhookTests(infra)...) + t = append(t, nicTests(infra)...) ret := make(tests.Ts, len(t)) for i, t := range t { diff --git a/testing/e2e/suites/basic.go b/testing/e2e/suites/basic.go index 3c65dbc9..dd34e87b 100644 --- a/testing/e2e/suites/basic.go +++ b/testing/e2e/suites/basic.go @@ -3,9 +3,12 @@ package suites import ( "context" "fmt" + "sync" + "github.com/Azure/aks-app-routing-operator/testing/e2e/infra" "github.com/Azure/aks-app-routing-operator/testing/e2e/logger" "github.com/Azure/aks-app-routing-operator/testing/e2e/manifests" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" @@ -19,6 +22,8 @@ var ( // is used for the tests for that dns zone. Using shared namespaces // allow us to appropriately test upgrade scenarios. basicNs = make(map[string]*corev1.Namespace) + // nsMutex is a mutex that is used to protect the basicNs map. Without this we chance concurrent goroutine map access panics + nsMutex = sync.RWMutex{} ) func basicSuite(in infra.Provisioned) []test { @@ -31,7 +36,7 @@ func basicSuite(in infra.Provisioned) []test { withZones(manifests.NonZeroDnsZoneCounts, manifests.NonZeroDnsZoneCounts). build(), run: func(ctx context.Context, config *rest.Config, operator manifests.OperatorConfig) error { - if err := clientServerTest(ctx, config, operator, basicNs, in, nil); err != nil { + if err := clientServerTest(ctx, config, operator, basicNs, in, nil, nil); err != nil { return err } @@ -54,7 +59,7 @@ func basicSuite(in infra.Provisioned) []test { service.SetAnnotations(annotations) return nil - }); err != nil { + }, nil); err != nil { return err } @@ -69,13 +74,16 @@ type modifier func(ingress *netv1.Ingress, service *corev1.Service, z zoner) err // clientServerTest is a test that deploys a client and server application and ensures the client can reach the server. // This is the standard test used to check traffic flow is working. -var clientServerTest = func(ctx context.Context, config *rest.Config, operator manifests.OperatorConfig, namespaces map[string]*corev1.Namespace, infra infra.Provisioned, mod modifier) error { +var clientServerTest = func(ctx context.Context, config *rest.Config, operator manifests.OperatorConfig, namespaces map[string]*corev1.Namespace, infra infra.Provisioned, mod modifier, serviceName *string) error { lgr := logger.FromContext(ctx) lgr.Info("starting test") if namespaces == nil { namespaces = make(map[string]*corev1.Namespace) } + if serviceName == nil { + serviceName = to.Ptr("nginx") + } c, err := client.New(config, client.Options{}) if err != nil { @@ -105,7 +113,10 @@ var clientServerTest = func(ctx context.Context, config *rest.Config, operator m } if operator.Zones.Public == manifests.DnsZoneCountNone && operator.Zones.Private == manifests.DnsZoneCountNone { - zones = append(zones, zone{name: "nginx.app-routing-system.svc.cluster.local:80", nameserver: infra.Cluster.GetDnsServiceIp()}) + zones = append(zones, zone{ + name: fmt.Sprintf("%s.app-routing-system.svc.cluster.local:80", *serviceName), + nameserver: infra.Cluster.GetDnsServiceIp(), + }) } var eg errgroup.Group @@ -115,10 +126,13 @@ var clientServerTest = func(ctx context.Context, config *rest.Config, operator m lgr := logger.FromContext(ctx).With("zone", zone.GetName()) ctx := logger.WithContext(ctx, lgr) + // multiple goroutines access the same map at the same time which is not safe + nsMutex.Lock() if val, ok := namespaces[zone.GetName()]; !ok || val == nil { namespaces[zone.GetName()] = manifests.UncollisionedNs() } ns := namespaces[zone.GetName()] + nsMutex.Unlock() lgr.Info("creating namespace") if err := upsert(ctx, c, ns); err != nil { @@ -128,7 +142,7 @@ var clientServerTest = func(ctx context.Context, config *rest.Config, operator m lgr = lgr.With("namespace", ns.Name) ctx = logger.WithContext(ctx, lgr) - testingResources := manifests.ClientAndServer(ns.Name, "e2e-testing", zone.GetName(), zone.GetNameserver(), infra.Cert.GetId(), operator.Zones.Public == manifests.DnsZoneCountNone && operator.Zones.Private == manifests.DnsZoneCountNone) + testingResources := manifests.ClientAndServer(ns.Name, "e2e-testing", zone.GetName(), zone.GetNameserver(), infra.Cert.GetId(), operator.Zones.Public == manifests.DnsZoneCountNone && operator.Zones.Private == manifests.DnsZoneCountNone, *serviceName) if mod != nil { if err := mod(testingResources.Ingress, testingResources.Service, zone); err != nil { return fmt.Errorf("modifying ingress and service: %w", err) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 45f88d1f..b46cc36d 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -2,6 +2,7 @@ package suites import ( "context" + "errors" "fmt" "time" @@ -9,13 +10,14 @@ import ( "github.com/Azure/aks-app-routing-operator/testing/e2e/infra" "github.com/Azure/aks-app-routing-operator/testing/e2e/logger" "github.com/Azure/aks-app-routing-operator/testing/e2e/manifests" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + netv1 "k8s.io/api/networking/v1" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" rbacv1 "k8s.io/api/rbac/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -40,10 +42,10 @@ func init() { rbacv1.AddToScheme(scheme) } -func nicWebhookTests(in infra.Provisioned) []test { +func nicTests(in infra.Provisioned) []test { return []test{ { - name: "nic webhook", + name: "nic validations", cfgs: builderFromInfra(in). withOsm(in, false, true). withVersions(manifests.OperatorVersionLatest). @@ -60,25 +62,21 @@ func nicWebhookTests(in infra.Provisioned) []test { return fmt.Errorf("creating client: %w") } - testNIC := manifests.NewNginxIngressController("nginx-ingress-controller", "test-nic-ingress-class") - lgr.Info("creating basic NginxIngressController") - if err := upsert(ctx, c, testNIC); err != nil { - return fmt.Errorf("creating NginxIngressController: %w", err) + // validate that crd rejected with invalid fields + testNIC := manifests.NewNginxIngressController("nginx-ingress-controller", "Invalid+Characters") + lgr.Info("creating NginxIngressController with invalid ingressClassName") + if err := c.Create(ctx, testNIC); err == nil { + return fmt.Errorf("able to create NginxIngressController with invalid ingressClassName '%s'", testNIC.Spec.IngressClassName) } - oldNICName := testNIC.Spec.IngressClassName - testNIC.Spec.IngressClassName = existingOperatorIngressClass - lgr.Info("testing existing ingressclass") - if err := upsert(ctx, c, testNIC); err == nil { - return fmt.Errorf("created NginxIngressController with existing IngressClass") + testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") + testNIC.Spec.ControllerNamePrefix = "Invalid+Characters" + lgr.Info("creating NginxIngressController with invalid controllerNamePrefix") + if err := c.Create(ctx, testNIC); err == nil { + return fmt.Errorf("able to create NginxIngressController with invalid controllerNamePrefix '%s'", testNIC.Spec.ControllerNamePrefix) } - testNIC.Spec.IngressClassName = oldNICName - if err = c.Delete(ctx, testNIC); err != nil { - return fmt.Errorf("deleting NginxIngressController: %w", err) - } - - lgr.Info("finished testing nic webhook") + lgr.Info("finished testing") return nil }, }, @@ -100,61 +98,60 @@ func nicWebhookTests(in infra.Provisioned) []test { return fmt.Errorf("creating client: %w") } - testNIC := manifests.NewNginxIngressController("default", existingOperatorIngressClass) - var copyNIC v1alpha1.NginxIngressController - err = c.Get(ctx, client.ObjectKeyFromObject(testNIC), ©NIC) - if err != nil { - return fmt.Errorf("get default nic: %w", err) - } - - copyNIC.Spec.LoadBalancerAnnotations = map[string]string{ + privateNic := manifests.NewNginxIngressController("private", "private.ingress.class") + privateNic.Spec.LoadBalancerAnnotations = map[string]string{ "service.beta.kubernetes.io/azure-load-balancer-internal": "true", } - - lgr.Info("updating NIC to internal") - if err := c.Update(ctx, ©NIC); err != nil { - return fmt.Errorf("updating NIC to internal: %w", err) + if err := upsert(ctx, c, privateNic); err != nil { + return fmt.Errorf("ensuring private NIC: %w", err) } - if err := wait.PollImmediate(1*time.Second, 3*time.Minute, func() (bool, error) { - lgr.Info("validating service is updated with new annotations") - - var serviceCopy corev1.Service - if err := c.Get(ctx, client.ObjectKey{Namespace: "app-routing-system", Name: "nginx"}, &serviceCopy); err != nil { - if apierrors.IsNotFound(err) { - lgr.Info("nginx service not found") - return false, nil - } - - return false, fmt.Errorf("getting nginx service: %w", err) + var service v1alpha1.ManagedObjectReference + lgr.Info("waiting for service associated with private NIC to be ready") + if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + lgr.Info("checking if private NIC service is ready") + var nic v1alpha1.NginxIngressController + if err := c.Get(ctx, client.ObjectKeyFromObject(privateNic), &nic); err != nil { + return false, fmt.Errorf("get private nic: %w", err) } - if _, ok := serviceCopy.ObjectMeta.Annotations["service.beta.kubernetes.io/azure-load-balancer-internal"]; !ok { - lgr.Info("nginx annotations not found") + if nic.Status.ManagedResourceRefs == nil { return false, nil } - return true, nil + for _, ref := range nic.Status.ManagedResourceRefs { + if ref.Kind == "Service" { + lgr.Info("found service") + service = ref + return true, nil + } + } + + lgr.Info("service not found") + return false, nil }); err != nil { - return fmt.Errorf("waiting for updated nginx service: %w", err) + return fmt.Errorf("waiting for private NIC to be ready: %w", err) } - if err := clientServerTest(ctx, config, operator, nil, in, nil); err != nil { - return err + lgr.Info("validating service contains private annotations") + var serviceCopy corev1.Service + if err := c.Get(ctx, client.ObjectKey{Namespace: service.Namespace, Name: service.Name}, &serviceCopy); err != nil { + return fmt.Errorf("getting service: %w", err) } - err = c.Get(ctx, client.ObjectKeyFromObject(testNIC), ©NIC) - if err != nil { - return fmt.Errorf("get default nic: %w", err) + if _, ok := serviceCopy.ObjectMeta.Annotations["service.beta.kubernetes.io/azure-load-balancer-internal"]; !ok { + lgr.Error("private nginx annotations not found") + return errors.New("private nginx annotations not found") } - copyNIC.Spec.LoadBalancerAnnotations = map[string]string{} - lgr.Info("reverting NIC to external") - if err := c.Update(ctx, ©NIC); err != nil { - return fmt.Errorf("updating NIC to external: %w", err) + if err := clientServerTest(ctx, config, operator, nil, in, func(ingress *netv1.Ingress, service *corev1.Service, z zoner) error { + ingress.Spec.IngressClassName = to.Ptr(privateNic.Spec.IngressClassName) + return nil + }, to.Ptr(service.Name)); err != nil { + return err } - lgr.Info("finished testing nic webhook") + lgr.Info("finished testing") return nil }, }, diff --git a/testing/e2e/suites/osm.go b/testing/e2e/suites/osm.go index 813ff825..317697e6 100644 --- a/testing/e2e/suites/osm.go +++ b/testing/e2e/suites/osm.go @@ -30,7 +30,7 @@ func osmSuite(in infra.Provisioned) []test { ingress.SetAnnotations(annotations) return nil - }); err != nil { + }, nil); err != nil { return err } @@ -53,7 +53,7 @@ func osmSuite(in infra.Provisioned) []test { service.SetAnnotations(annotations) return nil - }); err != nil { + }, nil); err != nil { return err } diff --git a/testing/e2e/suites/utils.go b/testing/e2e/suites/utils.go index b5243dc9..50481c0f 100644 --- a/testing/e2e/suites/utils.go +++ b/testing/e2e/suites/utils.go @@ -55,7 +55,7 @@ func upsert(ctx context.Context, c client.Client, obj client.Object) error { } lgr.Info("object already exists, attempting to update") - if err := c.Update(ctx, copy); err != nil { + if err := c.Patch(ctx, copy, client.Apply, client.ForceOwnership, client.FieldOwner("e2e-test")); err != nil { return fmt.Errorf("updating object: %w", err) }