-
Notifications
You must be signed in to change notification settings - Fork 143
add controller to sync aws resource tags #196
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
Closed
staebler
wants to merge
2
commits into
openshift:master
from
staebler:infra_resource_tags_controller
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| reviewers: | ||
| - e-tienne | ||
| - jhixson74 | ||
| - jstuever | ||
| - mtnbikenc | ||
| - patrickdillon | ||
| - rna-afk | ||
| - staebler | ||
| approvers: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| package aws_resource_tags | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "regexp" | ||
| "time" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" | ||
| configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||
| "github.com/openshift/library-go/pkg/controller/factory" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| operatorv1helpers "github.com/openshift/library-go/pkg/operator/v1helpers" | ||
| "k8s.io/apimachinery/pkg/api/equality" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
| "k8s.io/client-go/tools/cache" | ||
| ) | ||
|
|
||
| // tagRegex is used to check that the keys and values of a tag contain only valid characters | ||
| var tagRegex = regexp.MustCompile(`^[0-9A-Za-z_.:/=+-@]*$`) | ||
| // kubernetesNamespaceRegex is used to check that a tag key is not in the kubernetes.io namespace | ||
| var kubernetesNamespaceRegex = regexp.MustCompile(`^([^/]*\.)?kubernetes.io/`) | ||
| // openshiftNamespaceRegex is used to check that a tag key is not in the openshift.io namespace | ||
| var openshiftNamespaceRegex = regexp.MustCompile(`^([^/]*\.)?openshift.io/`) | ||
|
|
||
| // AWSResourceTagsController syncs the AWS resource tags from the spec of the `infrastructure.config.openshift.io/v1` | ||
| // `cluster` object to the status. | ||
| type AWSResourceTagsController struct { | ||
| infraClient configv1client.InfrastructureInterface | ||
| infraLister configv1listers.InfrastructureLister | ||
| } | ||
|
|
||
| // NewController returns a AWSResourceTagsController | ||
| func NewController(operatorClient operatorv1helpers.OperatorClient, | ||
| infraClient configv1client.InfrastructuresGetter, infraLister configv1listers.InfrastructureLister, infraInformer cache.SharedIndexInformer, | ||
| recorder events.Recorder) factory.Controller { | ||
| c := &AWSResourceTagsController{ | ||
| infraClient: infraClient.Infrastructures(), | ||
| infraLister: infraLister, | ||
| } | ||
| return factory.New(). | ||
| WithInformers( | ||
| operatorClient.Informer(), | ||
| infraInformer, | ||
| ). | ||
| WithSync(c.sync). | ||
| WithSyncDegradedOnError(operatorClient). | ||
| ResyncEvery(time.Minute). | ||
| ToController("AWSResourceTagsController", recorder) | ||
| } | ||
|
|
||
| func (c AWSResourceTagsController) sync(ctx context.Context, syncCtx factory.SyncContext) error { | ||
| obji, err := c.infraLister.Get("cluster") | ||
| if errors.IsNotFound(err) { | ||
| syncCtx.Recorder().Warningf("AWSResourceTagsController", "Required infrastructures.%s/cluster not found", configv1.GroupName) | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| currentInfra := obji.DeepCopy() | ||
|
|
||
| var desiredResourceTags []configv1.AWSResourceTag | ||
| if awsSpec := currentInfra.Spec.PlatformSpec.AWS; awsSpec != nil { | ||
| keys := sets.NewString() | ||
| desiredResourceTags = make([]configv1.AWSResourceTag, 0, len(awsSpec.ResourceTags)) | ||
| for _, tag := range awsSpec.ResourceTags { | ||
| if err := validateTag(tag); err != nil { | ||
| syncCtx.Recorder().Warningf("AWSResourceTagsController", "The resource tag with key=%q and value=%q is invalid: %v", tag.Key, tag.Value, err) | ||
| continue | ||
| } | ||
| if keys.Has(tag.Key) { | ||
| syncCtx.Recorder().Warningf("AWSResourceTagsController", "The resource tag with key=%q and value=%q is a duplicate", tag.Key, tag.Value) | ||
| continue | ||
| } | ||
| keys.Insert(tag.Key) | ||
| desiredResourceTags = append(desiredResourceTags, tag) | ||
| } | ||
| } | ||
|
|
||
| var currentResourceTags []configv1.AWSResourceTag | ||
| if currentInfra.Status.PlatformStatus != nil && | ||
| currentInfra.Status.PlatformStatus.AWS != nil { | ||
| currentResourceTags = currentInfra.Status.PlatformStatus.AWS.ResourceTags | ||
| } | ||
|
|
||
| if len(desiredResourceTags) == 0 && len(currentResourceTags) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| if equality.Semantic.DeepEqual(desiredResourceTags, currentResourceTags) { | ||
| return nil | ||
| } | ||
|
|
||
| if currentInfra.Status.PlatformStatus == nil { | ||
| currentInfra.Status.PlatformStatus = &configv1.PlatformStatus{} | ||
| } | ||
| if currentInfra.Status.PlatformStatus.AWS == nil { | ||
| currentInfra.Status.PlatformStatus.AWS = &configv1.AWSPlatformStatus{} | ||
| } | ||
| currentInfra.Status.PlatformStatus.AWS.ResourceTags = desiredResourceTags | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless order matters, please sort the status resourcetags. |
||
|
|
||
| _, err = c.infraClient.UpdateStatus(ctx, currentInfra, metav1.UpdateOptions{}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think an event on successful update is also warranted. |
||
| if err != nil { | ||
| syncCtx.Recorder().Warningf("AWSResourceTagsController", "Unable to update the infrastructure status") | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // validateTag checks the following things to ensure that the tag is acceptable as an additional tag. | ||
| // * The key and value contain only valid characters. | ||
| // * The key is not empty and at most 128 characters. | ||
| // * The value is not empty and at most 256 characters. Note that, while many AWS services accept empty tag values, | ||
| // the additional tags may be applied to resources in services that do not accept empty tag values. Consequently, | ||
| // OpenShift cannot accept empty tag values. | ||
| // * The key is not in the kubernetes.io namespace. | ||
| // * The key is not in the openshift.io namespace. | ||
| func validateTag(tag configv1.AWSResourceTag) error { | ||
| if !tagRegex.MatchString(tag.Key) { | ||
| return fmt.Errorf("key contains invalid characters") | ||
| } | ||
| if !tagRegex.MatchString(tag.Value) { | ||
| return fmt.Errorf("value contains invalid characters") | ||
| } | ||
| if len(tag.Key) == 0 { | ||
| return fmt.Errorf("key is empty") | ||
| } | ||
| if len(tag.Key) > 128 { | ||
| return fmt.Errorf("key is too long") | ||
| } | ||
| if len(tag.Value) == 0 { | ||
| return fmt.Errorf("value is empty") | ||
| } | ||
| if len(tag.Value) > 256 { | ||
| return fmt.Errorf("value is too long") | ||
| } | ||
| if kubernetesNamespaceRegex.MatchString(tag.Key) { | ||
| return fmt.Errorf("key is in the kubernetes.io namespace") | ||
| } | ||
| if openshiftNamespaceRegex.MatchString(tag.Key) { | ||
| return fmt.Errorf("key is in the openshift.io namespace") | ||
| } | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
seems like this case is handled by the deepequal on the next line. is it not?