-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-1087: Added fields for ca certificate configuration #379
Conversation
@OlivierCazade: This pull request references NETOBSERV-1087 which is a valid jira issue. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
fc240a8
to
e94358a
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #379 +/- ##
==========================================
+ Coverage 53.67% 55.66% +1.98%
==========================================
Files 44 46 +2
Lines 5559 5954 +395
==========================================
+ Hits 2984 3314 +330
- Misses 2359 2412 +53
- Partials 216 228 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
if b.desired.Processor.Metrics.Server.TLS.Provided.CaFile != "" { | ||
if b.desired.Processor.Metrics.Server.TLS.Provided.Type == flowslatest.RefTypeConfigMap { | ||
flpServiceMonitorObject.Spec.Endpoints[0].TLSConfig.SafeTLSConfig.CA = monitoringv1.SecretOrConfigMap{ | ||
ConfigMap: &corev1.ConfigMapKeySelector{ | ||
LocalObjectReference: corev1.LocalObjectReference{ | ||
Name: b.desired.Processor.Metrics.Server.TLS.Provided.Name, | ||
}, | ||
Key: b.desired.Processor.Metrics.Server.TLS.Provided.CaFile, | ||
}, | ||
} | ||
} else if b.desired.Processor.Metrics.Server.TLS.Provided.Type == flowslatest.RefTypeSecret { | ||
flpServiceMonitorObject.Spec.Endpoints[0].TLSConfig.SafeTLSConfig.CA = monitoringv1.SecretOrConfigMap{ | ||
Secret: &corev1.SecretKeySelector{ | ||
LocalObjectReference: corev1.LocalObjectReference{ | ||
Name: b.desired.Processor.Metrics.Server.TLS.Provided.Name, | ||
}, | ||
Key: b.desired.Processor.Metrics.Server.TLS.Provided.CaFile, | ||
}, | ||
} | ||
} | ||
} |
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.
Do we need to provide ConfigMap or Secret if InsecureSkipVerify
is true ?
I would suggest to create an helper to check both b.desired.Processor.Metrics.Server.TLS.Provided.Type
and b.desired.Processor.Metrics.Server.TLS.Provided.InsecureSkipVerify
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.
Done, thanks!
api/v1beta1/flowcollector_types.go
Outdated
@@ -678,6 +678,16 @@ type CertificateReference struct { | |||
// certKey defines the path to the certificate private key file name within the config map or secret. Omit when the key is not necessary. | |||
// +optional | |||
CertKey string `json:"certKey,omitempty"` | |||
|
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.
These added fields are confusing when CertificateReference
is used from ClientTLS
, as it also contains a reference for caCert and insecureSkipVerify.
I think we need an intermediate struct between ServerTLS
and CertificateReference
, and move these fields in that new struct
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.
I moved the field directly to the ServerTLS.
1634e2f
to
9c04347
Compare
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:0c381db make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-0c381db Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-0c381db
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:a7d07d9 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-a7d07d9 Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-a7d07d9
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
// // This function need to be manually created because conversion-gen not able to create it intentionally because | ||
// // we have new defined fields in v1beta1 not in v1alpha1 | ||
// // nolint:golint,stylecheck,revive | ||
// func Convert_v1beta1_CertificateReference_To_v1alpha1_CertificateReference(in *v1beta1.CertificateReference, out *CertificateReference, s apiconversion.Scope) error { |
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.
nit: looks like some garbage commented out?
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.
Yes, removed, thank you!
/lgtm |
I'm merging for the branch cut, so this must be verified post-merge |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.