Adding Credential Provider Package with source and helm chart #829

junshun · 2023-02-21T00:47:47Z

*Issue #717

Description of changes:
Adding source code and helm chart for credential provider package. This package provides nodes with credentials access to private registries using Kubernetes Credential Provider. This inital commit will only support Elastic Container Registry.

This package will take a aws config secret on the cluster and handle creation of the KubeletCredentialProviderConfig, putting binaries on the individual nodes, and setting the flags on the kubelet using a Daemonset.

For Bottlerocket there specific API is used to trigger the changes necessary on each node. For other OS a directory is mounted on the node to put the necessary files for authentication.

Notes For Review

credentialproviderpackage/charts - includes the helm chart following the format of other helm charts for packages. There is also a helper function added to detect OS which is used to mount file paths depending on the OS.

credentialproviderpackage/pkg/filewriter
credentialproviderpackage/pkg/templater
These two were manually brought over from eks-anywhere as to not have to force a dependency on them.

credentialproviderpackage/skaffold.yaml
Used to help locally test and debug changes

eks-distro-bot · 2023-02-21T00:47:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

junshun · 2023-02-21T00:48:03Z

/hold

junshun · 2023-02-27T18:28:12Z

/unhold

TerryHowe · 2023-02-27T23:41:19Z

credentialproviderpackage/skaffold.yaml

@@ -0,0 +1,24 @@
+apiVersion: skaffold/v3


Not sure we want this

This combined with the dockerfile helps enable locally testing. This package is otherwise hard to test locally as its meant to run on the node.

For vscode users maybe

I've never used skaffold. Might be worth doing a quick demo to see if it's a tool we might leverage.

credentialproviderpackage/pkg/utils/utils.go

TerryHowe · 2023-02-28T18:44:58Z

credentialproviderpackage/Dockerfile

+ARG SKAFFOLD_GO_GCFLAGS
+RUN go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -o app cmd/aws-credential-provider/*.go
+
+CMD ["/app/app"]


I never like that we have docker files, but don't use them.

In this case the dockerfile is useful for local testing as this binary can't really be tested locally since it requires the dependent binaries (ecr-credential-provider, iam-signing-helper) to copy.

TerryHowe · 2023-02-28T18:46:28Z

credentialproviderpackage/Makefile

+
+build-linux:
+	[ -d bin ] || mkdir bin
+	env CGO_ENABLED=0 GOOS=linux GOARCH=amd64 $(MAKE) build


Don't we need to build this for linux arm and amd?

I didn't think we supported ARM for the actual cluster but I can add it. Should work the same on ARM

We're not going to use this makefile for the actual image build right? This will be in build-tooling.

The package controller does do arch or os builds

build: go.sum generate ## Build package-manager binary. $(GO) build -o $(BIN_DIR)/package-manager main.go

TerryHowe · 2023-02-28T18:51:38Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+	checkErrAndLog(err, utils.ErrorLogger)
+
+	utils.InfoLogger.Println("Kubelet Restarted")
+


You could increase your test coverage a lot if this was broken up

This code isn't tested.

So you would like to see a test where the configurator interface is mocked this code is called from another package?

TerryHowe · 2023-02-28T18:56:13Z

credentialproviderpackage/pkg/configurator/bottlerocket/bottlerocket.go

+	config  constants.CredentialProviderConfigOptions
+}
+
+type AwsCred struct {


Suggested change

type AwsCred struct {

type awsCred struct {

Maybe?

Why use this at all since it only contains one instance of Aws?

This is used to match the bottlerocket api. I just generated the stucts based on the json.

lewisdiamond · 2023-02-22T15:14:55Z

credentialproviderpackage/Dockerfile

@@ -0,0 +1,14 @@
+FROM golang:1.18-buster


We want to use internal base images here. But why do we need a Dockerfile here, this will be in build-tooling.

So both this dockerfile and makefile are used for local testing. I am fine to remove it

lewisdiamond · 2023-02-22T15:19:23Z

credentialproviderpackage/charts/credential-provider-package/Chart.yaml

@@ -0,0 +1,24 @@
+apiVersion: v2
+name: credential-provider-package
+description: A Helm chart for Kubernetes


Clean up this file.

lewisdiamond · 2023-02-22T15:19:46Z

credentialproviderpackage/charts/credential-provider-package/templates/NOTES.txt

@@ -0,0 +1 @@
+1. Installed Credential Provider Package


Can you remove this file?

lewisdiamond · 2023-02-22T15:30:12Z

credentialproviderpackage/charts/credential-provider-package/templates/deployment.yaml

+            {{- end}}
+          env:
+            - name: OS_TYPE
+              value: {{ template "template.getOSName" }}


reuse $os

lewisdiamond · 2023-02-28T20:40:59Z

credentialproviderpackage/Makefile

+
+build-linux:
+	[ -d bin ] || mkdir bin
+	env CGO_ENABLED=0 GOOS=linux GOARCH=amd64 $(MAKE) build


We're not going to use this makefile for the actual image build right? This will be in build-tooling.

credentialproviderpackage/charts/credential-provider-package/templates/daemonset.yaml

credentialproviderpackage/charts/credential-provider-package/templates/serviceaccount.yaml

lewisdiamond · 2023-02-28T20:56:14Z

credentialproviderpackage/charts/credential-provider-package/templates/daemonset.yaml

+            {{- end}}
+          env:
+            - name: OS_TYPE
+              value: {{ template "template.getOSName" }}


lewisdiamond · 2023-03-03T15:41:00Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+}
+
+func main() {
+	utils.InfoLogger.Println("Running at " + time.Now().UTC().String())


Don't print the date. Logs are already dated.

lewisdiamond · 2023-03-03T15:43:14Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+	var configurator cfg.Configurator
+	osType := strings.ToLower(os.Getenv("OS_TYPE"))
+	if osType == "" {
+		utils.ErrorLogger.Println("Missing Environment Variable OS")


Suggested change

utils.ErrorLogger.Println("Missing Environment Variable OS")

utils.ErrorLogger.Println("Missing Environment Variable OS_TYPE")

lewisdiamond · 2023-03-03T15:54:25Z

credentialproviderpackage/pkg/utils/utils.go

+)
+
+func init() {
+	InfoLogger = log.New(os.Stdout, "INFO: ", log.Ldate|log.Ltime|log.Lshortfile)


Don't include the date. Agree with Terry on this package.

lewisdiamond · 2023-03-03T15:55:03Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+
+func checkErrAndLog(err error, logger *log.Logger) {
+	if err != nil {
+		logger.Println(err)


If you're always doing os.EXIT(1), don't take a logger as input and call log.Fatal. Although repeatedly checking errors with if err != nil { ... } is annoying, this function call doesn't really improve it much if at all. I'd instead do the check directly and remove this.

lewisdiamond · 2023-03-03T15:58:05Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+			configurator = bottlerocket.NewBottleRocketConfigurator(constants.SocketPath)
+
+		} else {
+			utils.ErrorLogger.Printf("Unexpected type %s expected socket\n", socket.Mode().Type())


Just call log.Fatal

lewisdiamond · 2023-03-03T15:58:58Z

credentialproviderpackage/go.mod

@@ -0,0 +1,28 @@
+module credential-provider
+
+go 1.18


lewisdiamond · 2023-03-03T16:04:01Z

credentialproviderpackage/pkg/configurator/bottlerocket/bottlerocket.go

+	config  constants.CredentialProviderConfigOptions
+}
+
+type AwsCred struct {


Why use this at all since it only contains one instance of Aws?

lewisdiamond · 2023-03-03T16:04:50Z

credentialproviderpackage/pkg/configurator/bottlerocket/bottlerocket.go

+type credentialProviders struct {
+	EcrCredentialProvider ecrCredentialProvider `json:"ecr-credential-provider"`
+}
+type kubernetes struct {


I see many structs that contain a single thing of another type. Why do we need those?

These structs were from br where they can have more in the structure, however I removed the used portions from ours.

I think they are messages to BR

lewisdiamond · 2023-03-03T17:06:43Z

credentialproviderpackage/pkg/configurator/bottlerocket/bottlerocket_test.go

+			},
+			patchResponse: response{
+				statusCode:   http.StatusNoContent,
+				expectedBody: []byte("{\"aws\":{\"config\":\"W3Byb2ZpbGUgZWtzYS1wYWNrYWdlc10KYXdzX2FjY2Vzc19rZXlfaWQ9QUtJQUlPU0ZPRE5ON0VYQU1QTEUKYXdzX3NlY3JldF9hY2Nlc3Nfa2V5PXdKYWxyWFV0bkZFTUkvSzdNREVORy9iUHhSZmlDWUVYQU1QTEVLRVk=\",\"profile\":\"eksa-packages\",\"region\":\"\"}}"),


Consider decoding this and calling the encode function before creating this object.

TerryHowe · 2023-03-06T20:20:54Z

credentialproviderpackage/Makefile

+
+build-linux:
+	[ -d bin ] || mkdir bin
+	env CGO_ENABLED=0 GOOS=linux GOARCH=amd64 $(MAKE) build


The package controller does do arch or os builds

build: go.sum generate ## Build package-manager binary. $(GO) build -o $(BIN_DIR)/package-manager main.go

TerryHowe · 2023-03-06T20:27:03Z

...entialproviderpackage/pkg/configurator/linux/testdata/expected-config-multiple-patterns.yaml

+    apiVersion: credentialprovider.kubelet.k8s.io/v1alpha1
+    env:
+      - name: AWS_PROFILE
+        value: eksa-packages


Suggested change

value: eksa-packages

value: {{.profile}}

This is the test file that is matched against hence the values being written here instead of templating.

TerryHowe · 2023-03-06T20:28:06Z

credentialproviderpackage/pkg/configurator/linux/testdata/expected-config.yaml

+    apiVersion: credentialprovider.kubelet.k8s.io/v1alpha1
+    env:
+      - name: AWS_PROFILE
+        value: eksa-packages


Suggested change

value: eksa-packages

value: {{.profile}}

Same as above, This is the test file that is matched against hence the values being written here instead of templating.

TerryHowe · 2023-03-06T20:29:47Z

credentialproviderpackage/charts/credential-provider-package/values.yaml

+application:
+  secretName: aws-cred
+  matchImages: []
+  defaultCacheDuration: ""


should the chart provide a default duration?

I can include one, currently the application provides one if not included by chart

TerryHowe · 2023-03-06T20:31:52Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+	if osType == "" {
+		log.ErrorLogger.Println("Missing Environment Variable OS_TYPE")
+		os.Exit(1)
+	}


This is fine, why not command line args?

I usually just fine environment variables easier to test with and setup IDEs as well

Command line args are better I think in this case. One can easily find the args that need to be used without needing to run the application vs in this case the application needs to be run to find out what is missing.

I find command line args are more difficult to test with tools such as scaffold where they try to override the entrypoint with a different one for debugging.

If there is strong feelings towards this let me know and I can change it to command line

TerryHowe · 2023-03-06T20:49:22Z

credentialproviderpackage/pkg/configurator/linux/linux.go

+	}
+
+	out := strings.Join(lines, "\n")
+	err = ioutil.WriteFile(c.extraArgsPath, []byte(out), 0644)


World read?

This file is the KUBELET_EXTRA_ARGS file which by default is world read

TerryHowe · 2023-03-06T20:50:57Z

credentialproviderpackage/pkg/configurator/linux/linux.go

+func copyBinaries() (string, error) {
+	srcPath := constants.BinPath + constants.ECRCredProviderBinary
+	dstPath := constants.BasePath + constants.ECRCredProviderBinary
+	err := copyWithPermissons(srcPath, dstPath, 0744)


World read? If these are binaries, why not 755?

TerryHowe · 2023-03-06T20:52:19Z

credentialproviderpackage/pkg/configurator/linux/linux.go

+
+	srcPath = constants.BinPath + constants.IAMRolesSigningBinary
+	dstPath = constants.BasePath + constants.IAMRolesSigningBinary
+	err = copyWithPermissons(srcPath, dstPath, 0744)


World read? Even group read I would question.

TerryHowe · 2023-03-06T20:54:08Z

credentialproviderpackage/pkg/constants/constants.go

@@ -0,0 +1,41 @@
+package constants
+
+const (


Why globals? Each thing should have their own constants and most likely all or at least some of these can be lower case for reduced scope

TerryHowe · 2023-03-06T20:58:04Z

credentialproviderpackage/skaffold.yaml

@@ -0,0 +1,24 @@
+apiVersion: skaffold/v3


For vscode users maybe

lewisdiamond · 2023-03-07T14:47:29Z

credentialproviderpackage/internal/test/files.go

+	wantFile := ReadFile(t, wantPath)
+
+	if gotFile != wantFile {
+		cmd := exec.Command("diff", wantPath, gotPath)


Do we really need to diff? This adds a dependency to an OS binary which could be avoided by simply running a hash.

I think it is a small price to pay for easy test debugging. Do we need to support running on windows?

This is diff in test directory so it will not be in needed in the final image

lewisdiamond · 2023-03-07T15:48:18Z

credentialproviderpackage/pkg/configurator/linux/linux.go

+	if err != nil {
+		return err
+	}
+	return nil


There are many instances of similar things across the PR. It doesn't affect functionality but the if is unnecessary.

Suggested change

if err != nil {

return err

}

return nil

return err

lewisdiamond · 2023-03-07T15:51:43Z

credentialproviderpackage/pkg/templater/testdata/test_indent_template.yaml

+key2: {{ .Key2 }}
+{{ if .Conditional }}
+{{ .KeyAndValue3 | indent 2 }}
+{{ end }}


no newline at EOF

This package and filewriter were copied from eks-anywhere. I would like to keep the consistency between the two

Text files need to end with \n under the unix spec. If this comes from the partialyaml.go trimsuffix, we should remove that or fix the logic.

lewisdiamond · 2023-03-07T15:55:23Z

credentialproviderpackage/pkg/log/log.go

+)
+
+func init() {
+	InfoLogger = log.New(os.Stdout, "INFO: ", log.Ltime|log.Lshortfile)


Remove the time.

a-cool-train · 2023-03-08T17:13:00Z

credentialproviderpackage/Makefile

+
+REPO_ROOT=$(shell git rev-parse --show-toplevel)/credentialproviderpackage
+GOLANG_VERSION?="1.19"
+#GO ?= $(shell source $(REPO_ROOT)/scripts/common.sh && build::common::get_go_path $(GOLANG_VERSION))/go


Remove this if it is not used.

a-cool-train · 2023-03-08T17:18:47Z

credentialproviderpackage/charts/credential-provider-package/templates/daemonset.yaml

+            - name: aws-creds
+              mountPath: /secrets/aws-creds
+            {{ $os := include "template.getOSName" .}}
+            {{- if eq $os "bottlerocket" }}


I think we should reduce the logic here. Maybe move it to the helpers.tpl?

Same as other comment

a-cool-train · 2023-03-08T17:19:04Z

credentialproviderpackage/charts/credential-provider-package/templates/daemonset.yaml

+          secret:
+            secretName: {{.Values.application.secretName}}
+            optional: false
+        {{- if eq $os "bottlerocket" }}


I think we should reduce the logic here. Maybe move it to the helpers.tpl?

Strongly prefer to keep this logic here. Makes the daemonset on its own much easier to read then having to figure out whats mounted from helpers.tpl

a-cool-train · 2023-03-08T17:20:31Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+
+import (
+	_ "embed"
+	"github.com/fsnotify/fsnotify"


Can we try to run a gofmt on this and see if this is formatted correctly.

a-cool-train · 2023-03-08T17:26:50Z

credentialproviderpackage/cmd/aws-credential-provider/main.go

+		configurator = linux.NewLinuxConfigurator()
+	}
+
+	configurator.Initialize(config)


I second Terry on breaking this code down. And also I think the main file has much more code than usual. Maybe break this down into several easily readable files.

Do you guys have an example for what you would like to see. Currently the main file calls 5 functions, a constructor for the configurator, initializer, credential creator, create config, and commit changes. That is basically all the application really handles so I am not sure of the value of making it a seperate file.

a-cool-train · 2023-03-08T17:28:33Z

credentialproviderpackage/pkg/configurator/bottlerocket/testdata/testcreds

@@ -0,0 +1,3 @@
+[profile eksa-packages]
+aws_access_key_id=AKIAIOSFODNN7EXAMPLE
+aws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY


Add a new line.

…nitialize interface to remove socketpath and add that to bottlerocket constructor, Removed Notes from chart, moved from Deployment to Daemonset

…r to Amazon Linux 2

…structor itself.

lewisdiamond · 2023-03-13T13:35:27Z

.github/workflows/gosec.yml

@@ -17,4 +17,4 @@ jobs:
      - name: Run Gosec Security Scanner
        uses: securego/gosec@master
        with:
-          args: --exclude-dir=kubetest-plugins --exclude-dir generatebundlefile --exclude-dir ecrtokenrefresher  ./...


Why do we have this?

This is for the github workflows

Right, but why is gosec ignoring parts of our repo?

Created issue #870 to follow up on this

lewisdiamond · 2023-03-13T13:37:42Z

credentialproviderpackage/charts/credential-provider-package/values.yaml

+# Default values for credential-provider.
+# This is a YAML-formatted file.
+
+replicaCount: 1


This is most likely unused.

TerryHowe

/lgtm

eks-distro-bot · 2023-03-14T12:15:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: junshun, lewisdiamond, TerryHowe

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~~ [TerryHowe,junshun,lewisdiamond]

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

junshun requested a review from lewisdiamond February 21, 2023 00:47

junshun self-assigned this Feb 21, 2023

eks-distro-bot added the do-not-merge/work-in-progress label Feb 21, 2023

eks-distro-bot added the approved label Feb 21, 2023

eks-distro-bot added size/XXL do-not-merge/hold labels Feb 21, 2023

junshun marked this pull request as ready for review February 24, 2023 18:36

eks-distro-bot removed the do-not-merge/work-in-progress label Feb 24, 2023

junshun requested a review from TerryHowe February 24, 2023 18:37

junshun changed the title ~~[DRAFT] Adding Credential Provider Package with source and helm chart~~ Adding Credential Provider Package with source and helm chart Feb 24, 2023

eks-distro-bot removed the do-not-merge/hold label Feb 27, 2023

TerryHowe reviewed Feb 28, 2023

View reviewed changes

lewisdiamond requested changes Feb 28, 2023

View reviewed changes

lewisdiamond reviewed Feb 28, 2023

View reviewed changes

lewisdiamond reviewed Mar 3, 2023

View reviewed changes

TerryHowe reviewed Mar 6, 2023

View reviewed changes

lewisdiamond reviewed Mar 7, 2023

View reviewed changes

a-cool-train reviewed Mar 8, 2023

View reviewed changes

Adding Credential Package with source and helm chart

1e5db53

junshun added 13 commits March 10, 2023 17:07

Adding new line to end of Dockerfile and Makefile

084346c

Changed matchImages to take array instead of one repository. Change I…

c6b3a91

…nitialize interface to remove socketpath and add that to bottlerocket constructor, Removed Notes from chart, moved from Deployment to Daemonset

Namespacing serviceaccount.yaml and removing comments from values.yaml

4011c00

Copy binaries always to cover update case. Update references to docke…

32cfebd

…r to Amazon Linux 2

Adding AWS Profile to be configurable

09764a3

Update go version to 1.19, cleanup chart, cleanup bottlerocket tests

20cf973

Move util to pkg/log. Update to go 1.19 in makefile

e0fc178

Removing time from log and updating global reads to group reads

a82c2e1

Updating Gosec

24ea5ad

Update Makefile for go version

e0e4d52

Formatting fixes

55c13a7

Allowed arm builds for linux

2aa00ee

Moving constants to individual files. Moved BR socket logic to BR con…

a7e35d0

…structor itself.

junshun force-pushed the credential-package branch from 5b8853c to a7e35d0 Compare March 10, 2023 17:07

TerryHowe approved these changes Mar 10, 2023

View reviewed changes

lewisdiamond reviewed Mar 13, 2023

View reviewed changes

junshun added 2 commits March 13, 2023 13:53

Removing unused replicas in values.yaml

0b49d95

Adding new lines to partialyaml and updated tests

1147cb2

lewisdiamond approved these changes Mar 13, 2023

View reviewed changes

TerryHowe approved these changes Mar 14, 2023

View reviewed changes

eks-distro-bot assigned TerryHowe Mar 14, 2023

eks-distro-bot added the lgtm label Mar 14, 2023

eks-distro-bot merged commit 318fba4 into aws:main Mar 14, 2023
3 checks passed

		checkErrAndLog(err, utils.ErrorLogger)

		utils.InfoLogger.Println("Kubelet Restarted")

	utils.ErrorLogger.Println("Missing Environment Variable OS")
	utils.ErrorLogger.Println("Missing Environment Variable OS_TYPE")

Adding Credential Provider Package with source and helm chart #829

Adding Credential Provider Package with source and helm chart #829

Conversation

junshun commented Feb 21, 2023

eks-distro-bot commented Feb 21, 2023

junshun commented Feb 21, 2023

junshun commented Feb 27, 2023

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-cool-train Mar 8, 2023 • edited

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

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

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

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

a-cool-train Mar 8, 2023 •

edited