LOG-8109 & LOG-8091 & NetworkPolicy refactor #3159
Conversation
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| port = parsedPort | ||
| // Panic on non-standard schemes without a port | ||
| } else if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| panic(fmt.Sprintf("non-standard scheme %q requires an explicit port in URL %q", parsedURL.Scheme, urlStr)) |
There was a problem hiding this comment.
This would mean we allowed an 'address:port' without the port? The offenders I can think of our the socket level outputs like kafka or syslog. Admins need to specify the port for even forwarding to work but my concern is we do not have validation at admission that would catch a missing port and panic for the operator looks agressive.
I think 'panic' is the right approach if we are certain we have plenty of safeguards upstream of this call. Are we able to confirm that? Can we add validations (declarative) if we do not? I know we have isURL in place but I do not believe that will catch a requirement of 'address:port'
There was a problem hiding this comment.
That is correct. We do allow non standard schemes without a port as the validations do that check for that. We will need to add validation for these URLs. I am fine either way not panicking if we can validate all URLs.
There was a problem hiding this comment.
We do allow non standard schemes without a port as the validations do that check for that
We do...and validations check(and fail)... if ⬅️ is the current behavior then let's leave the panic in
There was a problem hiding this comment.
My mistake on that comment, we DO NOT check for that. isURL() only checks if it is a proper url even if not port is specified.
For something like Kafka, validation only ensures the URL starts with tcp or tls but doesn't check for a port. We DON'T have a validation for ensuring ports exist for those URLs.
I believe we should add in validations for non standard schemes to ensure they also include a port. Then we can keep this panic.
There was a problem hiding this comment.
We should be able to add regex into the declarative validations to enforce port specification. Please either add this to this PR or open a new JIRA so we capture the work for later.
|
/retest |
|
/retest |
|
@Clee2691: all tests passed! 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-sigs/prow repository. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Clee2691, 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 |
openshift-ci
bot
added
the
approved
2 participants
Description
This PR refactors the
NetworkPolicyfeature to improve port handling and configuration robustness, which also resolves the following issues: LOG-8109 and LOG-8091.Changes:
80/443) forHTTP/HTTPSURLs if no port is specified.UDP,TLS,TCP) are used without an explicit port.Kafkabrokers and URLs.proxyURLforHTTPoutputs.HTTPSport443by default forGCL,AzureHTTPSport443by default forCloudwatch, andS3outputs when a URL is undefined.This PR will supersede: #3152.
/cc @cahartma @vparfonov
/assign @jcantrill
Links