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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gke-cluster-standard: change logging configuration #1638

Conversation

olliefr
Copy link
Collaborator

@olliefr olliefr commented Aug 31, 2023

This PR changes how gke-cluster-standard module handles an optional logging_config argument.

The change is to use the same object-based interface to configure logging as proposed for gke-cluster-autopilot in #1625. This was requested by @juliocc.

Notes

Logging configuration is more tricky for GKE Standard clusters than it is for GKE Autopilot clusters. This is because in Autopilot, you always have system components and workloads, and the rest of the components are all optional and independent from one another. In Standard, you can have:

  • no components at all (Cloud Logging integration would be turned off)
  • only the system logs (a single component)
  • the system logs and other components. The system logs are mandatory if something else is enabled as well.

I think I have managed to achieve the validation and code generation for all three cases outlined above. But more testing or any other feedback is welcome!

Examples and tests

I also updated the module README with two examples and added two corresponding tests to ./tests/modules/gke_cluster_standard/. I couldn't figure out how to run the full test harness, so please double check if I got this right 馃ゲ

Changes to other components

Since this module already had an implementation for logging_config, I searched the whole CFT repo for references to this module and investigated for each match whether it should be updated. I found only a single place that required updates to its logging configuration - everything else did not mention logging configuration at all because it was relying on the default one. The default configuration did not change.

The updated blueprint is GKE Multitenant Fleet. I modelled my changes on the existing practice for handling input of type object in that module.

Feedback is welcome!


Checklist

I applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

Update logging configuration input to use object interface in line with gke-cluster-autopilot module.
Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

Looks great. Left just one small comment below

@olliefr I added you as contributor. You should be able to merge this once the checks finish

blueprints/gke/multitenant-fleet/variables.tf Outdated Show resolved Hide resolved
@juliocc
Copy link
Collaborator

juliocc commented Aug 31, 2023

The FAST failures are because of the variables types. Just make them match to the new type.

The module test is missing an outer list. It should look something like this:

values:
  module.cluster-1.google_container_cluster.cluster:
    logging_config:
    - enable_components: []

and

  module.cluster-1.google_container_cluster.cluster:
    logging_config:
    - enable_components:
      - APISERVER
      - CONTROLLER_MANAGER
      - SCHEDULER
      - SYSTEM_COMPONENTS
      - WORKLOADS

Update to use the new object-based interface for logging_config when creating GKE Standard clusters.

Add "WORKLOADS" log source to logging configuration. This is to align with what the README says.
@olliefr olliefr linked an issue Aug 31, 2023 that may be closed by this pull request
@olliefr
Copy link
Collaborator Author

olliefr commented Aug 31, 2023

@juliocc all done! What's the preferred way of dealing with the drift from the master branch? Is it OK to merge the master into this branch and then, if successful, merge back into master?

@juliocc
Copy link
Collaborator

juliocc commented Aug 31, 2023

@juliocc all done! What's the preferred way of dealing with the drift from the master branch? Is it OK to merge the master into this branch and then, if successful, merge back into master?

Yes, merging master should be fine.

@olliefr olliefr merged commit 988fd2e into GoogleCloudPlatform:master Aug 31, 2023
9 checks passed
@olliefr olliefr deleted the olliefr/gke-standard-logging-config branch August 31, 2023 12:02
simonebruzzechesse pushed a commit that referenced this pull request Aug 31, 2023
* Update logging configuration of this module to use object interface in harmony with `gke-cluster-autopilot` module.
* Update blueprints that use this module.
* Add "WORKLOADS" log source to logging configuration of the blueprints where the README files say so.
* Update FAST stage 3 because it uses this module.
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.

GKE Multitenant Blueprint - logging configuration
2 participants