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

feat(cw): added cloudwatch loggroup #939

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

haarchri
Copy link
Member

Signed-off-by: haarchri chhaar30@googlemail.com

Description of your changes

added cloudwatch loggroup
why we need this:

  • control over eks controlplane logs as "managed" loggroup in format /aws/eks/<cluster-name>/cluster to set retentionInDays and the option to delete the loggroup when eks controlplane is removed

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • create & delete
kubectl get loggroup
NAME                READY   SYNCED   EXTERNAL-NAME
sample-loggroup     True    True     /aws/eks/sample-cluster/cluster
  • create & delete multiple loggroups
 kubectl get loggroup
NAME                READY   SYNCED   EXTERNAL-NAME
sample-loggroup     True    True     /aws/eks/sample-cluster/cluster
sample-loggroup-2   True    True     /aws/eks/sample-cluster-2/cluster
kubectl describe loggroup.cloudwatchlogs.aws.crossplane.io/sample-loggroup
Name:         sample-loggroup
Namespace:    
Labels:       <none>
Annotations:  crossplane.io/external-create-pending: 2021-11-14T22:25:03+01:00
              crossplane.io/external-create-succeeded: 2021-11-14T22:25:04+01:00
              crossplane.io/external-name: /aws/eks/sample-cluster/cluster
API Version:  cloudwatchlogs.aws.crossplane.io/v1alpha1
Kind:         LogGroup
Metadata:
  Creation Timestamp:  2021-11-14T21:25:03Z
  Finalizers:
    finalizer.managedresource.crossplane.io
  Generation:  1
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2021-11-14T21:25:03Z
    API Version:  cloudwatchlogs.aws.crossplane.io/v1alpha1
    Manager:         provider
    Operation:       Update
    Time:            2021-11-14T21:25:04Z
  Resource Version:  32821
  UID:               40914687-b241-40d3-bec6-925d37d37581
Spec:
  Deletion Policy:  Delete
  For Provider:
    Log Group Name:     /aws/eks/sample-cluster/cluster
    Region:             us-east-1
    Retention In Days:  1
  Provider Config Ref:
    Name:  example
Status:
  At Provider:
  Conditions:
    Last Transition Time:  2021-11-14T21:25:04Z
    Reason:                Available
    Status:                True
    Type:                  Ready
    Last Transition Time:  2021-11-14T21:25:04Z
    Reason:                ReconcileSuccess
    Status:                True
    Type:                  Synced
Events:
  Type    Reason                   Age   From                                               Message
  ----    ------                   ----  ----                                               -------
  Normal  CreatedExternalResource  10s   managed/loggroup.cloudwatchlogs.aws.crossplane.io  Successfully requested creation of external resource
  Normal  UpdatedExternalResource  10s   managed/loggroup.cloudwatchlogs.aws.crossplane.io  Successfully requested update of external resource

@haarchri haarchri force-pushed the feature/cw-loggroup branch 2 times, most recently from 8f26f10 to 49df9bd Compare November 15, 2021 08:13
Copy link
Collaborator

@chlunde chlunde left a comment

Choose a reason for hiding this comment

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

Looked through the code, looks good to me, but I don't have time to test it at the moment. I think the special case resetting the retention policy should be double checked on slack #dev or with owners.

// CustomLogGroupParameters contains the additional fields for LogGroup.
type CustomLogGroupParameters struct {
// The number of days to retain the log events in the specified log group.
// To set a log group to never have log events expire, use DeleteRetentionPolicy
Copy link
Collaborator

@chlunde chlunde Nov 15, 2021

Choose a reason for hiding this comment

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

I see 0 is added as a special case for crossplane. Not sure if we have something similar elsewhere. I guess nil causes issues with late init? Another option might be to use k8s intstr and support "Forever" or "Infinite"?

I don't think this comment relevant in user facing docs exactly as it is written here: // To set a log group to never have log events expire, use DeleteRetentionPolicy, but I do think it would be good to have a note about the special case w/ 0 and that this calls DeleteRetentionPolicy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough i will Change the Description - my Idea was to use 0 Like Cloudwatch LogGroup in Terraform "If you select 0, the events in the log group are always retained and never expirex

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the description

pkg/controller/cloudwatchlogs/loggroup/setup.go Outdated Show resolved Hide resolved
Signed-off-by: haarchri <chhaar30@googlemail.com>
@haarchri haarchri requested a review from muvaf November 16, 2021 07:16
@muvaf muvaf merged commit 9794480 into crossplane-contrib:master Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants