-
Notifications
You must be signed in to change notification settings - Fork 737
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
Set minimum TLS version in webhooks #1426
Conversation
if err != nil { | ||
return nil, err | ||
} | ||
selectorsByGVK[gvk] = selector |
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.
@shomron I am getting error
cannot use selector (variable of type internal.Selector) as internal.Selector value in assignmentcompilerIncompatibleAssign
any ideas?
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, the types come from different packages (upstream cache and forked cache) - and can't just be assigned to each other. This resolves it:
selectorsByGVK[gvk] = internal.Selector{
Label: selector.Label,
Field: selector.Field,
}
I'll go through the PR to see if there's anything else.
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.
Opened sozercan#73 with fixes to the rebase.
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.
thanks @shomron!
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.
LGTM
21c266c
to
7d3dfa7
Compare
@shomron when you get a chance, can you please help take a look at these failures: https://github.com/open-policy-agent/gatekeeper/pull/1426/checks?check_run_id=3550979914 |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Oren Shomron <shomron@gmail.com>
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
7d3dfa7
to
8d4b631
Compare
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Looks like #1538 updated the constraint framework vendor without go.mod change. I updated frameworks to |
https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/config/config_controller_test.go#L95 |
@maxsmythe still LGTY? |
Codecov Report
@@ Coverage Diff @@
## master #1426 +/- ##
==========================================
+ Coverage 52.77% 52.82% +0.04%
==========================================
Files 89 89
Lines 7843 7849 +6
==========================================
+ Hits 4139 4146 +7
- Misses 3356 3357 +1
+ Partials 348 346 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
7e89a28
to
178b70e
Compare
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
178b70e
to
234c33b
Compare
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.
One comment about metrics
WRT the informer, are we also upgrading our fork as part of this PR?
@@ -31,4 +32,4 @@ func DisableRESTClientMetrics() { | |||
|
|||
type noopLatency struct{} | |||
|
|||
func (noopLatency) Observe(string, url.URL, time.Duration) {} | |||
func (noopLatency) Observe(context.Context, string, url.URL, time.Duration) {} |
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.
Why are we adding context
here? Looking at the definition of clientmetrics.RequestLatency, it doesn't appear to expect a context.
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.
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.
ah, might have been looking at an older version of the code. LGTM
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.
LGTM
Signed-off-by: Sertac Ozercan sozercan@gmail.com
What this PR does / why we need it:
tls-min-version
flag to set min TLS version (defaulting to 1.2)Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #1424
Special notes for your reviewer: