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.

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

chore: bump externaldata v1beta1 api #2438

Merged
merged 32 commits into from
Dec 15, 2022

Conversation

sozercan
Copy link
Member

@sozercan sozercan commented Dec 7, 2022

Signed-off-by: Alex Pana 8968914+acpana@users.noreply.github.comWhat this PR does / why we need it:

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 #2434 #2454

Special notes for your reviewer:
vendoring in open-policy-agent/frameworks#270

based on #2436, will rebase once it merges

acpana and others added 5 commits December 7, 2022 18:07
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan sozercan changed the title chore: bump externaldata v1vbeta1 api chore: bump externaldata v1beta1 api Dec 7, 2022
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Base: 53.68% // Head: 53.63% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (95dbd65) compared to base (0fdd27e).
Patch coverage: 73.91% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2438      +/-   ##
==========================================
- Coverage   53.68%   53.63%   -0.05%     
==========================================
  Files         117      117              
  Lines       10292    10303      +11     
==========================================
+ Hits         5525     5526       +1     
- Misses       4346     4353       +7     
- Partials      421      424       +3     
Flag Coverage Δ
unittests 53.63% <73.91%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/webhook/common.go 65.21% <0.00%> (ø)
...controller/externaldata/externaldata_controller.go 57.53% <70.58%> (+1.08%) ⬆️
pkg/mutation/system_external_data.go 87.58% <100.00%> (ø)
pkg/readiness/ready_tracker.go 69.83% <100.00%> (ø)
pkg/webhook/policy.go 40.13% <100.00%> (ø)
pkg/readiness/list.go 79.41% <0.00%> (-11.77%) ⬇️
...onstrainttemplate/constrainttemplate_controller.go 56.69% <0.00%> (-0.48%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sozercan sozercan force-pushed the externaldata-v1beta1 branch 3 times, most recently from ed31f16 to ded8deb Compare December 8, 2022 00:10
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: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan sozercan marked this pull request as ready for review December 9, 2022 00:07
@sozercan
Copy link
Member Author

sozercan commented Dec 9, 2022

@maxsmythe still working on helm upgrade test but rest is ready for review

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM assuming the helm upgrade test doesn't require any novel code

pkg/externaldata/externaldata.go Show resolved Hide resolved
@sozercan sozercan force-pushed the externaldata-v1beta1 branch 7 times, most recently from 9626eb5 to 1684a27 Compare December 10, 2022 02:16
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan
Copy link
Member Author

sozercan commented Dec 12, 2022

@maxsmythe Fixed the issue with the helm upgrade. However, we had to bump the helm upgrade base version from 3.5 to 3.9 and we have to have external data enabled. Otherwise, GK certificate does not get generated with clientAuth which is passed to cert-controller

keyUsages = append(keyUsages, x509.ExtKeyUsageClientAuth)
This means a user cannot upgrade from non-external data or pre-3.9 external data to external data with mTLS (TLS should be fine) using GK generated certificates. They'll have to use their own or regenerate the certs.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@ritazh
Copy link
Member

ritazh commented Dec 12, 2022

This means a user cannot upgrade from non-external data or pre-3.9 external data to external data with mTLS (TLS should be fine) using GK generated certificates. They'll have to use their own or regenerate the certs.

Let's make sure to add this to the external data mtls doc (maybe a followup PR).

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

LGTM

@sozercan sozercan merged commit 2fd0473 into open-policy-agent:master Dec 15, 2022
@sozercan sozercan deleted the externaldata-v1beta1 branch December 15, 2022 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

graduate external data resources to v1beta1
5 participants