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

Bug 1950284: Annotate services of type LoadBalancer with user tags (AWS only) #578

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

frobware
Copy link
Contributor

@frobware frobware commented Mar 23, 2021

This PR addresses https://issues.redhat.com/browse/NE-563.

This is waiting on:

If there are AWS.UserTags defined then services of type LoadBalancer
will be annotated with the following annotation.

service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags

Values for this annotation are encoded as a comma separated list. For example:

"Key1=Val1,Key2=Val2,KeyNoVal1=,KeyNoVal2"

The conceptual docs for this annotation are documented here:
https://kubernetes.io/docs/concepts/services-networking/service/#aws-load-balancer-additional-resource-tags

The operator will only add the annotation on create; there is no
ongoing reconciliation so changes to AWS.UserTags will not propagate
once the LoadBalancer service has been created.

There are restrictions on the key and values, documented here:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html

Resources involved for classic (CLB) and network (NLB) differ.

Atomic create/tag issues on CLBs:

When a CLB is created (w.r.t. tags) the following happens:

  • the load balancer is created with tags (atomic create/tag)

then:

  • the security groups are created
  • then the security groups are updated with tag keys/values

Atomic create/tag issues on NLBs:

When an NBL is created (w.r.t. tags) the following happens:

  • the load balancer is created with tags (atomic create/tag)

then:

  • a target group is created
  • tags are added to the target group
  • a listener is created
  • NO tags are created for the listener (BUG?)

Given that we only tag on create the following issues will not bite
immediately, but capturing them here for completeness:

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2021
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021
@frobware
Copy link
Contributor Author

/hold

Accumulating things to do as I discover them and also to provide a link for the Jira ticket.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2021
@frobware frobware changed the title [WIP] Label AWS Classic LB with user tags -- NE-563 [WIP] Annotate AWS Classic LB with user tags -- NE-563 Mar 24, 2021
@frobware frobware changed the title [WIP] Annotate AWS Classic LB with user tags -- NE-563 [WIP] Annotate AWS Classic LB service with user tags -- NE-563 Mar 24, 2021
@frobware frobware changed the title [WIP] Annotate AWS Classic LB service with user tags -- NE-563 [WIP] Annotate services of type LoadBalancer with user tags (AWS only) Mar 25, 2021
@frobware frobware force-pushed the aws-label-LB-service branch 2 times, most recently from a0cef45 to a92bb3e Compare March 25, 2021 13:32
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2021
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2021
@frobware
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Contributor

@frobware: PR needs rebase.

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/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2021
@frobware frobware force-pushed the aws-label-LB-service branch 2 times, most recently from 048ce97 to 923b024 Compare April 12, 2021 16:14
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2021
@frobware
Copy link
Contributor Author

/test e2e-aws

@frobware
Copy link
Contributor Author

/retest

@frobware frobware changed the title [WIP] Annotate services of type LoadBalancer with user tags (AWS only) Annotate services of type LoadBalancer with user tags (AWS only) Apr 13, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2021
@frobware frobware changed the title Annotate services of type LoadBalancer with user tags (AWS only) [WIP] Annotate services of type LoadBalancer with user tags (AWS only) Apr 15, 2021
@frobware
Copy link
Contributor Author

in case you want to address #578 (comment), but I don't mind merging as-is if you prefer.

Thanks for catching this. Done in af1839e.

If the infrastructure object (on AWS) has additional user supplied
resource tags specified then annotate services of type LoadBalancer as
follows:

 "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags": "Key1=Value1,KeyN=ValueN"

https://issues.redhat.com/browse/NE-563

This is only done at create time, per the enhancement proposal.
Existing services are not tagged. Equally, no reconciliation is done
if the resource keys/values change.
@frobware
Copy link
Contributor Author

/skip

@frobware
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 20, 2021
@frobware
Copy link
Contributor Author

/retest

@Miciah
Copy link
Contributor

Miciah commented Apr 20, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, Miciah

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:

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

@Miciah
Copy link
Contributor

Miciah commented Apr 20, 2021

failed to acquire lease: resources not found

/test e2e-aws-operator

@frobware
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@frobware
Copy link
Contributor Author

failed to acquire lease: resources not found

/test e2e-gcp-serial

@frobware
Copy link
Contributor Author

/test e2e-aws

@sgreene570
Copy link
Contributor

/retest

@frobware
Copy link
Contributor Author

failed to acquire lease: resources not found

/test e2e-gcp-serial

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3e78d80 into openshift:master Apr 22, 2021
@openshift-ci-robot
Copy link
Contributor

@frobware: All pull requests linked via external trackers have merged:

Bugzilla bug 1950284 has been moved to the MODIFIED state.

In response to this:

Bug 1950284: Annotate services of type LoadBalancer with user tags (AWS only)

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/test-infra repository.

@frobware
Copy link
Contributor Author

/cherry-pick release-4.7

@openshift-cherrypick-robot

@frobware: #578 failed to apply on top of branch "release-4.7":

Applying: bump: openshift/api
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
A	vendor/github.com/openshift/api/Makefile
A	vendor/github.com/openshift/api/build/v1/generated.pb.go
A	vendor/github.com/openshift/api/build/v1/generated.proto
A	vendor/github.com/openshift/api/build/v1/types.go
A	vendor/github.com/openshift/api/build/v1/zz_generated.deepcopy.go
A	vendor/github.com/openshift/api/build/v1/zz_generated.swagger_doc_generated.go
M	vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure.crd.yaml
M	vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_ingress.crd.yaml
M	vendor/github.com/openshift/api/config/v1/types_feature.go
M	vendor/github.com/openshift/api/config/v1/types_infrastructure.go
M	vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go
M	vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go
A	vendor/github.com/openshift/api/install.go
A	vendor/github.com/openshift/api/network/v1/001-clusternetwork-crd.yaml
A	vendor/github.com/openshift/api/network/v1/002-hostsubnet-crd.yaml.git/rebase-apply/patch:3480: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

A	vendor/github.com/openshift/api/network/v1/003-netnamespace-crd.yaml
A	vendor/github.com/openshift/api/network/v1/004-egressnetworkpolicy-crd.yaml
M	vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01_crd.yaml
M	vendor/github.com/openshift/api/operator/v1/0000_70_console-operator.crd.yaml
M	vendor/github.com/openshift/api/operator/v1/types_console.go
M	vendor/github.com/openshift/api/operator/v1/types_network.go
M	vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go
M	vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging vendor/k8s.io/api/extensions/v1beta1/generated.pb.go
CONFLICT (content): Merge conflict in vendor/k8s.io/api/extensions/v1beta1/generated.pb.go
Auto-merging vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go
Auto-merging vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go
Auto-merging vendor/github.com/openshift/api/operator/v1/types_network.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/types_network.go
Auto-merging vendor/github.com/openshift/api/operator/v1/types_console.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/types_console.go
Auto-merging vendor/github.com/openshift/api/operator/v1/0000_70_console-operator.crd.yaml
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/0000_70_console-operator.crd.yaml
Auto-merging vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01_crd.yaml
CONFLICT (modify/delete): vendor/github.com/openshift/api/network/v1/004-egressnetworkpolicy-crd.yaml deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/network/v1/004-egressnetworkpolicy-crd.yaml left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/network/v1/003-netnamespace-crd.yaml deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/network/v1/003-netnamespace-crd.yaml left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/network/v1/002-hostsubnet-crd.yaml deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/network/v1/002-hostsubnet-crd.yaml left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/network/v1/001-clusternetwork-crd.yaml deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/network/v1/001-clusternetwork-crd.yaml left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/install.go deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/install.go left in tree.
Auto-merging vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go
Auto-merging vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go
Auto-merging vendor/github.com/openshift/api/config/v1/types_infrastructure.go
Auto-merging vendor/github.com/openshift/api/config/v1/types_feature.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/config/v1/types_feature.go
Auto-merging vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_ingress.crd.yaml
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_ingress.crd.yaml
Auto-merging vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure.crd.yaml
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure.crd.yaml
CONFLICT (modify/delete): vendor/github.com/openshift/api/build/v1/zz_generated.swagger_doc_generated.go deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/build/v1/zz_generated.swagger_doc_generated.go left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/build/v1/zz_generated.deepcopy.go deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/build/v1/zz_generated.deepcopy.go left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/build/v1/types.go deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/build/v1/types.go left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/build/v1/generated.proto deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/build/v1/generated.proto left in tree.
CONFLICT (modify/delete): vendor/github.com/openshift/api/Makefile deleted in HEAD and modified in bump: openshift/api. Version bump: openshift/api of vendor/github.com/openshift/api/Makefile left in tree.
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 bump: openshift/api
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.7

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants