OCPBUGS-62792: Add unit tests for pkg/controller/controllercmd/cmd.go #2182
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,312 @@ | ||
| package controllercmd | ||
|
|
||
| import ( | ||
| "crypto/x509" | ||
| "encoding/pem" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" | ||
| ) | ||
|
|
||
| func TestAddDefaultRotationToConfig_SelfSignedCertLifetime(t *testing.T) { | ||
| // Skip if service-serving-cert files exist, as we need to test the self-signed cert path | ||
| if hasServiceServingCerts("/var/run/secrets/serving-cert") { | ||
| t.Skip("skipping: service-serving-cert exists, cannot test self-signed cert generation") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. To avoid this, we should rather create a private method and test that: func (c *ControllerCommandConfig) AddDefaultRotationToConfig(config *operatorv1alpha1.GenericOperatorConfig, configContent []byte) (map[string][]byte, []string, error) {
return c.addDefaultRotationToConfigAtPath("/var/run/secrets/serving-cert", config, configContent)
}
func (c *ControllerCommandConfig) addDefaultRotationToConfigAtPath(certDir string, config *operatorv1alpha1.GenericOperatorConfig, configContent []byte) (map[string][]byte, []string, error) {
...
}Then we can simply use a temp directory. |
||
|
|
||
| c := &ControllerCommandConfig{ | ||
| componentName: "test-controller", | ||
| basicFlags: &ControllerFlags{}, | ||
| } | ||
|
|
||
| config := &operatorv1alpha1.GenericOperatorConfig{ | ||
| ServingInfo: configv1.HTTPServingInfo{}, | ||
| } | ||
| config.ServingInfo.CertFile = "" | ||
| config.ServingInfo.KeyFile = "" | ||
|
Contributor
There was a problem hiding this comment. Hasn't this just been initialized to an empty object? |
||
|
|
||
| _, _, err := c.AddDefaultRotationToConfig(config, []byte{}) | ||
|
jubittajohn marked this conversation as resolved.
|
||
| if err != nil { | ||
| t.Fatalf("AddDefaultRotationToConfig failed: %v", err) | ||
| } | ||
|
|
||
| if config.ServingInfo.CertFile == "" { | ||
| t.Fatal("expected CertFile to be set, got empty string") | ||
| } | ||
| if config.ServingInfo.KeyFile == "" { | ||
| t.Fatal("expected KeyFile to be set, got empty string") | ||
| } | ||
|
|
||
| certPEM, err := os.ReadFile(config.ServingInfo.CertFile) | ||
| if err != nil { | ||
| t.Fatalf("failed to read generated certificate: %v", err) | ||
| } | ||
|
|
||
| block, _ := pem.Decode(certPEM) | ||
| if block == nil { | ||
| t.Fatal("failed to parse certificate PEM") | ||
| } | ||
|
|
||
| cert, err := x509.ParseCertificate(block.Bytes) | ||
| if err != nil { | ||
| t.Fatalf("failed to parse certificate: %v", err) | ||
| } | ||
|
|
||
| actualLifetime := cert.NotAfter.Sub(cert.NotBefore) | ||
| expectedLifetime := 30 * 24 * time.Hour | ||
|
|
||
| // Allow 1 minute tolerance | ||
| tolerance := 1 * time.Minute | ||
| lowerBound := expectedLifetime - tolerance | ||
| upperBound := expectedLifetime + tolerance | ||
|
|
||
| if actualLifetime < lowerBound || actualLifetime > upperBound { | ||
| t.Errorf("certificate lifetime is incorrect: expected %v (30 days), got %v", expectedLifetime, actualLifetime) | ||
| } | ||
|
|
||
| // Ensure the certificate is not already expired (would indicate nanosecond lifetime bug) | ||
| if time.Now().After(cert.NotAfter) { | ||
| t.Errorf("certificate is already expired - likely created with nanosecond lifetime (the bug)") | ||
| } | ||
|
|
||
| // Verify the certificate won't expire in the next 29 days | ||
| minValidUntil := time.Now().Add(29 * 24 * time.Hour) | ||
| if cert.NotAfter.Before(minValidUntil) { | ||
| t.Errorf("certificate expires too soon: expected valid for ~30 days, expires %v", cert.NotAfter) | ||
| } | ||
| } | ||
|
|
||
| func TestHasServiceServingCerts(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| setup func(string) error | ||
| expected bool | ||
| }{ | ||
| { | ||
| name: "both cert and key exist", | ||
| setup: func(dir string) error { | ||
| if err := os.WriteFile(filepath.Join(dir, "tls.crt"), []byte("cert"), 0644); err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(filepath.Join(dir, "tls.key"), []byte("key"), 0644) | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "only cert exists", | ||
| setup: func(dir string) error { | ||
| return os.WriteFile(filepath.Join(dir, "tls.crt"), []byte("cert"), 0644) | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "only key exists", | ||
| setup: func(dir string) error { | ||
| return os.WriteFile(filepath.Join(dir, "tls.key"), []byte("key"), 0644) | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "neither exists", | ||
| setup: func(dir string) error { return nil }, | ||
| expected: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| if err := tt.setup(tmpDir); err != nil { | ||
| t.Fatalf("setup failed: %v", err) | ||
| } | ||
|
|
||
| result := hasServiceServingCerts(tmpDir) | ||
|
Contributor
There was a problem hiding this comment. Honestly this function is weird. It returns |
||
| if result != tt.expected { | ||
| t.Errorf("expected %v, got %v", tt.expected, result) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestAddDefaultRotationToConfig(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| configFile string | ||
| existingCertFile string | ||
| existingKeyFile string | ||
| expectGenerated bool | ||
|
Contributor
There was a problem hiding this comment. It would be really nice to add docs to the fields so that it's obvious what setting that field does. |
||
| expectObservedLen int | ||
| }{ | ||
| { | ||
| name: "generates self-signed cert when none specified", | ||
| expectGenerated: true, | ||
| expectObservedLen: 2, // service cert paths | ||
| }, | ||
| { | ||
| name: "does not generate when certs already specified", | ||
| existingCertFile: "/existing/tls.crt", | ||
| existingKeyFile: "/existing/tls.key", | ||
| expectGenerated: false, | ||
| expectObservedLen: 2, | ||
| }, | ||
| { | ||
| name: "observes config file when provided", | ||
| configFile: "test-config.yaml", | ||
| expectGenerated: true, | ||
| expectObservedLen: 3, // service cert paths + config file | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| var configFile string | ||
| if tt.configFile != "" { | ||
| configFile = filepath.Join(tmpDir, tt.configFile) | ||
| if err := os.WriteFile(configFile, []byte("test-content"), 0644); err != nil { | ||
| t.Fatalf("failed to create config file: %v", err) | ||
| } | ||
| } | ||
|
|
||
| c := &ControllerCommandConfig{ | ||
| componentName: "test-controller", | ||
| basicFlags: &ControllerFlags{ | ||
| ConfigFile: configFile, | ||
| }, | ||
| } | ||
|
|
||
| config := &operatorv1alpha1.GenericOperatorConfig{ | ||
| ServingInfo: configv1.HTTPServingInfo{}, | ||
| } | ||
| config.ServingInfo.CertFile = tt.existingCertFile | ||
| config.ServingInfo.KeyFile = tt.existingKeyFile | ||
|
|
||
| startingContent, observedFiles, err := c.AddDefaultRotationToConfig(config, []byte("test-content")) | ||
| if err != nil { | ||
| t.Fatalf("AddDefaultRotationToConfig failed: %v", err) | ||
| } | ||
|
|
||
| if tt.expectGenerated { | ||
| if config.ServingInfo.CertFile == "" { | ||
| t.Error("expected CertFile to be generated") | ||
| } | ||
| if config.ServingInfo.KeyFile == "" { | ||
| t.Error("expected KeyFile to be generated") | ||
| } | ||
| } else { | ||
| if config.ServingInfo.CertFile != tt.existingCertFile { | ||
| t.Errorf("expected CertFile to remain %q, got %q", tt.existingCertFile, config.ServingInfo.CertFile) | ||
| } | ||
| if config.ServingInfo.KeyFile != tt.existingKeyFile { | ||
| t.Errorf("expected KeyFile to remain %q, got %q", tt.existingKeyFile, config.ServingInfo.KeyFile) | ||
| } | ||
| } | ||
|
|
||
| if len(observedFiles) != tt.expectObservedLen { | ||
| t.Errorf("expected %d observed files, got %d: %v", tt.expectObservedLen, len(observedFiles), observedFiles) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. I find this whole test a bit confusing. We are setting |
||
|
|
||
| // Config file should be in observed files if provided | ||
| if tt.configFile != "" { | ||
| found := false | ||
| for _, f := range observedFiles { | ||
| if f == configFile { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("expected config file %q in observed files", configFile) | ||
| } | ||
|
|
||
| // Config file should be in starting content | ||
| if _, ok := startingContent[configFile]; !ok { | ||
| t.Error("expected config file in starting content") | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestConfig(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| configContent string | ||
| expectError bool | ||
| expectNilUnstr bool | ||
| validateConfig func(*testing.T, *operatorv1alpha1.GenericOperatorConfig) | ||
|
|
||
| }{ | ||
| { | ||
| name: "no config file", | ||
| expectNilUnstr: true, | ||
| validateConfig: func(t *testing.T, config *operatorv1alpha1.GenericOperatorConfig) { | ||
| if config == nil { | ||
| t.Error("expected config to be initialized") | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| name: "valid config file", | ||
| configContent: ` | ||
| apiVersion: operator.openshift.io/v1alpha1 | ||
| kind: GenericOperatorConfig | ||
| servingInfo: | ||
| bindAddress: https://0.0.0.0:8443 | ||
| leaderElection: | ||
| leaseDuration: 90s | ||
| `, | ||
| expectNilUnstr: false, | ||
| validateConfig: func(t *testing.T, config *operatorv1alpha1.GenericOperatorConfig) { | ||
| if config.ServingInfo.BindAddress != "https://0.0.0.0:8443" { | ||
| t.Errorf("expected BindAddress to be parsed correctly, got %q", config.ServingInfo.BindAddress) | ||
| } | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| c := &ControllerCommandConfig{ | ||
| componentName: "test", | ||
| basicFlags: &ControllerFlags{}, | ||
| } | ||
|
|
||
| if tt.configContent != "" { | ||
| tmpDir := t.TempDir() | ||
| configFile := filepath.Join(tmpDir, "config.yaml") | ||
| if err := os.WriteFile(configFile, []byte(tt.configContent), 0644); err != nil { | ||
| t.Fatalf("failed to write config file: %v", err) | ||
| } | ||
| c.basicFlags.ConfigFile = configFile | ||
| } | ||
|
|
||
| unstructured, config, content, err := c.Config() | ||
|
|
||
| if tt.expectError && err == nil { | ||
| t.Error("expected error but got none") | ||
| } | ||
| if !tt.expectError && err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if tt.expectNilUnstr && unstructured != nil { | ||
| t.Error("expected unstructured to be nil") | ||
| } | ||
| if !tt.expectNilUnstr && unstructured == nil { | ||
| t.Error("expected unstructured to be non-nil") | ||
| } | ||
|
|
||
| if tt.validateConfig != nil { | ||
| tt.validateConfig(t, config) | ||
| } | ||
|
|
||
| if tt.configContent != "" && content == nil { | ||
| t.Error("expected content to be set when config file exists") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Can we have something more declarative rather then this bunch of ifs pls? |
||
| }) | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I find it very helpful to add a docstring to each test explaining what is being tested and potentially why we need the test. Just the overall context, you know.