-
Notifications
You must be signed in to change notification settings - Fork 95
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
OCPBUGS-35727: Ingress controller related certificates' validate dates gathering #945
Merged
openshift-merge-bot
merged 20 commits into
openshift:master
from
ncaak:CCXDEV-12503/ingress-related-certs
Jun 18, 2024
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
a581135
feat: ingress certificates gather
6fda77b
refactor: invert ingress certificate logic
4768a65
refactor: better data serialization
6f47871
refactor: adjust cert check
6b10f12
chore: remove deprecated linters
4f5bcb9
fix: remove hardcored test data
b8cf797
fix: missing controller for new certs
bc6cc1a
Minor refactor filename and time format
ncaak cad48d0
(draft) Add new unit test for getCertificateInfoFromSecret func
ncaak d215a33
Refactor unit tests for gatherClusterIngressCertificates func
ncaak 89145e9
Add new unit test for custom certificate and minor refactor
ncaak 0251087
Add new unit test for failure in cluster certs and errors control
ncaak 814cf26
Refactor cert match logic to a map and change on signature to improve…
ncaak a7bfdcf
Fix unit tests to adapt better to gather function return
ncaak 59732b6
Fix linting
ncaak 70bec41
Add documentation for ClusterIngressCertificates gatherer
ncaak c722441
Add documentation for ClusterIngressCertificates gatherer
ncaak 0c0ab88
Fix logline moved to error trigger
ncaak fa50916
Rollback golangci configuration
ncaak 82311f7
Refactor function naming and minor slice accesses
ncaak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 21 additions & 0 deletions
21
docs/insights-archive-sample/aggregated/ingress_controllers_certs.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
[ | ||
{ | ||
"name": "router-ca", | ||
"namespace": "openshift-ingress-operator", | ||
"not_before": "2024-06-07T07:27:34Z", | ||
"not_after": "2026-06-07T07:27:35Z", | ||
"controllers": [ | ||
{ | ||
"name": "test-ingresscontroller-name", | ||
"namespace": "openshift-ingress-operator" | ||
} | ||
] | ||
}, | ||
{ | ||
"name": "router-certs-default", | ||
"namespace": "openshift-ingress", | ||
"not_before": "2024-06-07T07:27:35Z", | ||
"not_after": "2026-06-07T07:27:36Z", | ||
"controllers": [] | ||
} | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
225 changes: 225 additions & 0 deletions
225
pkg/gatherers/clusterconfig/gather_cluster_ingress_certificates.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
package clusterconfig | ||
|
||
import ( | ||
"context" | ||
"crypto/x509" | ||
"encoding/pem" | ||
"fmt" | ||
"time" | ||
|
||
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 | ||
|
||
type CertificateInfo struct { | ||
Name string `json:"name"` | ||
Namespace string `json:"namespace"` | ||
NotBefore time.Time `json:"not_before"` | ||
NotAfter time.Time `json:"not_after"` | ||
Controllers []ControllerInfo `json:"controllers"` | ||
} | ||
|
||
type ControllerInfo struct { | ||
Name string `json:"name"` | ||
Namespace string `json:"namespace"` | ||
} | ||
|
||
type CertificateInfoKey struct { | ||
Name string `json:"name"` | ||
Namespace string `json:"namespace"` | ||
} | ||
|
||
// GatherClusterIngressCertificates Collects the certificate's NotBefore and NotAfter dates from the cluster's ingress controller certificates. | ||
// It also collects the name and namespace of any Ingress Controllers using the certificates. | ||
// | ||
// ### API Reference | ||
// - https://docs.openshift.com/container-platform/4.13/rest_api/operator_apis/ingresscontroller-operator-openshift-io-v1.html | ||
// - https://docs.openshift.com/container-platform/4.13/rest_api/security_apis/secret-v1.html | ||
// | ||
// ### Sample data | ||
// - docs/insights-archive-sample/aggregated/ingress_controllers_certs.json | ||
// | ||
// ### Location in archive | ||
// - `aggregated/ingress_controllers_certs.json` | ||
// | ||
// ### Config ID | ||
// `clusterconfig/ingress_certificates` | ||
// | ||
// ### Released version | ||
// - 4.17 | ||
// | ||
// ### Backported versions | ||
// TBD | ||
// | ||
// ### Changes | ||
// None | ||
func (g *Gatherer) GatherClusterIngressCertificates(ctx context.Context) ([]record.Record, []error) { | ||
const Filename = "aggregated/ingress_controllers_certs" | ||
|
||
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} | ||
} | ||
|
||
certificates, errs := gatherClusterIngressCertificates(ctx, gatherKubeClient.CoreV1(), operatorClient) | ||
|
||
return []record.Record{{ | ||
Name: Filename, | ||
Item: record.JSONMarshaller{Object: certificates}, | ||
}}, errs | ||
} | ||
|
||
func gatherClusterIngressCertificates( | ||
ctx context.Context, | ||
coreClient corev1client.CoreV1Interface, | ||
operatorClient operatorclient.Interface) ([]*CertificateInfo, []error) { | ||
// | ||
var ingressAllowedNS = [2]string{"openshift-ingress-operator", "openshift-ingress"} | ||
|
||
var certificatesInfo = make(map[CertificateInfoKey]*CertificateInfo) | ||
var errs []error | ||
|
||
// Step 1: Collect router-ca and router-certs-default | ||
rCAinfo, err := getRouterCACertInfo(ctx, coreClient) | ||
if err != nil { | ||
errs = append(errs, err) | ||
} | ||
if rCAinfo != nil { | ||
certificatesInfo[newCertificateInfoKey(rCAinfo)] = rCAinfo | ||
} | ||
|
||
rCDinfo, err := getRouterCertsDefaultCertInfo(ctx, coreClient) | ||
if err != nil { | ||
errs = append(errs, err) | ||
} | ||
if rCDinfo != nil { | ||
certificatesInfo[newCertificateInfoKey(rCDinfo)] = rCDinfo | ||
} | ||
|
||
// Step 2: List all Ingress Controllers | ||
for _, namespace := range ingressAllowedNS { | ||
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 i := range controllers.Items { | ||
controller := controllers.Items[i] | ||
|
||
// Step 4: Check the certificate limits | ||
if len(certificatesInfo) >= ingressCertificatesLimits { | ||
errs = append(errs, fmt.Errorf( | ||
"reached the limit of ingress certificates (%d), skipping additional certificates", | ||
ingressCertificatesLimits)) | ||
break | ||
} | ||
|
||
if controller.Spec.DefaultCertificate != nil { | ||
secretName := controller.Spec.DefaultCertificate.Name | ||
certInfo, certErr := getCertificateInfoFromSecret(ctx, coreClient, namespace, secretName) | ||
if certErr != nil { | ||
errs = append(errs, certErr) | ||
continue | ||
} | ||
|
||
// Step 5: Add certificate info to the certificates list | ||
c, exists := certificatesInfo[newCertificateInfoKey(certInfo)] | ||
if exists { | ||
c.Controllers = append(c.Controllers, | ||
ControllerInfo{Name: controller.Name, Namespace: controller.Namespace}, | ||
) | ||
// | ||
} else { | ||
certInfo.Controllers = append(certInfo.Controllers, | ||
ControllerInfo{Name: controller.Name, Namespace: controller.Namespace}, | ||
) | ||
certificatesInfo[newCertificateInfoKey(certInfo)] = certInfo | ||
} | ||
} | ||
} | ||
} | ||
|
||
var ci []*CertificateInfo | ||
if len(certificatesInfo) > 0 { | ||
// Step 6: Generate the certificates record | ||
for _, v := range certificatesInfo { | ||
ci = append(ci, v) | ||
} | ||
} | ||
|
||
return ci, errs | ||
} | ||
|
||
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: cert.NotBefore, | ||
NotAfter: cert.NotAfter, | ||
Controllers: []ControllerInfo{}, | ||
}, nil | ||
} | ||
|
||
func getRouterCACertInfo(ctx context.Context, coreClient corev1client.CoreV1Interface) (*CertificateInfo, error) { | ||
const ( | ||
routerCASecret string = "router-ca" | ||
routerCANamespace string = "openshift-ingress-operator" | ||
) | ||
return getCertificateInfoFromSecret(ctx, coreClient, routerCANamespace, routerCASecret) | ||
} | ||
|
||
func getRouterCertsDefaultCertInfo(ctx context.Context, coreClient corev1client.CoreV1Interface) (*CertificateInfo, error) { | ||
const ( | ||
routerCertsSecret string = "router-certs-default" | ||
routerCertsNamespace string = "openshift-ingress" | ||
) | ||
return getCertificateInfoFromSecret(ctx, coreClient, routerCertsNamespace, routerCertsSecret) | ||
} | ||
|
||
func newCertificateInfoKey(info *CertificateInfo) CertificateInfoKey { | ||
return CertificateInfoKey{Name: info.Name, Namespace: info.Namespace} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not necessary, but let's not run the CI again :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aw yeah... After the refactor of the inner code the conditional is of no use :(
I'll leave it marked in my local repo for the next PR to include it as an improvement. Thanks.