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.

Sign up for GitHub

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

Jump to bottom

LOG-3569: Add functionality to enable TLS Security feature gate #1887

Merged
merged 1 commit into from
Mar 14, 2023
Merged

LOG-3569: Add functionality to enable TLS Security feature gate #1887

merged 1 commit into from
Mar 14, 2023

Conversation

jcantrill
Copy link
Contributor

Description

This PR:

  • Adds functionality to enable a preview feature gate for TLS Security Profile
  • Adds an option for consumption by the collector config generator

Links

cc @syedriko

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 23, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 23, 2023 •
edited by openshift-ci bot

@jcantrill: This pull request references LOG-3569 which is a valid jira issue.

In response to this:

Description

This PR:

  • Adds functionality to enable a preview feature gate for TLS Security Profile
  • Adds an option for consumption by the collector config generator

Links

cc @syedriko

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.

@jcantrill jcantrill added release/5.7 and removed jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Feb 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2023
@jcantrill
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2023
@jcantrill
Copy link
Contributor Author

/hold

syedriko
syedriko reviewed Feb 27, 2023
View reviewed changes
internal/k8shandler/forwarding_features_test.go Outdated
Entry("enables TLS Security profile for eEabled", constants.PreviewTLSSecurityProfile, "", constants.PreviewTLSSecurityProfile, "eNabled"),
Entry("disables TLS Security profile for true", "", "", constants.PreviewTLSSecurityProfile, "true"),
Entry("enables old remote syslog for enabled", UseOldRemoteSyslogPlugin, "", UseOldRemoteSyslogPlugin, "enabled"),
Entry("enables old remote syslog for eEabled", UseOldRemoteSyslogPlugin, "", UseOldRemoteSyslogPlugin, "eNabled"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcantrill eEabled sp. here and a few lines above

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023
@jcantrill jcantrill force-pushed the log3569 branch 2 times, most recently from 7ff612e to 9d299f4 Compare March 7, 2023 16:38
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2023
@jcantrill jcantrill force-pushed the log3569 branch 2 times, most recently from e0134b1 to aa2dcb5 Compare March 8, 2023 13:53
@jcantrill
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023
@jcantrill jcantrill mentioned this pull request Mar 8, 2023
Merged
vparfonov
vparfonov reviewed Mar 9, 2023
View reviewed changes
internal/k8shandler/forwarding.go
}
for key, value := range forwarder.Annotations {
switch key {
case constants.PreviewTLSSecurityProfile:
Copy link
Contributor

@vparfonov vparfonov Mar 9, 2023 •
edited

Choose a reason for hiding this comment

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

nit: Do nothing here, maybe just remove this case

Copy link
Contributor

@syedriko syedriko Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

It is doing for constants.PreviewTLSSecurityProfile the same thing as for constants.UseOldRemoteSyslogPlugin. Which is the same as

switch key {
case constants.PreviewTLSSecurityProfile, constants.UseOldRemoteSyslogPlugin:
...

syedriko
syedriko reviewed Mar 10, 2023
View reviewed changes
internal/generator/options.go Outdated

// Has takes a key and returns true if it exists
func (o Options) Has(key string) bool {
if _, found := o[key]; found {
Copy link
Contributor

Choose a reason for hiding this comment

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

_, found := o[key]
return found

syedriko
syedriko reviewed Mar 10, 2023
View reviewed changes
internal/generator/options_test.go Outdated
var (
options = Options{}
)
It("should should be false when the key does not exist", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should

syedriko
syedriko reviewed Mar 10, 2023
View reviewed changes
internal/generator/options_test.go Outdated
Expect(options.Has("foo")).To(BeFalse())
})

It("should should be true when the key exists", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should

syedriko
syedriko reviewed Mar 10, 2023
View reviewed changes
internal/k8shandler/forwarding.go
if clusterRequest.ForwarderRequest == nil {
return false
// EvaluateAnnotationsForEnabledCapabilities populates generator options with capabilities enabled by the ClusterLogForwarder
func EvaluateAnnotationsForEnabledCapabilities(forwarder *logging.ClusterLogForwarder, options generator.Options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it doesn't look like EvaluateAnnotationsForEnabledCapabilities needs to be exported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test to require it to be exported 😉

syedriko
syedriko reviewed Mar 10, 2023
View reviewed changes
internal/k8shandler/forwarding_features_test.go
})
DescribeTable("when forwarder is not nil", func(enabledOption, value string, annotations ...string) {
pairs := annotations
if len(pairs)%2 != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just name annotations pairs and drop

pairs := annotations

?

@syedriko
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 14, 2023 •
edited

@jcantrill: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/functional-target 0e56ba4 link false /test functional-target
ci/prow/e2e-ocp-target-minus-one 0e56ba4 link false /test e2e-ocp-target-minus-one

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f366041 and 2 for PR HEAD 0e56ba4 in total

@openshift-merge-robot openshift-merge-robot merged commit 10599af into openshift:master Mar 14, 2023
@jcantrill jcantrill deleted the log3569 branch March 14, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vparfonov vparfonov vparfonov left review comments

@syedriko syedriko syedriko left review comments

@vimalk78 vimalk78 Awaiting requested review from vimalk78

Assignees

@syedriko syedriko

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release/5.7
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jcantrill @openshift-ci-robot @syedriko @vparfonov @openshift-merge-robot