Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NO-JIRA: (feat) ingress certificates gather #917

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ linters:
disable-all: true
enable:
- bodyclose
- deadcode
- dogsled
- dupl
- errcheck
Expand All @@ -90,13 +89,11 @@ linters:
- noctx
- rowserrcheck
- staticcheck
- structcheck
- stylecheck
- typecheck
- unconvert
- unparam
- unused
- varcheck
- whitespace

# don't enable:
Expand Down
1 change: 1 addition & 0 deletions pkg/gatherers/clusterconfig/clusterconfig_gatherer.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ var gatheringFunctions = map[string]gathererFuncPtr{
"image_registries": (*Gatherer).GatherClusterImageRegistry,
"infrastructures": (*Gatherer).GatherClusterInfrastructure,
"ingress": (*Gatherer).GatherClusterIngress,
"ingress_certificates": (*Gatherer).GatherClusterIngressCertificates,
"install_plans": (*Gatherer).GatherInstallPlans,
"jaegers": (*Gatherer).GatherJaegerCR,
"kube_controller_manager_logs": (*Gatherer).GatherKubeControllerManagerLogs,
Expand Down
168 changes: 168 additions & 0 deletions pkg/gatherers/clusterconfig/gather_cluster_ingress_certificates.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package clusterconfig

import (
"context"
"crypto/x509"
"encoding/pem"
"fmt"

operatorclient "github.com/openshift/client-go/operator/clientset/versioned"

"k8s.io/klog/v2"

"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"

"github.com/openshift/insights-operator/pkg/record"
)

const ingressCertificatesLimits = 64

var ingressNamespaces = []string{
"openshift-ingress-operator",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that it's possible (supported) to create an Ingress controller also in this namespace please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

"openshift-ingress",
}

type CertificateInfo struct {
Name string `json:"name"`
Namespace string `json:"namespace"`
NotBefore metav1.Time `json:"not_before"`
NotAfter metav1.Time `json:"not_after"`
Controllers []ControllerInfo `json:"controllers"`
}

type ControllerInfo struct {
Name string `json:"name"`
Namespace string `json:"namespace"`
}

func (g *Gatherer) GatherClusterIngressCertificates(ctx context.Context) ([]record.Record, []error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is missing here please

gatherKubeClient, err := kubernetes.NewForConfig(g.gatherProtoKubeConfig)
if err != nil {
return nil, []error{err}
}

operatorClient, err := operatorclient.NewForConfig(g.gatherKubeConfig)
if err != nil {
return nil, []error{err}
}

return gatherClusterIngressCertificates(ctx, gatherKubeClient.CoreV1(), operatorClient)
}

func gatherClusterIngressCertificates(
ctx context.Context,
coreClient corev1client.CoreV1Interface,
operatorClient operatorclient.Interface) ([]record.Record, []error) {

var certificates []*CertificateInfo
var errs []error

// Step 1: Collect router-ca and router-certs-default
routerCACert, routerCACertErr := getCertificateInfoFromSecret(ctx, coreClient, "openshift-ingress-operator", "router-ca")
if routerCACertErr != nil {
errs = append(errs, routerCACertErr)
} else {
certificates = append(certificates, routerCACert)
}

routerCertsDefaultCert, routerCertsDefaultCertErr := getCertificateInfoFromSecret(ctx, coreClient, "openshift-ingress", "router-certs-default")
if routerCertsDefaultCertErr != nil {
errs = append(errs, routerCertsDefaultCertErr)
} else {
certificates = append(certificates, routerCertsDefaultCert)
}

// Step 2: List all Ingress Controllers
for _, namespace := range ingressNamespaces {
controllers, err := operatorClient.OperatorV1().IngressControllers(namespace).List(ctx, metav1.ListOptions{})
if errors.IsNotFound(err) {
klog.V(2).Infof("Ingress Controllers not found in '%s' namespace", namespace)
continue
}
if err != nil {
errs = append(errs, err)
continue
}

// Step 3: Filter Ingress Controllers with spec.defaultCertificate and get certificate info
for _, controller := range controllers.Items {

// Step 4: Check the certificate limits
if len(certificates) >= ingressCertificatesLimits {
klog.V(2).Infof("Reached the limit of ingress certificates (%d), skipping additional certificates", ingressCertificatesLimits)
break
}

if controller.Spec.DefaultCertificate != nil {
certName := controller.Spec.DefaultCertificate.Name
rluders marked this conversation as resolved.
Show resolved Hide resolved
certInfo, certErr := getCertificateInfoFromSecret(ctx, coreClient, namespace, certName)
if certErr != nil {
errs = append(errs, certErr)
continue
}

// Step 5: Add certificate info to the certificates list
found := false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to use a map for storing the ControllerInfo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not remember it all here, but I get the certificates and relate them with the Controllers because sometimes (yes it is weird). Still, the certificate and the controllers are living in separated namespaces. I didn't find a way to identify it using the API, so the best approach I found was to get all the certificates and them, check if any controller is using them. Weird? Yes.

for _, cert := range certificates {
// I need to compare both, since a certificate seems to be able to live outside the controller's namespace
if cert.Name == certInfo.Name && cert.Namespace == certInfo.Namespace {
// Certificate already exists, add the controller to its list
cert.Controllers = append(cert.Controllers, ControllerInfo{Name: controller.Name, Namespace: controller.Namespace})
found = true
break
}
}

if !found {
// Certificate not found, create a new entry
certInfo.Controllers = append(certInfo.Controllers, ControllerInfo{Name: controller.Name, Namespace: controller.Namespace})
certificates = append(certificates, certInfo)
}
}
}
}

var records []record.Record
if len(certificates) > 0 {
// Step 6: Generate the certificates record
records = append(records, record.Record{
Name: "config/ingress/certificates",
Item: record.JSONMarshaller{Object: certificates},
})
}

return records, nil
}

func getCertificateInfoFromSecret(ctx context.Context, coreClient corev1client.CoreV1Interface, namespace, secretName string) (*CertificateInfo, error) {
secret, err := coreClient.Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("failed to get secret '%s' in namespace '%s': %v", secretName, namespace, err)
}

crtData, found := secret.Data["tls.crt"]
if !found {
return nil, fmt.Errorf("'tls.crt' not found in secret '%s' in namespace '%s'", secretName, namespace)
}

block, _ := pem.Decode(crtData)
if block == nil {
return nil, fmt.Errorf("unable to decode certificate (x509) from secret '%s' in namespace '%s'", secretName, namespace)
}

cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return nil, fmt.Errorf("failed to parse certificate from secret '%s' in namespace '%s': %v", secretName, namespace, err)
}

return &CertificateInfo{
Name: secretName,
Namespace: namespace,
NotBefore: metav1.NewTime(cert.NotBefore),
NotAfter: metav1.NewTime(cert.NotAfter),
Controllers: []ControllerInfo{},
}, nil
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package clusterconfig

import (
"context"
"testing"
"time"

"k8s.io/apimachinery/pkg/runtime"

configv1 "github.com/openshift/api/config/v1"
configfake "github.com/openshift/client-go/config/clientset/versioned/fake"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
corefake "k8s.io/client-go/kubernetes/fake"

"github.com/openshift/insights-operator/pkg/record"
"github.com/stretchr/testify/assert"
)

func Test_gatherClusterIngressCertificates(t *testing.T) {
certData := `
-----BEGIN CERTIFICATE-----
MIIDazCCAlOgAwIBAgIUfTstqHMAhGLL+j3n6pmwLw8vt84wDQYJKoZIhvcNAQEL
BQAwRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM
GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAeFw0yNDAzMDYxMjU5MTFaFw0yNTAz
MDYxMjU5MTFaMEUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEw
HwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQwggEiMA0GCSqGSIb3DQEB
AQUAA4IBDwAwggEKAoIBAQDoqkPZMeMi5qjkG384ZwpAc3QScOGYBWOEDFAioq5C
YhtDGBSMq2VwS0r8RvEEhbebvXuH5PLcIuEVO/MZRQD9gSacCfLlMLRKZYpv168m
KUYyhx1bXKmUlbQxCnpAPZ7nf14A3Pb0TzLfsKjoUdUOv/1eorA6+oU78StWx/Nt
W94ad9n3o+cjiMPu/RS3g9b+x07bG5mFYuzpWk/Svb5Lb42g8AtonzqFJBbhlStU
A+9UyzmyXMeTlbI9fFmku7mb5Uq0SZ8jhpH+fyCoQOxefTfVrvjrkkdavDn43hjz
5hCwGJ3mV96MU9hh398oBguOHaJ6V3/UHtW1spsFY83RAgMBAAGjUzBRMB0GA1Ud
DgQWBBR/zvuHjFadvifzAGBHegOxmRXnCzAfBgNVHSMEGDAWgBR/zvuHjFadvifz
AGBHegOxmRXnCzAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQBs
E2U+jQzJuEt9e6UEnS1T0cb2NxaGb7CYsSX0TjZK1VgloAKbnxaCLjRruTOOwfm6
s5CFzFjJoIhUASzoA295Np2AR0UEYr5fendIjKCztCMlpj0fp92jFL6/RZWNGM1A
qECHYtZckeqJjg9vUdfHtiBRoyEHJUJ/tsDDlslwzocdJUqKL8V6KerZsh5SIAkS
rJ8EgVyDvwQaLPQMttjk62croI1Wi3FLmkvvtTbNcMgTnVhFfGjyHOiGnQeBfqax
5P0VBuAUCihegskKEUCJB8HFPkC4hqbrEk0+psQ2Gm8kjoll/SpltFLS77Xjhrz9
1qaiDHuWnUSifz6SGpWr
-----END CERTIFICATE-----
`

tests := []struct {
name string
ingressDefinitions []configv1.Ingress
secretData map[string][]byte
wantRecords []record.Record
wantErrCount int
}{
{
name: "successful retrieval cluster ingress certificates",
ingressDefinitions: []configv1.Ingress{
{ObjectMeta: metav1.ObjectMeta{Name: "example-ingress", Namespace: "openshift-ingress-operator"}},
},
secretData: map[string][]byte{
"tls.crt": []byte(certData),
},
wantRecords: []record.Record{
{
Name: "config/ingress/openshift-ingress-operator/ingress_certificates.json",
Item: record.JSONMarshaller{
Object: []IngressControllerInfo{
{
Name: "example-ingress",
OperatorGeneratedCertificate: []IngressCertificateInfo{
{
Name: "router-ca",
NotBefore: time.Date(2024, time.March, 6, 12, 59, 11, 0, time.UTC),
NotAfter: time.Date(2025, time.March, 6, 12, 59, 11, 0, time.UTC),
},
},
CustomCertificates: []IngressCertificateInfo{},
},
},
},
},
},
wantErrCount: 0,
},
{
name: "failed retrieval cluster ingress certificates",
ingressDefinitions: []configv1.Ingress{
{
ObjectMeta: metav1.ObjectMeta{
Name: "example-ingress",
},
Spec: configv1.IngressSpec{},
},
},
secretData: map[string][]byte{},
wantRecords: nil,
wantErrCount: 1, // There should be an error due to missing 'tls.crt'
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
objects := make([]runtime.Object, len(tt.ingressDefinitions))
for i, ingress := range tt.ingressDefinitions {
objects[i] = &ingress
}
configClient := configfake.NewSimpleClientset(objects...)
coreClient := corefake.NewSimpleClientset(&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Name: "router-ca", Namespace: "openshift-ingress-operator"},
Data: tt.secretData,
})

records, errs := gatherClusterIngressCertificates(context.TODO(), coreClient.CoreV1(), configClient.ConfigV1())
assert.Equal(t, tt.wantRecords, records)
assert.Len(t, errs, tt.wantErrCount)
})
}
}