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

gke-cluster-autopilot: add logging configuration #1625

Conversation

olliefr
Copy link
Collaborator

@olliefr olliefr commented Aug 27, 2023

Although one cannot disable Cloud Logging and Cloud Monitoring integration in GKE Autopilot clusters, one has some flexibility over which control plane components' logs should be ingested: Available logs

This PR adds a new variable and a block to the cluster resource to facilitate that.

I have attempted to imitate the style of the original code to the best of my abilities. I'm happy to change it if I got it wrong.

I run Terraform format; followed by tfdoc to generate the README.

Feedback is welcome 🙃

PS Against my better judgement, I also fixed a few typos in comments elsewhere, I hope it's OK 😈

PPS If this PR is of interest, I could also do the monitoring config to the same standard in a separate PR...

Although one cannot disable Cloud Logging and Cloud Monitoring integration in GKE Autopilot clusters, one has some flexibility over which control plane components' logs should be ingested. This commit adds a new variable and a block to the cluster resource to facilitate that.
@juliocc
Copy link
Collaborator

juliocc commented Aug 27, 2023

thanks @olliefr for the patch and fixing the typos!

Reading the documentation, it looks like this variable would make sense for standard clusters too. Would you mind porting it to the gke-cluster-standard module too?

PPS If this PR is of interest, I could also do the monitoring config to the same standard in a separate PR...

Please do!

…ter-standard

The input is now defined and consumed in the same style as in gke-cluster-standard module for consistency reasons. Bonus: added validation for the logging_config input variable.
@olliefr
Copy link
Collaborator Author

olliefr commented Aug 29, 2023

Hi @juliocc

I believe gke-cluster-standard already has support for logging configuration:

So what I did, I updated my PR to match the coding style and added input variable validation on top of that so now there is going to be consistent interface between the standard and autopilot modules.

@ludoo ludoo enabled auto-merge (squash) August 29, 2023 02:26
@juliocc
Copy link
Collaborator

juliocc commented Aug 29, 2023

@olliefr I actually prefer your original interface using an object. Would you mind reverting your last commit?

And if you're up to it, also update the standard module to use a similar syntax. You can do this in a separate PR if you prefer.

auto-merge was automatically disabled August 31, 2023 00:18

Head branch was pushed to by a user without write access

@olliefr
Copy link
Collaborator Author

olliefr commented Aug 31, 2023

@juliocc I happen to prefer the original interface too! 😆 I reverted the interface to use an object in the latest commit.

I also added a test for logging_config to the README file. I haven't actually run it because I could not figure out how to fully set up the test environment. But I meticulously compared my test to the existing ones and read the CONTRIBUTING file several times over, so I hope I got it right. Any feedback on the test is welcome!

The logging_config for gke-cluster-standard is a bit more tricky because the validation required there is more complex. But I have already built a little prototype and I think I figured out what to do with the validation. I'll submit a separate PR for gke-cluster-standard.

@olliefr
Copy link
Collaborator Author

olliefr commented Aug 31, 2023

I also just searched the whole cloud-foundation-fabric repo for references to gke-cluster-autopilot to make sure that introducing logging_config does not break any existing code. Because there was no logging_config for gke-cluster-autopilot before, there is no existing code to fix 🙃

@olliefr olliefr merged commit 80e85ad into GoogleCloudPlatform:master Aug 31, 2023
@olliefr olliefr deleted the olliefr/gke-autopilot-logging-config branch August 31, 2023 11:23
simonebruzzechesse pushed a commit that referenced this pull request Aug 31, 2023
Although one cannot disable Cloud Logging and Cloud Monitoring integration in GKE Autopilot clusters, one has some flexibility over which control plane components' logs should be ingested. This commit adds a new variable and a block to the cluster resource to facilitate that.

* gke-cluster-autopilot: update logging configuration and add an example to module README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants