-
Notifications
You must be signed in to change notification settings - Fork 191
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
CFE-1134: Watch infrastructure and update AWS tags #1148
base: master
Are you sure you want to change the base?
Conversation
@chiragkyal: This pull request references CFE-1134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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. |
2063ff8
to
7d38529
Compare
/retest |
ea36409
to
f2e5cf8
Compare
@chiragkyal: This pull request references CFE-1134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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. |
@chiragkyal: This pull request references CFE-1134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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. |
/assign @Miciah |
/assign |
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.
Could you add an E2E test? (I don't know whether an E2E test can update the ResourceTags
in the infrastructure config status.)
@@ -134,6 +136,12 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) { | |||
if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{}, handler.EnqueueRequestsFromMapFunc(reconciler.ingressConfigToIngressController))); err != nil { | |||
return nil, err | |||
} | |||
// Watch for changes to infrastructure config to update user defined tags | |||
if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Infrastructure{}, handler.EnqueueRequestsFromMapFunc(reconciler.ingressConfigToIngressController), | |||
predicate.NewPredicateFuncs(hasName(clusterInfrastructureName)), |
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.
The other watches technically should have this predicate too, and ingressConfigToIngressController
should be renamed. However, adding the predicate to the other watches and renaming the map function should be addressed in a follow-up.
ignoredAnnotations := managedLoadBalancerServiceAnnotations.Union(sets.NewString(awsLBAdditionalResourceTags)) | ||
ignoredAnnotations := managedLoadBalancerServiceAnnotations.Clone() | ||
ignoredAnnotations.Delete(awsLBAdditionalResourceTags) |
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.
Can't we just use managedLoadBalancerServiceAnnotations
now?
ignoredAnnotations := managedLoadBalancerServiceAnnotations.Union(sets.NewString(awsLBAdditionalResourceTags)) | |
ignoredAnnotations := managedLoadBalancerServiceAnnotations.Clone() | |
ignoredAnnotations.Delete(awsLBAdditionalResourceTags) | |
return loadBalancerServiceAnnotationsChanged(current, expected, managedLoadBalancerServiceAnnotations) |
To elaborate on that question, there are two general rules at play here:
- First, the status logic sets
Upgradeable=False
if, and only if, it observes a discrepancy between the "managed" annotations' expected values and the actual values. - Second, by the time the status logic runs, there will not be any discrepancy between the expected (desired) annotation values and the actual annotation values.
And these general rules have exceptions:
- As an exception to the first rule, before this PR,
awsLBAdditionalResourceTags
wasn't "managed", but even so, we setUpgradeable=False
if it had been modified. (This is the logic that you are modifying here.) - As an exception to the second rule, if
shouldRecreateLoadBalancer
indicates that changing an annotation value requires recreating the service, then the desired and actual values can differ when the status logic observes them.
So now that you are making the awsLBAdditionalResourceTags
annotation a managed annotation, don't we still want to set Upgradeable=False
if the annotation value doesn't match the expected value?
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.
Thank you for the detailed explanation on how the status logic works and how it sets Upgradeable=False
, as well as the exception that existed with awsLBAdditionalResourceTags
before this PR. Earlier, I was under the impression that the status logic would still set Upgradeable=False
even if awsLBAdditionalResourceTags
was updated by the controller.
So now that you are making the awsLBAdditionalResourceTags annotation a managed annotation, don't we still want to set Upgradeable=False if the annotation value doesn't match the expected value?
Since awsLBAdditionalResourceTags
will now be managed by the controller, and we still want to set Upgradeable=False
if it’s updated by something other than the ingress controller, it does indeed make sense to use managedLoadBalancerServiceAnnotations
directly in this logic. This way, the status logic will behave consistently for managed annotations when any discrepancy is observed.
I've removed the loadBalancerServiceTagsModified()
function and used loadBalancerServiceAnnotationsChanged()
directly inside loadBalancerServiceIsUpgradeable()
and also added some comments for clearer understanding of the flow.
@chiragkyal: This pull request references CFE-1134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I need to try it to see if updating the infrastructure config status is possible through E2E. I used Having said that, do you think we should get QE sign-off for this PR? |
The tests in What I'm wondering is whether the test can update the infrastructure config status without some other controller stomping the changes, and whether there could be other reasons specific to the infrastructures resource or
As a general matter, we should have QE sign-off for this PR. QE might prefer to do pre-merge testing as well. Is day2 tags support being handled by a specific group of QA engineers, or are the QA engineers for each affected component responsible for testing the feature? Cc: @lihongan. |
I just pushed a commit to add an E2E test. It's working fine locally, hope it should work on CI as well.
The QA engineers for each affected component are testing this feature. |
/retest-required |
Did pre-merge test on standalone OCP cluster and it can add new tags key/value pair and update existing tags value, but cannot delete the user added tags. see
@chiragkyal please confirm if that's expected. |
And the tags can be added to new created NLB custom ingresscontroler, but when updating tags in infrastructure, the NLB cannot be updated accordingly.
It looks like bug with NLB? checked the NLB service and we can find the annotation of updated tags, see
|
looks kubernetes/kubernetes#96939 is for NLB fix but closed |
It looks like the tags are getting merged with the existing AWS Resource tags. I think we can only control the @Miciah is there a way we can control this behavior ? |
Okay, it looks like an existing issue then. |
Yeah, it looks like the behavior would need to be corrected in cloud-provider-aws. However, the upstream maintainers' usual response is to suggest using aws-load-balancer-controller instead: kubernetes/cloud-provider-aws#113 (comment). This has been an issue with other features that depend on cloud-provider-aws support as well. Absent support in cloud-provider-aws, these are the options that come to mind:
Is handling updates a hard requirement? openshift/enhancements#1700 is ambiguous:
But also:
The first blockquote was present from the earlier implementation of the feature, so maybe it was supposed to be removed. I'll follow up on openshift/enhancements#1700. |
I started a discussion here: https://github.com/openshift/enhancements/pull/1700/files#r1806975007 |
Okay. The lack of tags reconciliation support for NLB in /cc @TrilokGeer |
@chiragkyal Also did pre-mrege testing with HyperShift and got same results. |
Do we need confirmation that this exception is acceptable? Would the exception be documented in openshift/enhancements#1700? Aside from the matter of reconciling tags for NLBs managed by cloud-controller-manager/cloud-provider-aws, is this PR ready for review? |
I had a discussion with @TrilokGeer on this. He'll check with other stakeholders regarding this issue and once accepted we can document the exception in the EP.
I believe so. And if I recall correctly, we need another review from @candita or @alebedev87 before final approval. |
test/e2e/operator_test.go
Outdated
// awsLBAdditionalResourceTags is set correctly on the | ||
// loadBalancer service associated with the default Ingress Controller. | ||
func TestAWSResourceTagsChanged(t *testing.T) { | ||
t.Parallel() |
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.
Generally, any test that modifies cluster config should be a serial test. I think that that general rule holds true here. We want to avoid nondeterministic test results should changing the aws-load-balancer-additional-resource-tags
annotation somehow affect other tests.
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 for the suggestion. I agreed that we should run this test in serial. I've removed t.Parallel()
and added a go-doc for the function related to this.
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! You also need to update TestAll
.
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! right. I've updated TestAll
and moved TestAWSResourceTagsChanged
into the serial section.
err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) { | ||
service := &corev1.Service{} | ||
if err := kclient.Get(context.Background(), controller.LoadBalancerServiceName(defaultIC), service); err != nil { | ||
t.Logf("failed to get service %s: %v", controller.LoadBalancerServiceName(defaultIC), err) | ||
return false, nil | ||
} | ||
if actualTags, ok := service.Annotations[awsLBAdditionalResourceTags]; !ok { | ||
t.Logf("load balancer has no %q annotation: %v", awsLBAdditionalResourceTags, service.Annotations) | ||
return false, nil | ||
} else if actualTags != expectedTags { | ||
t.Logf("expected %s, found %s", expectedTags, actualTags) | ||
return false, nil | ||
} | ||
return true, nil | ||
}) | ||
if err != nil { | ||
t.Fatalf("timed out waiting for the %s annotation to be updated: %v", awsLBAdditionalResourceTags, err) | ||
} |
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.
Can you use assertServiceAnnotation
here?
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.
assertServiceAnnotation
does not use wait.PollImmediate
, and I think we should wait for the controller to update the annotation using the polling loop. I checked other tests like TestInternalLoadBalancer
and TestAWSLBTypeChange
which are also using similar logic. If it's safe to use assertServiceAnnotation
here, then we can update it.
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.
As we discussed on the call, I'm fine with not using assertServiceAnnotation
. You could add a code comment explaining why you cannot use assertServiceAnnotation
, but I'm fine with the test in any case.
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.
Sure, I've added the following code comment explaining the reason
// Use a polling loop instead of assertServiceAnnotation since
// the operator might not update the annotation immediately.
- Ingress controller now monitors changes to the Infrastructure object, ensuring that modifications to user-defined AWS ResourceTags (platform.AWS.ResourceTags) trigger updates to the load balancer service. - Consider awsLBAdditionalResourceTags annotation as a managed annotation. Signed-off-by: chiragkyal <ckyal@redhat.com>
@chiragkyal: The following tests failed, say
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. |
The PR introduces the following changes:
The ingress controller now watches for the
Infrastructure
object changes. This ensures that any modifications to user-defined tags (platform.AWS.ResourceTags
) trigger an update of the load balancer service.Consider the
awsLBAdditionalResourceTags
annotation as a managed annotation. Any changes to user-defined tags in the Infrastructure object will be reflected in this annotation, prompting an update to the load balancer service.Implements: CFE-1134