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

[CECO-662] Add registry options in admission controller #1181

Merged

Conversation

kisungyi92
Copy link
Collaborator

@kisungyi92 kisungyi92 commented May 13, 2024

What does this PR do?

A brief description of the change being made with this pull request.

  • This PR is for adding registry option in admissionController config so the customer can add custom registry based on needs.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

Agent: v7.51.0
Cluster Agent: v7.51.0

Describe your test plan

Test label overrides with < 1.7.0. (e.g. 1.6.0)

  • Create DDA with spec.global.registry to “public.ecr.aws/datadog”.
spec:
  global:
    registry : public.ecr.aws/datadog

This configuration is applied for all agent image deployment, However, the init container image created by admission controller wasn’t affected by it.

In agent image deployment,

Containers:
  agent:
    Image:         public.ecr.aws/datadog/agent:7.53.0

But not in sidecar container image.

e.g. init container by admission controller - Library injection.

  • follows default registry.
Init Containers:
  datadog-lib-js-init:
    Container ID:  
    Image:         gcr.io/datadoghq/dd-lib-js-init:latest
  • Update the operator 1.7.0+.

Scenario 1

spec.global.registry will now be affected to not only all agent images but also admission controller sidecar container image.This will end up adding an environment variable : DD_ADMISSION_CONTROLLER_CONTAINER_REGISTRY in the cluster agent.

  • Check the environment variable has been applied properly :
DD_ADMISSION_CONTROLLER_CONTAINER_REGISTRY:                gcr.io/datadoghq

Scenario 2

Depending on the use-case, init container image can only be edited by specifying “spec.features.admissionController.registry”.

This will take precedence over spec.global.registry. (which means, If both spec.global.registry and spec.features.admissionController.registry are set, second one will be applied for init container.)

  • Add below parameter :
  features:
    admissionController:
      registry: asia.gcr.io/datadoghq

  • Check the environment variable has been applied properly :
    • Take a look at the registry name If the features.admissionController.registry takes precedence.
DD_ADMISSION_CONTROLLER_CONTAINER_REGISTRY:                asia.gcr.io/datadoghq
  • Lastly, check with test application If the sidecar container image has been applied correctly.
Init Containers:
  datadog-lib-js-init:
    Image:         asia.gcr.io/datadoghq/dd-lib-js-init:latest

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@kisungyi92 kisungyi92 requested review from a team as code owners May 13, 2024 18:11
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, refactoring, documentation, tooling, dependencies

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.93%. Comparing base (0cf699a) to head (20f596d).
Report is 9 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
- Coverage   59.55%   58.93%   -0.63%     
==========================================
  Files         174      175       +1     
  Lines       21559    21941     +382     
==========================================
+ Hits        12839    12930      +91     
- Misses       7941     8227     +286     
- Partials      779      784       +5     
Flag Coverage Δ
unittests 58.93% <100.00%> (-0.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
apis/datadoghq/v2alpha1/datadogagent_types.go 100.00% <ø> (ø)
...atadogagent/feature/admissioncontroller/feature.go 77.69% <100.00%> (+1.91%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cf699a...20f596d. Read the comment docs.

Copy link
Contributor

@rtrieu rtrieu left a comment

Choose a reason for hiding this comment

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

Minor suggestion but otherwise looks good!

@kisungyi92 kisungyi92 added the enhancement New feature or request label May 15, 2024
@kisungyi92 kisungyi92 changed the title Add registry options in admission controller [CECO-662] Add registry options in admission controller May 17, 2024
Copy link
Contributor

@khewonc khewonc left a comment

Choose a reason for hiding this comment

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

Can you add tests for setting the registry field in the feature_test.go file?

Copy link
Collaborator Author

@kisungyi92 kisungyi92 left a comment

Choose a reason for hiding this comment

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

Test code file for AC has been added.

@@ -584,6 +584,9 @@ type AdmissionControllerFeatureConfig struct {
// Default: "datadog-webhook"
// +optional
WebhookName *string `json:"webhookName,omitempty"`

//The registry enables setting a custom registry name on the admission controller.
Registry *string `json:"registry,omitempty"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This variable is correspond to "clusterAgent.admissionController.containerRegistry" in helm chart.

apis/datadoghq/v2alpha1/datadogagent_types.go Outdated Show resolved Hide resolved
Update docs/configuration.v2alpha1.md

Co-authored-by: Rosa Trieu <107086888+rtrieu@users.noreply.github.com>

Update datadoghq.com_datadogagents.yaml

Update datadoghq.com_datadogagents.yaml

edited some code in features.go to check If there is a value on global.registry

Edited comments for better clarification

Edited some typos

added AC test and deleted for v1alpha1

editing some AC test

editing some AC test code

changing AC test codes

Changing variables in AC feature test code

Adding global registry option test case for AC feature test

Adding v1alpha1 test case back in AC feature test

making empty commit

making empty commit with signature
@kisungyi92 kisungyi92 force-pushed the kisung/add_registry_option_for_admission_controller branch from e8ee6c3 to 20f596d Compare May 22, 2024 18:37
@kisungyi92
Copy link
Collaborator Author

/mergequeue

@dd-devflow
Copy link

dd-devflow bot commented May 22, 2024

⚠️

Action not found: mergequeue

If you need support, contact us on Slack #devflow!

@kisungyi92
Copy link
Collaborator Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 22, 2024

❌ MergeQueue

You are not allowed to use the merge queue towards main.

If you need support, contact us on Slack #devflow with those details!

@kisungyi92 kisungyi92 merged commit 3e962f4 into main May 22, 2024
19 checks passed
@kisungyi92 kisungyi92 deleted the kisung/add_registry_option_for_admission_controller branch May 22, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants