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

Fix hmac secret #2768

Merged
merged 6 commits into from
Mar 5, 2024
Merged

Fix hmac secret #2768

merged 6 commits into from
Mar 5, 2024

Conversation

zhaohuabing
Copy link
Member

This PR does the following:

  • Generates an HMAC secret and stores it in a K8s secret in the CertGen job.
  • Watches this k8s secret in the Kuberetes provider.
  • Uses the HMAC generated in the above step for envoy ouath2 filter configuration.

Signed-off-by: huabing zhao <[email protected]>
@zhaohuabing zhaohuabing requested a review from a team as a code owner March 4, 2024 09:47
@zhaohuabing zhaohuabing requested a review from arkodg March 4, 2024 09:48
@@ -153,6 +154,11 @@ func GenerateCerts(cfg *config.Server) (*Certificates, error) {
return nil, err
}

oidcHMACSecret, err := generateHMACSecret()
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens during upgrade if this secret already exists ? do we skip or overwrite ?

Copy link
Contributor

Choose a reason for hiding this comment

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

relates to

func CreateOrUpdateSecrets(ctx context.Context, client client.Client, secrets []corev1.Secret, update bool) ([]corev1.Secret, error) {

Copy link
Member Author

Choose a reason for hiding this comment

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

The creation of HMAC secret will be skipped in CreateOrUpdateSecrets if it's already there, like the other certs. No changes are needed.

@arkodg
Copy link
Contributor

arkodg commented Mar 4, 2024

hey @zhaohuabing has this been tested ? are you sure we are reconciling this secret ? I think we may need some more code in

func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Request) (reconcile.Result, error) {
to make sure this secret is part of the resources

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Mar 5, 2024

hey @zhaohuabing has this been tested ? are you sure we are reconciling this secret ?

I haven't tested it manually yet. It should be covered by the OIDC e2e test: #2730

I think we may need some more code in

func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Request) (reconcile.Result, error) {

to make sure this secret is part of the resources

Yeah, I missed that :-(. It's fixed.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 61.17647% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 63.46%. Comparing base (f1ac586) to head (5b3f3f6).

Files Patch % Lines
internal/gatewayapi/securitypolicy.go 50.00% 5 Missing and 2 partials ⚠️
internal/provider/kubernetes/secrets.go 0.00% 7 Missing ⚠️
internal/crypto/certgen.go 66.66% 4 Missing and 2 partials ⚠️
internal/ir/zz_generated.deepcopy.go 0.00% 5 Missing ⚠️
internal/provider/kubernetes/predicates.go 66.66% 2 Missing and 1 partial ⚠️
internal/provider/kubernetes/controller.go 92.30% 2 Missing ⚠️
internal/gatewayapi/runner/runner.go 0.00% 1 Missing ⚠️
internal/ir/xds.go 0.00% 1 Missing ⚠️
internal/xds/translator/oidc.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2768      +/-   ##
==========================================
+ Coverage   63.45%   63.46%   +0.01%     
==========================================
  Files         125      125              
  Lines       20544    20604      +60     
==========================================
+ Hits        13036    13077      +41     
- Misses       6672     6690      +18     
- Partials      836      837       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: huabing zhao <[email protected]>
@zhaohuabing zhaohuabing self-assigned this Mar 5, 2024
@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 5, 2024

/retest

1 similar comment
@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 5, 2024

/retest

@zhaohuabing zhaohuabing requested a review from arkodg March 5, 2024 04:01
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

🚀

@zhaohuabing zhaohuabing merged commit 5ecbdcd into envoyproxy:main Mar 5, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants