MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull. #5884

paul-maidment · 2024-01-15T08:55:24Z

MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull.

This PR introduces a new field to the AgentServiceConfig CRD imagePullCAConfigMap which is a LocalObjectReference to a config map containing CA certificates
These certificates are to be used by the image service for the purpose of verifying the CA of HTTPS connections used for pulling images.

imagePullConfigMap is expected to reference a ConfigMap containing a single file extra-ca.pem which contains the CA certificate.
If multiple CA certificates are to be used, the user is expected to append these into the same PEM.

This code also passes the path ADDITIONAL_CA_FILE to the image service if the additional CA file is present.

This code sets up the Volume for this and maps to either an empty directory or maps the content of the ConfigMap.

This will ensure that the CA's will be available for use by the image service.

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
[] This change does not require a documentation update (docstring, docs, README, etc)
Docstrings have been updated to describe the purpose of this change.
There is also a ticket in the Epic to support more general documentation.
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2024-01-15T08:55:28Z

@paul-maidment: This pull request references MGMT-16508 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

This PR introduces a new field to the AgentServiceConfig CRD imagePullCAConfigMap which is a LocalObjectReference to a config map containing CA certificates These certificates are to be used by the image service for the purpose of verifying the CA of HTTPS connections used for pulling images.

If imagePullCAConfigMap has an influence over a volume definition for "additional-ca-bundle" in the StatefulSet for assisted-image-service. Any certificates defined within the data section of the map are to be stored in "/additional-ca-bundle"

This code sets up the Volume for this and maps to either an empty directory or maps the content of the ConfigMap.

This will ensure that the CA's will be available for use by the image service.

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

[] This change does not require a documentation update (docstring, docs, README, etc)
Docstrings have been updated to describe the purpose of this change.
There is also a ticket in the Epic to support more general documentation.

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

codecov · 2024-01-15T09:27:52Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (dd3e90d) 68.18% compared to head (c4f131d) 68.21%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5884      +/-   ##
==========================================
+ Coverage   68.18%   68.21%   +0.02%     
==========================================
  Files         236      236              
  Lines       34788    34836      +48     
==========================================
+ Hits        23721    23763      +42     
- Misses       8999     9002       +3     
- Partials     2068     2071       +3

Files	Coverage Δ
...oller/controllers/agentserviceconfig_controller.go	`85.08% <81.63%> (+0.07%)`	⬆️

... and 2 files with indirect coverage changes

paul-maidment · 2024-01-15T11:23:44Z

/retest

gamli75 · 2024-01-18T09:15:11Z

@avishayt @carbonin please review

paul-maidment · 2024-01-18T15:16:34Z

/retest

openshift-ci-robot · 2024-01-18T15:17:43Z

@paul-maidment: This pull request references MGMT-16508 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull.

This PR introduces a new field to the AgentServiceConfig CRD imagePullCAConfigMap which is a LocalObjectReference to a config map containing CA certificates
These certificates are to be used by the image service for the purpose of verifying the CA of HTTPS connections used for pulling images.

imagePullConfigMap is expected to reference a ConfigMap containing a single file extra-ca.pem which contains the CA certificate.
If multiple CA certificates are to be used, the user is expected to append these into the same PEM.

This code also passes the path ADDITIONAL_CA_FILE to the image service if the additional CA file is present.

This code sets up the Volume for this and maps to either an empty directory or maps the content of the ConfigMap.

This will ensure that the CA's will be available for use by the image service.

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

[] This change does not require a documentation update (docstring, docs, README, etc)
Docstrings have been updated to describe the purpose of this change.
There is also a ticket in the Epic to support more general documentation.

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-01-18T15:18:05Z

@paul-maidment: This pull request references MGMT-16508 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull.

This PR introduces a new field to the AgentServiceConfig CRD imagePullCAConfigMap which is a LocalObjectReference to a config map containing CA certificates
These certificates are to be used by the image service for the purpose of verifying the CA of HTTPS connections used for pulling images.

imagePullConfigMap is expected to reference a ConfigMap containing a single file extra-ca.pem which contains the CA certificate.
If multiple CA certificates are to be used, the user is expected to append these into the same PEM.

This code also passes the path ADDITIONAL_CA_FILE to the image service if the additional CA file is present.

This code sets up the Volume for this and maps to either an empty directory or maps the content of the ConfigMap.

This will ensure that the CA's will be available for use by the image service.

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

[] This change does not require a documentation update (docstring, docs, README, etc)
Docstrings have been updated to describe the purpose of this change.
There is also a ticket in the Epic to support more general documentation.

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

carbonin · 2024-01-19T13:31:05Z

api/v1beta1/agentserviceconfig_types.go

+	// this certificate will be used by the assisted-image-service when pulling OS images.
+	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Image pull CA config map reference"
+	// +optional
+	ImagePullCAConfigMapRef *corev1.LocalObjectReference `json:"imagePullCAConfigMapRef,omitempty"`


I think this should reference OSImages in some way to make sure it's not confused with pulling container images.

Maybe OSImageCACertRef?

carbonin · 2024-01-19T13:31:52Z

api/v1beta1/agentserviceconfig_types.go

@@ -109,6 +109,12 @@ type AgentServiceConfigSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="List of container registries without authentication"
 	// +optional
 	UnauthenticatedRegistries []string `json:"unauthenticatedRegistries,omitempty"`
+	// ImagePullCAConfigMapRef is a reference to a config map containing a certificate authority certificate


Can you just add a blank line between all the fields and the next field comment?
I see it's inconsistent in this struct, but we can fix that now.

carbonin · 2024-01-19T13:32:32Z

api/v1beta1/agentserviceconfig_types.go

@@ -109,6 +109,12 @@ type AgentServiceConfigSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="List of container registries without authentication"
 	// +optional
 	UnauthenticatedRegistries []string `json:"unauthenticatedRegistries,omitempty"`
+	// ImagePullCAConfigMapRef is a reference to a config map containing a certificate authority certificate
+	// this is an optional certificate to allow a customer to add a certificate authority for a HTTPS source of images


s/customer/user/

carbonin · 2024-01-19T13:35:25Z

internal/controller/controllers/agentserviceconfig_controller.go

+			// Read the referenced ConfigMap to ensure that it contains a single file "extra-ca.crt"
+			additionalCABundleCM := &corev1.ConfigMap{}
+			if err := asc.Client.Get(ctx, types.NamespacedName{Name: asc.spec.ImagePullCAConfigMapRef.Name, Namespace: asc.namespace}, additionalCABundleCM); err != nil {
+				log.WithError(err).Errorf("Failed to get additional CA Bundle config map %s", asc.spec.ImagePullCAConfigMapRef.Name)
+				return err
+			}
+			var err error
+			_, ok := additionalCABundleCM.Data["extra-ca.pem"]
+			if !ok {
+				err = multierror.Append(err, fmt.Errorf("expected to find single file `extra-ca.pem` in the ConfigMap %s but it was not found", asc.spec.ImagePullCAConfigMapRef.Name))
+			}
+			if len(additionalCABundleCM.Data) != 1 {
+				err = multierror.Append(err, fmt.Errorf("found multiple files in ConfigMap %s, a single file was expected but %d files were found", asc.spec.ImagePullCAConfigMapRef.Name, len(additionalCABundleCM.Data)))
+			}
+			if err != nil {
+				return err
+			}


I feel like all this validation doesn't belong in the StatefulSet mutate function.

I'd say this should be in the validate function and we should set a condition if any of the references are invalid.

carbonin · 2024-01-19T13:38:52Z

internal/controller/controllers/agentserviceconfig_controller.go

+			volumes = ensureVolume(volumes, corev1.Volume{
+				Name: "additional-ca-bundle",
+				VolumeSource: corev1.VolumeSource{
+					EmptyDir: &corev1.EmptyDirVolumeSource{},


We're not putting anything here in this case so the volume and volume mount should be added only if we have certs to add.

carbonin · 2024-01-19T13:39:43Z

internal/controller/controllers/agentserviceconfig_controller.go

+			volumeSource := &corev1.ConfigMapVolumeSource{
+				LocalObjectReference: corev1.LocalObjectReference{
+					Name: asc.spec.ImagePullCAConfigMapRef.Name,
+				},
+			}
+			volumes = ensureVolume(volumes, corev1.Volume{
+				Name: "additional-ca-bundle",
+				VolumeSource: corev1.VolumeSource{
+					ConfigMap: volumeSource,
+				},
+			})


I don't think you need the volumeSource intermediate variable here I'd just put it all in one struct.

carbonin · 2024-01-19T13:41:04Z

internal/controller/controllers/agentserviceconfig_controller.go

@@ -1177,6 +1178,7 @@ func newImageServiceStatefulSet(ctx context.Context, log logrus.FieldLogger, asc
 			{Name: "tls-certs", MountPath: "/etc/image-service/certs"},
 			{Name: "service-cabundle", MountPath: "/etc/image-service/ca-bundle"},
 			{Name: "image-service-data", MountPath: "/data"},
+			{Name: "additional-ca-bundle", MountPath: "/additional-ca-bundle"},


Maybe put this in /etc/image-service like the others. How about /etc/image-service/os-images-ca-bundle?

paul-maidment · 2024-01-22T08:24:59Z

/retest

paul-maidment · 2024-01-22T10:23:32Z

/retest

paul-maidment · 2024-01-22T11:08:48Z

/retest

carbonin · 2024-01-24T14:03:37Z

/hold

Just thought of this. Are we doing anything to ensure we update the image service when the contents of this config map change?
Are we doing something like that currently for the other reference?

paul-maidment · 2024-01-24T23:32:50Z

/hold

Just thought of this. Are we doing anything to ensure we update the image service when the contents of this config map change? Are we doing something like that currently for the other reference?

When you make a change to the content of the ConfigMap these are kept in sync automatically, I actually believe this is core functionality in Kubernetes.

There doesn't appear to be a redeployment but the volume is simply kept in sync, if you shell into the image service then you can see the files being updated within 30 seconds of any change to the ConfigMap

As we can see in this excerpt of the statefulset, there is a reference to the ConfigMap in the volume, I think Kubernetes must be watching these ConfigMaps.

...
      volumes:
      - name: tls-certs
        secret:
          defaultMode: 420
          secretName: assisted-image-service
      - configMap:
          defaultMode: 420
          name: assisted-image-service
        name: service-cabundle
      - configMap:
          defaultMode: 420
          name: image-service-additional-ca
        name: additional-ca-bundle
...

I would assume this is similar for EnvFrom.
Of course, this alone does not cause the statefulset to redeploy, so we probably have to handle that anyway.

paul-maidment · 2024-01-25T15:06:21Z

/retest

carbonin · 2024-01-26T13:57:28Z

internal/controller/controllers/agentserviceconfig_controller.go

@@ -1712,6 +1743,7 @@ func newAssistedServiceDeployment(ctx context.Context, log logrus.FieldLogger, a
 		setAnnotation(meta, assistedConfigHashAnnotation, assistedConfigHash)
 		setAnnotation(meta, mirrorConfigHashAnnotation, mirrorConfigHash)
 		setAnnotation(meta, userConfigHashAnnotation, userConfigHash)
+		setAnnotation(meta, osImagesCAConfigHashAnnotation, osImagesCAConfigHash)


This should be being set on the image service, not assisted.

openshift-ci · 2024-01-26T14:02:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paul-maidment

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [paul-maidment]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

carbonin

Last nit. Otherwise looks good.

carbonin · 2024-01-29T13:12:03Z

internal/controller/controllers/agentserviceconfig_controller.go

@@ -168,6 +171,7 @@ func initASC(r *AgentServiceConfigReconciler, instance *aiv1beta1.AgentServiceCo
 	return asc
 }

+type NewStatefulSetFn func(context.Context, logrus.FieldLogger, ASC) (*appsv1.StatefulSet, controllerutil.MutateFn)


I don't think this should be defined here if it's only used in tests.

You are also only using AssertReconcileSuccessStatefulSetFn in one place. Maybe just don't define that function and then you wouldn't need this type.

…tificates for image pull. This PR introduces a new field to the AgentServiceConfig CRD `imagePullCAConfigMap` which is a LocalObjectReference to a config map containing CA certificates These certificates are to be used by the image service for the purpose of verifying the CA of HTTPS connections used for pulling images. `imagePullConfigMap` is expected to reference a ConfigMap containing a single file `extra-ca.pem` which contains the CA certificate. If multiple CA certificates are to be used, the user is expected to append these into the same PEM. This code also passes the path `ADDITIONAL_CA_FILE` to the image service if the additional CA file is present. This code sets up the Volume for this and maps to either an empty directory or maps the content of the ConfigMap. This will ensure that the CA's will be available for use by the image service.

carbonin · 2024-01-29T14:45:58Z

/unhold

carbonin · 2024-01-29T19:57:32Z

/unhold

carbonin · 2024-01-29T19:57:37Z

/lgtm

carbonin · 2024-01-30T14:02:15Z

/retest

openshift-ci-robot · 2024-01-30T15:12:23Z

/retest-required

Remaining retests: 0 against base HEAD 7dbb3b1 and 2 for PR HEAD c4f131d in total

openshift-bot · 2024-01-30T22:41:24Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-agent-installer-api-server-container-v4.16.0-202401302041.p0.g268d4ed.assembly.stream for distgit ose-agent-installer-api-server.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 15, 2024

openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 15, 2024

openshift-ci bot requested review from eliorerz and filanov January 15, 2024 08:56

openshift-ci bot added api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2024

paul-maidment force-pushed the MGMT-16508 branch 2 times, most recently from 692ceb3 to 2adc972 Compare January 15, 2024 08:59

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2024

paul-maidment force-pushed the MGMT-16508 branch from 2adc972 to 84082e5 Compare January 15, 2024 09:52

paul-maidment mentioned this pull request Jan 15, 2024

MGMT-16507: Enable image service to use additional ca certificates openshift/assisted-image-service#172

Merged

5 tasks

paul-maidment force-pushed the MGMT-16508 branch 5 times, most recently from 2fe86ed to 0a38c5a Compare January 18, 2024 15:12

carbonin reviewed Jan 19, 2024

View reviewed changes

paul-maidment force-pushed the MGMT-16508 branch from 0a38c5a to c4fe712 Compare January 21, 2024 23:58

paul-maidment force-pushed the MGMT-16508 branch from c4fe712 to 048de5a Compare January 22, 2024 11:37

paul-maidment force-pushed the MGMT-16508 branch 2 times, most recently from 1093ca5 to 0a28772 Compare January 24, 2024 12:54

paul-maidment requested a review from carbonin January 24, 2024 13:41

carbonin approved these changes Jan 24, 2024

View reviewed changes

openshift-ci bot assigned carbonin Jan 24, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2024

paul-maidment force-pushed the MGMT-16508 branch from 0a28772 to 1d6ce11 Compare January 25, 2024 08:58

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2024

carbonin requested changes Jan 26, 2024

View reviewed changes

paul-maidment force-pushed the MGMT-16508 branch 2 times, most recently from 3f1da87 to 746d282 Compare January 28, 2024 14:00

paul-maidment requested a review from carbonin January 29, 2024 08:08

carbonin reviewed Jan 29, 2024

View reviewed changes

paul-maidment force-pushed the MGMT-16508 branch from 746d282 to c4f131d Compare January 29, 2024 13:55

carbonin approved these changes Jan 29, 2024

View reviewed changes

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 29, 2024

openshift-merge-bot bot merged commit 268d4ed into openshift:master Jan 30, 2024
15 of 17 checks passed

MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull. #5884

MGMT-16508: Add API to AgentServiceConfig CRD to allow pass of CA certificates for image pull. #5884

Conversation

paul-maidment commented Jan 15, 2024 • edited by openshift-ci bot

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 15, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

codecov bot commented Jan 15, 2024 • edited

Codecov Report

paul-maidment commented Jan 15, 2024

gamli75 commented Jan 18, 2024

paul-maidment commented Jan 18, 2024

openshift-ci-robot commented Jan 18, 2024 • edited by openshift-ci bot

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 18, 2024 • edited by openshift-ci bot

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment commented Jan 22, 2024

paul-maidment commented Jan 22, 2024

paul-maidment commented Jan 22, 2024

carbonin commented Jan 24, 2024

paul-maidment commented Jan 24, 2024 • edited

paul-maidment commented Jan 25, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Jan 26, 2024

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonin commented Jan 29, 2024

carbonin commented Jan 29, 2024

carbonin commented Jan 29, 2024

carbonin commented Jan 30, 2024

openshift-ci-robot commented Jan 30, 2024

openshift-bot commented Jan 30, 2024

paul-maidment commented Jan 15, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 15, 2024 •

edited by openshift-ci bot

codecov bot commented Jan 15, 2024 •

edited

openshift-ci-robot commented Jan 18, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 18, 2024 •

edited by openshift-ci bot

paul-maidment commented Jan 24, 2024 •

edited