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

HIVE-2687: Privatelink improvements #2528

Open

jstuever wants to merge 4 commits into openshift:master from jstuever:HIVE-2687

Contributor

jstuever commented Dec 13, 2024

No description provided.

openshift-ci-robot added the jira/valid-reference label

openshift-ci bot added the do-not-merge/work-in-progress label

openshift-ci-robot commented Dec 13, 2024 •

edited by openshift-ci bot

Loading

@jstuever: This pull request references HIVE-2687 which is a valid jira issue.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci bot requested review from 2uasimojo and dlom

December 13, 2024 20:58

Contributor

openshift-ci bot commented Dec 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever

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:

~~OWNERS~~ [jstuever]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved label

jstuever force-pushed the HIVE-2687 branch 2 times, most recently from 97cc525 to 7bd9cd8 Compare

December 13, 2024 21:04

jstuever mentioned this pull request

HIVE-2525: privatelink: add privatelink controller unit tests #2520

Merged

jstuever force-pushed the HIVE-2687 branch 2 times, most recently from f5afd12 to 20cdfee Compare

December 16, 2024 21:53

codecov bot commented Dec 16, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 85.45455% with 16 lines in your changes missing coverage. Please review.

Project coverage is 49.90%. Comparing base (553c6c2) to head (eab4e06).
Report is 60 commits behind head on master.

Files with missing lines	Patch %	Lines
...privatelink/actuator/awsactuator/awshubactuator.go	82.60%	13 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
+ Coverage   49.78%   49.90%   +0.12%     
==========================================
  Files         281      281              
  Lines       33027    33127     +100     
==========================================
+ Hits        16442    16532      +90     
- Misses      15252    15259       +7     
- Partials     1333     1336       +3

Files with missing lines	Coverage Δ
...rivatelink/actuator/gcpactuator/gcplinkactuator.go	`85.75% <100.00%> (+0.40%)`	⬆️
...kg/controller/privatelink/conditions/conditions.go	`100.00% <100.00%> (ø)`
...privatelink/actuator/awsactuator/awshubactuator.go	`88.67% <82.60%> (-0.16%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

openshift-merge-robot added the needs-rebase label

jstuever force-pushed the HIVE-2687 branch from b90104b to ae0290e Compare

January 13, 2025 19:08

openshift-merge-robot removed the needs-rebase label

Contributor Author

jstuever commented Jan 13, 2025

/retest

jstuever added 2 commits

January 22, 2025 15:02


          privatelink: ready and failed conditions logged only when changed

e606ed0


          privatelink: ShouldSync returns false when privatelink is disabled

58aa63d

jstuever force-pushed the HIVE-2687 branch from ae0290e to 0a7fc48 Compare

January 22, 2025 23:05

Contributor

openshift-ci bot commented Jan 23, 2025 •

edited

Loading

@jstuever: The following test 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/e2e-vsphere	`ae0290e`	link	true	`/test e2e-vsphere`

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.

jstuever added 2 commits

January 23, 2025 11:03


          privatelink: actuator reconcile functions to requeue on every change


          privatelink: selectHostedZoneVPC to return a pointer to the selected vpc

eab4e06

jstuever force-pushed the HIVE-2687 branch from 0a7fc48 to eab4e06 Compare

January 23, 2025 19:03

jstuever changed the title ~~WIP: HIVE-2687: Privatelink improvements~~ HIVE-2687: Privatelink improvements

openshift-ci bot removed the do-not-merge/work-in-progress label

Contributor Author

jstuever commented Jan 23, 2025 •

edited

Loading

/assign @2uasimojo
This is ready for review.

openshift-ci bot assigned 2uasimojo

Member

2uasimojo commented Jan 26, 2025

Getting started on this.

It would be cool if the commit messages and the PR description could reference the original PR where the comments were made that prompted this one. (To be clear, not asking for references to specific comments -- just to the PR as a whole.)

2uasimojo reviewed

View reviewed changes

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

               		if err != nil {
               			return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment")
               		}
+              		return reconcile.Result{Requeue: true}, nil

Member

2uasimojo Jan 26, 2025

nit: Requeue: true is redundant as the update will trigger a requeue anyway. I don't hate being explicit though.

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

+              			}
+              		}
+              		break
+              	}
               	_, err = a.awsClientHub.ChangeResourceRecordSets(&route53.ChangeResourceRecordSetsInput{

Member

2uasimojo Jan 26, 2025

nit: the error message on L501 might should say s/adding/changing/?

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

Comment on lines +420 to +421

		for _, record := range record.ResourceRecords {
		oldRecords.Insert(aws.StringValue(record.Value))

Member

2uasimojo Jan 26, 2025

This is a valid but confusing reuse of the var name record. Working backwards, it would seem to make sense to name the outer loop var (defined on L396) some variant of recordSet instead. Which would then be confusing against rSet. Which could perhaps be mitigated by propagating the old vs desired prefixes all the way out.

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

+              			if aws.StringValue(record.Type) != aws.StringValue(rSet.Type) {
+              				modified = true
+              				hzLog.WithFields(log.Fields{
+              					"record": aws.StringValue(rSet.Name),

Member

2uasimojo Jan 26, 2025

The field key here should probably be recordSet or recordSetName or similar.
Is there a reason we wouldn't include this field for every message in this loop iter? Something like rsetLog := hzLog.WithField("recordSetName", rSet.Name) and then use rsetLog throughout this scope?

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

Comment on lines +445 to +451

+              			if record.AliasTarget == nil {
+              				modified = true
+              				hzLog.WithFields(log.Fields{
+              					"record": aws.StringValue(rSet.Name),
+              				}).Debug("updating the record to use alias target")
+              				break
+              			}

Member

2uasimojo Jan 26, 2025

Did I miss the path where we're switching from alias?

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

+              				return false, nil
+              			}
+              		} else if rSet.AliasTarget != nil {
+              			logger.Debugf("AliasTarget")

Member

2uasimojo Jan 26, 2025

Is this still needed?

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

Comment on lines +440 to +442

+              			if !modified {
+              				return false, nil
+              			}

Member

2uasimojo Jan 26, 2025

can this block be DRYed by moving it out one level?

pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go

               	rSet, err := a.recordSet(cd, apiDomain, dnsRecord)
               	if err != nil {
               		return false, errors.Wrap(err, "error generating DNS records")
               	}
+              	recordsResp, err := a.awsClientHub.ListResourceRecordSets(&route53.ListResourceRecordSetsInput{

Member

2uasimojo Jan 26, 2025

This whole delta exists just so we can tell whether we've changed something?

Ugh, I looked at the API and I can see why that needs to be the case. Gross :(

I guess a DeepEquals() isn't sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved jira/valid-reference